Review Request 127437: Fix many threading issues in KUrlCompletion

[David Edmundson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127437/#review93863
-----------------------------------------------------------


Ship it!




Ship It!

- David Edmundson


On March 20, 2016, 2:46 p.m., David Faure wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127437/
> -----------------------------------------------------------
> 
> (Updated March 20, 2016, 2:46 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> -------
> 
> * Major race with other threads due to using QDir::setCurrent()
> * Race conditions on m_terminationRequested and m_matches
> * Race with previous completion thread when its posted event arrives after cancelling
> * Cancellation code spread out in many methods and never done fully correctly
> * isRunning() was missing one of the two threads, making unit test fail in valgrind
> * Fix the rarely-hit code path where the thread isn't finished after 200ms
>    - the current search string was lost because finished() wasn't called
>    - the matches were not used, in case of user-completion (AFAICS)
>    - changing the search string while the thread was running could lead to the old search
>      string still being used for completion
>      (the misnamed finished() wasn't called, so KCompletion didn't get the new string)
>   => added a variant of the unittest which doesn't wait for the thread initially
> 
> + Simplify code using signal/slots rather than a custom event
> + Simplify code using enum rather than casting to/from int
> 
> Most of these bugs are from 2004 (e.g. commit ec83c408619e3) ;)
> 
> REVIEW: 127437
> 
> 
> Diffs
> -----
> 
>   autotests/CMakeLists.txt 4dff0a24d31897a9641a70017bd87e33415cef14 
>   autotests/kurlcompletiontest.cpp eef39ff17940fadcb9492e5e7092070c976310d4 
>   src/widgets/CMakeLists.txt 87dac50b377f6ea4c3cd39e9afa37a4680aecf31 
>   src/widgets/kurlcompletion.h 14fda22a0e08bcf2da30e19fed577c8bda64a4ca 
>   src/widgets/kurlcompletion.cpp 1dc8f4898fb78d6f49e687462446007a10305f98 
> 
> Diff: https://git.reviewboard.kde.org/r/127437/diff/
> 
> 
> Testing
> -------
> 
> Hit a crash in DirectoryListThread when playing with kopenwithtest.
> 
> kurlcompletiontest extended, kurlcompletion-nowait added, both pass, also in valgrind (different timings).
> 
> 
> Thanks,
> 
> David Faure
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160322/7eb617ac/attachment.html>