Summary: | DFG::Worklist should acquire the m_lock before iterating DFG plans | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | fpizlo, ggaren, mhahnenberg, mmirman, msaboff, oliver, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Mark Lam
2014-04-22 17:13:40 PDT
Interesting! That's a good catch. Looks like Worklist::visitChildren needs to lock m_lock. Is that what you were thinking? (In reply to comment #2) > Interesting! That's a good catch. Looks like Worklist::visitChildren needs to lock m_lock. Is that what you were thinking? Yes. And also Worklist::isActiveForVM(). (In reply to comment #3) > (In reply to comment #2) > > Interesting! That's a good catch. Looks like Worklist::visitChildren needs to lock m_lock. Is that what you were thinking? > > Yes. And also Worklist::isActiveForVM(). Oh, right! Agreed. Created attachment 229928 [details]
the patch
Comment on attachment 229928 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=229928&action=review R=me. > Source/JavaScriptCore/dfg/DFGWorklist.cpp:237 > for (unsigned i = m_threads.size(); i--;) { Can you do us a favor and add a comment on top of this loop that says that it doesn't need further locking because (1) no new threads can be added to m_threads so that data structure is immutable and needs no locks, and (2) ThreadDatea::m_safepoint is protected by that thread's rightToRun which we must be holding here because of a prior call to suspendAllThreads(). Or something to that effect. (In reply to comment #6) > Can you do us a favor and add a comment on top of this loop that says that it doesn't need further locking because (1) no new threads can be added to m_threads so that data structure is immutable and needs no locks, and (2) ThreadDatea::m_safepoint is protected by that thread's rightToRun which we must be holding here because of a prior call to suspendAllThreads(). Or something to that effect. Thanks. I love comments. =) It's done. Landed in r167692: <http://trac.webkit.org/r167692>. |