Changeset 167692 in webkit

Timestamp:

Apr 22, 2014 5:36:49 PM (10 years ago)

Author:

mark.lam@apple.com

Message:

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):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• dfg/DFGWorklist.cpp (modified) (2 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/dfg/DFGWorklist.cpp

@@ -77 +77 @@
 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) {
@@ -223 +224 @@
 {
     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();