Title: [253358] trunk/Source/_javascript_Core
- Revision
- 253358
- Author
- [email protected]
- Date
- 2019-12-10 17:45:00 -0800 (Tue, 10 Dec 2019)
Log 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):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (253357 => 253358)
--- trunk/Source/_javascript_Core/ChangeLog 2019-12-11 01:35:14 UTC (rev 253357)
+++ trunk/Source/_javascript_Core/ChangeLog 2019-12-11 01:45:00 UTC (rev 253358)
@@ -1,3 +1,36 @@
+2019-12-10 Mark Lam <[email protected]>
+
+ 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):
+
2019-12-10 Saam Barati <[email protected]>
methodOfGettingAValueProfileFor should return argument value profiles even when node and operandNode are the same origin
Modified: trunk/Source/_javascript_Core/dfg/DFGWorklist.cpp (253357 => 253358)
--- trunk/Source/_javascript_Core/dfg/DFGWorklist.cpp 2019-12-11 01:35:14 UTC (rev 253357)
+++ trunk/Source/_javascript_Core/dfg/DFGWorklist.cpp 2019-12-11 01:45:00 UTC (rev 253358)
@@ -307,10 +307,20 @@
void Worklist::deleteCancelledPlansForVM(LockHolder&, VM& vm)
{
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) {
RefPtr<Plan> plan = m_cancelledPlansPendingDestruction[i];
if (plan->unnukedVM() != &vm)
@@ -317,18 +327,15 @@
continue;
m_cancelledPlansPendingDestruction[i--] = m_cancelledPlansPendingDestruction.last();
m_cancelledPlansPendingDestruction.removeLast();
-#if !ASSERT_DISABLED
- removedPlans.add(plan);
-#endif
+ removedPlans.add(WTFMove(plan));
}
-#if !ASSERT_DISABLED
while (!removedPlans.isEmpty()) {
RefPtr<Plan> plan = removedPlans.takeAny();
- RELEASE_ASSERT(plan->stage() == Plan::Cancelled);
- RELEASE_ASSERT(plan->refCount() == 1);
+ ASSERT(plan->stage() == Plan::Cancelled);
+ if (plan->refCount() > 1)
+ m_cancelledPlansPendingDestruction.append(WTFMove(plan));
}
-#endif
}
void Worklist::removeAllReadyPlansForVM(VM& vm, Vector<RefPtr<Plan>, 8>& myReadyPlans)
@@ -459,7 +466,7 @@
RefPtr<Plan> plan = m_plans.take(*iter);
plan->cancel();
if (!isInMutator)
- m_cancelledPlansPendingDestruction.append(plan);
+ m_cancelledPlansPendingDestruction.append(WTFMove(plan));
}
Deque<RefPtr<Plan>> newQueue;
while (!m_queue.isEmpty()) {
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
