Changeset 253358 in webkit for trunk/Source/JavaScriptCore/dfg

Timestamp:

Dec 10, 2019, 5:45:00 PM (5 years ago)

Author:

[email protected]

Message:

Worklist::deleteCancelledPlansForVM() should not assume that a cancelled plan is ready for deletion.
​https://bugs.webkit.org/show_bug.cgi?id=205086
<rdar://problem/57795002>

Reviewed by Saam Barati.

Consider this race scenario:

1. The DFG thread finds a plan and started compiling, and it's holding a ref to the plan while it's compiling.
2. The GC thread discovers that we no longer need the plan and cancels it.
3. After the plan is cancelled but while the DFG thread is still compiling, the mutator thread calls Worklist::deleteCancelledPlansForVM().

Worklist::deleteCancelledPlansForVM() was assuming that by the time it is
called, Worklist::m_cancelledPlansPendingDestruction will contain the last ref
to the cancelled plan. However, this is an incorrect assumption, and the
assertion there that asserts refCount == 1 will fail.

This patch fixes Worklist::deleteCancelledPlansForVM() to append the cancelled
plan back into m_cancelledPlansPendingDestruction if its refCount is not 1
(implying that the compiler thread still has a ref to it), and defer deletion of
the plan to a subsequent call to deleteCancelledPlansForVM().

This patch also adds a WTFMove to Worklist::removeDeadPlans() when we append the
cancelled plan to m_cancelledPlansPendingDestruction there. This saves us one
unnecessary ref and deref of the plan.

• dfg/DFGWorklist.cpp:

(JSC::DFG::Worklist::deleteCancelledPlansForVM):
(JSC::DFG::Worklist::removeDeadPlans):

File:

• trunk/Source/JavaScriptCore/dfg/DFGWorklist.cpp (modified) (3 diffs)

trunk/Source/JavaScriptCore/dfg/DFGWorklist.cpp

@@ -308 +308 @@
 {
     RELEASE_ASSERT(vm.currentThreadIsHoldingAPILock());
-#if !ASSERT_DISABLED
     HashSet<RefPtr<Plan>> removedPlans;
-#endif
+
+    // The following scenario can occur:
+    // 1. The DFG thread started compiling a plan.
+    // 2. The GC thread cancels the plan, and adds it to m_cancelledPlansPendingDestruction.
+    // 3. The DFG thread finishes compiling, and discovers that the thread is cancelled.
+    //    To avoid destructing the plan in the DFG thread, it adds it to
+    //    m_cancelledPlansPendingDestruction.
+    // 4. The above occurs before the mutator runs deleteCancelledPlansForVM().
+    //
+    // Hence, the same cancelled plan can appear in m_cancelledPlansPendingDestruction
+    // more than once. This is why we need to filter the cancelled plans through
+    // the removedPlans HashSet before we do the refCount check below.
 
     for (size_t i = 0; i < m_cancelledPlansPendingDestruction.size(); ++i) {
@@ -318 +328 @@
         m_cancelledPlansPendingDestruction[i--] = m_cancelledPlansPendingDestruction.last();
         m_cancelledPlansPendingDestruction.removeLast();
-#if !ASSERT_DISABLED
-        removedPlans.add(plan);
-#endif
-    }
-
-#if !ASSERT_DISABLED
+        removedPlans.add(WTFMove(plan));
+    }
+
     while (!removedPlans.isEmpty()) {
         RefPtr<Plan> plan = removedPlans.takeAny();
-        RELEASE_ASSERT(plan->stage() == Plan::Cancelled);
-        RELEASE_ASSERT(plan->refCount() == 1);
-    }
-#endif
+        ASSERT(plan->stage() == Plan::Cancelled);
+        if (plan->refCount() > 1)
+            m_cancelledPlansPendingDestruction.append(WTFMove(plan));
+    }
 }
 
@@ -460 +467 @@
                 plan->cancel();
                 if (!isInMutator)
-                    m_cancelledPlansPendingDestruction.append(plan);
+                    m_cancelledPlansPendingDestruction.append(WTFMove(plan));
             }
             Deque<RefPtr<Plan>> newQueue;