Review Request: Make Sonnet::Speller::setLanguage threadsafe, fixing segfaults in krunner

[David Faure]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106242/#review18623
-----------------------------------------------------------


A number of comments on the implementation, but also a more general comment: does it even make sense to use a Speller in multiple threads, and to  change the language from one thread while another one is using the speller for spell-checking? It sounds like, even if a mutex/lock can prevent crashes, this is a weird thing to do anyway, since you have no idea at which point the spell-checking will switch to another language... could happen in the very middle of a sentence...

Maybe it would make more sense to "post" the language change operation to the spell-checking thread using the same mechanism as the one used to "post" spellchecking requests to it? (Disclaimer: I know nothing of the krunner architecture).


kdecore/sonnet/speller.cpp
<http://git.reviewboard.kde.org/r/106242/#comment14723>

    if() before delete is unnecessary, please revert this change.



kdecore/sonnet/speller.cpp
<http://git.reviewboard.kde.org/r/106242/#comment14726>

    That line is racy, if called from two different threads (copying a pointer is not atomic). If anything else, "helgrind" will warn for sure.
    
    Move it inside the lock.



kdecore/sonnet/speller.cpp
<http://git.reviewboard.kde.org/r/106242/#comment14727>

    This line is a no-op, remove it.



kdecore/sonnet/speller.cpp
<http://git.reviewboard.kde.org/r/106242/#comment14729>

    const QString& lang = QString()
    
    (missing const ref, missing spaces)



kdecore/sonnet/speller.cpp
<http://git.reviewboard.kde.org/r/106242/#comment14728>

    coding style: if (lang.isEmpty()) {
    (space after if, space before brace). Same thing on other lines below.



kdecore/sonnet/speller.cpp
<http://git.reviewboard.kde.org/r/106242/#comment14730>

    If setLanguage is called from another thread as the thread using "lang", then this creates a race on "lang".
    Protect it with a mutex.
    
    More generally, it seems that all member variables here need mutex protection, not just dict. So I'd rename dictLock to lock, and use it for all members.



kdecore/sonnet/speller.cpp
<http://git.reviewboard.kde.org/r/106242/#comment14724>

    See above. Delete NULL is perfectly valid (and no-op) in C++.



kdecore/sonnet/speller.cpp
<http://git.reviewboard.kde.org/r/106242/#comment14731>

    spaces around =



kdecore/sonnet/speller.cpp
<http://git.reviewboard.kde.org/r/106242/#comment14725>

    Please use QReadLocker instead. This will simplify the code, no need for the 3-liner return at the end of the method.


- David Faure


On Aug. 27, 2012, 9:33 p.m., Simeon Bird wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106242/
> -----------------------------------------------------------
> 
> (Updated Aug. 27, 2012, 9:33 p.m.)
> 
> 
> Review request for kdelibs and Plasma.
> 
> 
> Description
> -------
> 
> Krunner's spellcheck plugin has been pretty broken since bd291d21f096a714a171e7af3a534ba345ca5659 (about two years ago) because it called Sonnet::Speller::setLanguage every time the spellchecker was invoked, which was (very much) not thread-safe.
> 
> This patch makes Sonnet::Speller::setLanguage threadsafe by protecting all access to the internal dict pointer using QReadWriteLock. 
> 
> A related review request is 106244, which adds more fixes to the spellcheck feature.
> 
> 
> This addresses bugs 264779 and 303831.
>     http://bugs.kde.org/show_bug.cgi?id=264779
>     http://bugs.kde.org/show_bug.cgi?id=303831
> 
> 
> Diffs
> -----
> 
>   kdecore/sonnet/speller.cpp b19e74d 
> 
> Diff: http://git.reviewboard.kde.org/r/106242/diff/
> 
> 
> Testing
> -------
> 
> Compiled, installed, used for a week or so, spellchecked a bunch of things.
> 
> 
> Thanks,
> 
> Simeon Bird
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20120906/e81443d2/attachment-0001.html>