Summary: | REGRESSION: HipChat and Mail sometimes hang beneath JSC::Heap::lastChanceToFinalize() | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Saboff <msaboff> | ||||||
Component: | JavaScriptCore | Assignee: | Michael Saboff <msaboff> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, commit-queue, dbates, ggaren | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Michael Saboff
2016-12-16 11:07:22 PST
Created attachment 297335 [details]
Patch
Comment on attachment 297335 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297335&action=review > Source/WTF/wtf/AutomaticThread.cpp:50 > + m_condition.notifyOne(); This is sort of wrong since it makes notifyOne mean notifyTwo. Programs that use notifyOne often rely on the bounded nature of the wake-up. I think that one solution is to simply move this call to the bottom of the method: only m_condition.notifyOne() if we did not find any threads to wake up ourselves. > Source/WTF/wtf/AutomaticThread.cpp:212 > + if (!awokenByNotify && m_isWaiting) { > + m_isWaiting = false; Do we still need awokenByNotify? (In reply to comment #2) > Comment on attachment 297335 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=297335&action=review > > > Source/WTF/wtf/AutomaticThread.cpp:50 > > + m_condition.notifyOne(); > > This is sort of wrong since it makes notifyOne mean notifyTwo. > > Programs that use notifyOne often rely on the bounded nature of the wake-up. > I think that one solution is to simply move this call to the bottom of the > method: only m_condition.notifyOne() if we did not find any threads to wake > up ourselves. That makes sense. I'll make that change. > > Source/WTF/wtf/AutomaticThread.cpp:212 > > + if (!awokenByNotify && m_isWaiting) { > > + m_isWaiting = false; > > Do we still need awokenByNotify? No. I'll remove it. Created attachment 297339 [details]
Addressed Phil's comments and fixed m_condition ASSERTs
Committed r209938: <http://trac.webkit.org/changeset/209938> |