Bug 166645 - WorkQueueGeneric's platformInvalidate() can deadlock when called on the RunLoop's thread
Summary: WorkQueueGeneric's platformInvalidate() can deadlock when called on the RunLo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-01-03 02:30 PST by Zan Dobersek
Modified: 2017-01-03 23:53 PST (History)
9 users (show)

See Also:


Attachments
Patch (5.04 KB, patch)
2017-01-03 13:35 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (4.97 KB, patch)
2017-01-03 13:37 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2017-01-03 02:30:03 PST
It's possible that a WorkQueue object is destroyed on its own thread. In case of the WorkQueueGeneric implementation, that is the thread of the underlying RunLoop object.

When that occurs, platformInvalidate() in WorkQueueGeneric.cpp stops the RunLoop and then waits for a signal on the m_terminateRunLoopCondition indefinitely. That doesn't occur because the signal is supposed to be launched on the same thread after the RunLoop is stopped.
https://trac.webkit.org/browser/trunk/Source/WTF/wtf/generic/WorkQueueGeneric.cpp#L68

One solution is to not wait on m_terminateRunLoopCondition if we detect that we're on the RunLoop's thread. Another solution is to remove both m_terminateRunLoopCondition and the accompanying mutex.

First reported at https://github.com/Metrological/WebKitForWayland/pull/170
Comment 1 Carlos Garcia Campos 2017-01-03 02:34:17 PST
I think we can get rid of the terminate condition/mutex. It's ok to detach a running thread, and platformInvalidate is called from the destructor, so we don't need to protect the m_runLoop. Tasks take their own reference so it's not possible to have pending tasks at the point either.
Comment 2 Michael Catanzaro 2017-01-03 05:22:57 PST
I think Yusuke wrote this code; let's see what he thinks.
Comment 3 Yusuke Suzuki 2017-01-03 13:12:11 PST
I think we do not need to wait thread stop. Calling the detach thread ensures that the resources will be destroyed when the thread finishes.
Comment 4 Yusuke Suzuki 2017-01-03 13:35:42 PST
Created attachment 297944 [details]
Patch
Comment 5 Yusuke Suzuki 2017-01-03 13:37:15 PST
Created attachment 297945 [details]
Patch
Comment 6 Carlos Garcia Campos 2017-01-03 22:58:35 PST
Comment on attachment 297945 [details]
Patch

Thanks!
Comment 7 Yusuke Suzuki 2017-01-03 23:28:10 PST
Comment on attachment 297945 [details]
Patch

Thanks :)
Comment 8 WebKit Commit Bot 2017-01-03 23:53:01 PST
Comment on attachment 297945 [details]
Patch

Clearing flags on attachment: 297945

Committed r210271: <http://trac.webkit.org/changeset/210271>
Comment 9 WebKit Commit Bot 2017-01-03 23:53:07 PST
All reviewed patches have been landed.  Closing bug.