DFG::Worklist should acquire the m_lock before iterating DFG plans.
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 23 Apr 2014 00:36:49 +0000 (00:36 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 23 Apr 2014 00:36:49 +0000 (00:36 +0000)
<https://webkit.org/b/132032>

Reviewed by Filip Pizlo.

Currently, there's a rightToRun mechanism that ensures that no compilation
threads are running when the GC is iterating through the DFG worklists.
However, this does not prevent a Worker thread from doing a DFG compilation
and modifying the plans in the worklists thereby invalidating the plan
iterator that the GC is using.  This patch fixes the issue by acquiring
the worklist m_lock before iterating the worklist plans.

This issue was uncovered by running the fast/workers layout tests with
COLLECT_ON_EVERY_ALLOCATION enabled.

* dfg/DFGWorklist.cpp:
(JSC::DFG::Worklist::isActiveForVM):
(JSC::DFG::Worklist::visitChildren):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@167692 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGWorklist.cpp

index 2153b63..6f3d96b 100644 (file)
@@ -1,3 +1,24 @@
+2014-04-22  Mark Lam  <mark.lam@apple.com>
+
+        DFG::Worklist should acquire the m_lock before iterating DFG plans.
+        <https://webkit.org/b/132032>
+
+        Reviewed by Filip Pizlo.
+
+        Currently, there's a rightToRun mechanism that ensures that no compilation
+        threads are running when the GC is iterating through the DFG worklists.
+        However, this does not prevent a Worker thread from doing a DFG compilation
+        and modifying the plans in the worklists thereby invalidating the plan
+        iterator that the GC is using.  This patch fixes the issue by acquiring
+        the worklist m_lock before iterating the worklist plans.
+
+        This issue was uncovered by running the fast/workers layout tests with
+        COLLECT_ON_EVERY_ALLOCATION enabled.
+
+        * dfg/DFGWorklist.cpp:
+        (JSC::DFG::Worklist::isActiveForVM):
+        (JSC::DFG::Worklist::visitChildren):
+
 2014-04-22  Brent Fulgham  <bfulgham@apple.com>
 
         [Win] Support Python 2.7 in Cygwin
index 883394c..c16c550 100644 (file)
@@ -76,6 +76,7 @@ PassRefPtr<Worklist> Worklist::create(unsigned numberOfThreads, int relativePrio
 
 bool Worklist::isActiveForVM(VM& vm) const
 {
+    MutexLocker locker(m_lock);
     PlanMap::const_iterator end = m_plans.end();
     for (PlanMap::const_iterator iter = m_plans.begin(); iter != end; ++iter) {
         if (&iter->value->vm == &vm)
@@ -222,14 +223,21 @@ void Worklist::resumeAllThreads()
 void Worklist::visitChildren(SlotVisitor& visitor, CodeBlockSet& codeBlocks)
 {
     VM* vm = visitor.heap()->vm();
-    for (PlanMap::iterator iter = m_plans.begin(); iter != m_plans.end(); ++iter) {
-        Plan* plan = iter->value.get();
-        if (&plan->vm != vm)
-            continue;
-        iter->key.visitChildren(codeBlocks);
-        iter->value->visitChildren(visitor, codeBlocks);
+    {
+        MutexLocker locker(m_lock);
+        for (PlanMap::iterator iter = m_plans.begin(); iter != m_plans.end(); ++iter) {
+            Plan* plan = iter->value.get();
+            if (&plan->vm != vm)
+                continue;
+            iter->key.visitChildren(codeBlocks);
+            iter->value->visitChildren(visitor, codeBlocks);
+        }
     }
     
+    // This loop doesn't need further locking because:
+    // (1) no new threads can be added to m_threads. Hence, it is immutable and needs no locks.
+    // (2) ThreadData::m_safepoint is protected by that thread's m_rightToRun which we must be
+    //     holding here because of a prior call to suspendAllThreads().
     for (unsigned i = m_threads.size(); i--;) {
         ThreadData* data = m_threads[i].get();
         Safepoint* safepoint = data->m_safepoint;