- Revision
- 200933
- Author
- [email protected]
- Date
- 2016-05-15 16:08:21 -0700 (Sun, 15 May 2016)
Log Message
DFG::Plan shouldn't read from its VM once it's been cancelled
https://bugs.webkit.org/show_bug.cgi?id=157726
Reviewed by Saam Barati.
Plan::vm was a reference, not a pointer, and so wasn't nulled by Plan::cancel(). So, a
cancelled plan may have a dangling pointer to a VM: we could delete the VM after cancelling
the plan.
Prior to http://trac.webkit.org/changeset/200705, this was probably fine because nobody
would read Plan::vm if the plan was cancelled. But r200705 changed that. It was a hard
regression to spot because usually a cancelled plan will still refer to a valid VM.
This change fixes the regression and makes it a lot easier to spot the regression in the
future. Plan::vm is now a pointer and we null it in Plan::cancel(). Now if you make this
mistake, you will get a crash anytime the Plan is cancelled, not just anytime the plan is
cancelled and the VM gets deleted. Also, it's now very clear what to do when you want to
use Plan::vm on the cancel path: you can null-check vm; if it's null, assume the worst.
Because we null the VM of a cancelled plan, we cannot have Safepoint::vm() return the
plan's VM anymore. That's because when we cancel a plan that is at a safepoint, we use the
safepoint's VM to determine whether this is one of our safepoints *after* the plan is
already cancelled. So, Safepoint now has its own copy of m_vm, and that copy gets nulled
when the Safepoint is cancelled. The Safepoint's m_vm will be nulled moments after Plan's
vm gets nulled (see Worklist::removeDeadPlans(), which has a cancel path for Plans in one
loop and a cancel path for Safepoints in the loop after it).
* dfg/DFGJITFinalizer.cpp:
(JSC::DFG::JITFinalizer::finalizeCommon):
* dfg/DFGPlan.cpp:
(JSC::DFG::Plan::Plan):
(JSC::DFG::Plan::computeCompileTimes):
(JSC::DFG::Plan::reportCompileTimes):
(JSC::DFG::Plan::compileInThreadImpl):
(JSC::DFG::Plan::reallyAdd):
(JSC::DFG::Plan::notifyCompiling):
(JSC::DFG::Plan::finalizeWithoutNotifyingCallback):
(JSC::DFG::Plan::cancel):
* dfg/DFGPlan.h:
(JSC::DFG::Plan::canTierUpAndOSREnter):
* dfg/DFGSafepoint.cpp:
(JSC::DFG::Safepoint::cancel):
(JSC::DFG::Safepoint::vm):
* dfg/DFGSafepoint.h:
* dfg/DFGWorklist.cpp:
(JSC::DFG::Worklist::isActiveForVM):
(JSC::DFG::Worklist::waitUntilAllPlansForVMAreReady):
(JSC::DFG::Worklist::removeAllReadyPlansForVM):
(JSC::DFG::Worklist::rememberCodeBlocks):
(JSC::DFG::Worklist::visitWeakReferences):
(JSC::DFG::Worklist::removeDeadPlans):
(JSC::DFG::Worklist::runThread):
* ftl/FTLJITFinalizer.cpp:
(JSC::FTL::JITFinalizer::finalizeFunction):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (200932 => 200933)
--- trunk/Source/_javascript_Core/ChangeLog 2016-05-15 22:13:53 UTC (rev 200932)
+++ trunk/Source/_javascript_Core/ChangeLog 2016-05-15 23:08:21 UTC (rev 200933)
@@ -1,3 +1,60 @@
+2016-05-15 Filip Pizlo <[email protected]>
+
+ DFG::Plan shouldn't read from its VM once it's been cancelled
+ https://bugs.webkit.org/show_bug.cgi?id=157726
+
+ Reviewed by Saam Barati.
+
+ Plan::vm was a reference, not a pointer, and so wasn't nulled by Plan::cancel(). So, a
+ cancelled plan may have a dangling pointer to a VM: we could delete the VM after cancelling
+ the plan.
+
+ Prior to http://trac.webkit.org/changeset/200705, this was probably fine because nobody
+ would read Plan::vm if the plan was cancelled. But r200705 changed that. It was a hard
+ regression to spot because usually a cancelled plan will still refer to a valid VM.
+
+ This change fixes the regression and makes it a lot easier to spot the regression in the
+ future. Plan::vm is now a pointer and we null it in Plan::cancel(). Now if you make this
+ mistake, you will get a crash anytime the Plan is cancelled, not just anytime the plan is
+ cancelled and the VM gets deleted. Also, it's now very clear what to do when you want to
+ use Plan::vm on the cancel path: you can null-check vm; if it's null, assume the worst.
+
+ Because we null the VM of a cancelled plan, we cannot have Safepoint::vm() return the
+ plan's VM anymore. That's because when we cancel a plan that is at a safepoint, we use the
+ safepoint's VM to determine whether this is one of our safepoints *after* the plan is
+ already cancelled. So, Safepoint now has its own copy of m_vm, and that copy gets nulled
+ when the Safepoint is cancelled. The Safepoint's m_vm will be nulled moments after Plan's
+ vm gets nulled (see Worklist::removeDeadPlans(), which has a cancel path for Plans in one
+ loop and a cancel path for Safepoints in the loop after it).
+
+ * dfg/DFGJITFinalizer.cpp:
+ (JSC::DFG::JITFinalizer::finalizeCommon):
+ * dfg/DFGPlan.cpp:
+ (JSC::DFG::Plan::Plan):
+ (JSC::DFG::Plan::computeCompileTimes):
+ (JSC::DFG::Plan::reportCompileTimes):
+ (JSC::DFG::Plan::compileInThreadImpl):
+ (JSC::DFG::Plan::reallyAdd):
+ (JSC::DFG::Plan::notifyCompiling):
+ (JSC::DFG::Plan::finalizeWithoutNotifyingCallback):
+ (JSC::DFG::Plan::cancel):
+ * dfg/DFGPlan.h:
+ (JSC::DFG::Plan::canTierUpAndOSREnter):
+ * dfg/DFGSafepoint.cpp:
+ (JSC::DFG::Safepoint::cancel):
+ (JSC::DFG::Safepoint::vm):
+ * dfg/DFGSafepoint.h:
+ * dfg/DFGWorklist.cpp:
+ (JSC::DFG::Worklist::isActiveForVM):
+ (JSC::DFG::Worklist::waitUntilAllPlansForVMAreReady):
+ (JSC::DFG::Worklist::removeAllReadyPlansForVM):
+ (JSC::DFG::Worklist::rememberCodeBlocks):
+ (JSC::DFG::Worklist::visitWeakReferences):
+ (JSC::DFG::Worklist::removeDeadPlans):
+ (JSC::DFG::Worklist::runThread):
+ * ftl/FTLJITFinalizer.cpp:
+ (JSC::FTL::JITFinalizer::finalizeFunction):
+
2016-05-15 Yusuke Suzuki <[email protected]>
Modernize Intl constructors; using InternalFunction::createSubclassStructure
Modified: trunk/Source/_javascript_Core/dfg/DFGJITFinalizer.cpp (200932 => 200933)
--- trunk/Source/_javascript_Core/dfg/DFGJITFinalizer.cpp 2016-05-15 22:13:53 UTC (rev 200932)
+++ trunk/Source/_javascript_Core/dfg/DFGJITFinalizer.cpp 2016-05-15 23:08:21 UTC (rev 200933)
@@ -91,7 +91,7 @@
#endif // ENABLE(FTL_JIT)
if (m_plan.compilation)
- m_plan.vm.m_perBytecodeProfiler->addCompilation(m_plan.codeBlock, m_plan.compilation);
+ m_plan.vm->m_perBytecodeProfiler->addCompilation(m_plan.codeBlock, m_plan.compilation);
if (!m_plan.willTryToTierUp)
m_plan.codeBlock->baselineVersion()->m_didFailFTLCompilation = true;
Modified: trunk/Source/_javascript_Core/dfg/DFGPlan.cpp (200932 => 200933)
--- trunk/Source/_javascript_Core/dfg/DFGPlan.cpp 2016-05-15 22:13:53 UTC (rev 200932)
+++ trunk/Source/_javascript_Core/dfg/DFGPlan.cpp 2016-05-15 23:08:21 UTC (rev 200933)
@@ -138,13 +138,13 @@
Plan::Plan(CodeBlock* passedCodeBlock, CodeBlock* profiledDFGCodeBlock,
CompilationMode mode, unsigned osrEntryBytecodeIndex,
const Operands<JSValue>& mustHandleValues)
- : vm(*passedCodeBlock->vm())
+ : vm(passedCodeBlock->vm())
, codeBlock(passedCodeBlock)
, profiledDFGCodeBlock(profiledDFGCodeBlock)
, mode(mode)
, osrEntryBytecodeIndex(osrEntryBytecodeIndex)
, mustHandleValues(mustHandleValues)
- , compilation(codeBlock->vm()->m_perBytecodeProfiler ? adoptRef(new Profiler::Compilation(codeBlock->vm()->m_perBytecodeProfiler->ensureBytecodesFor(codeBlock), profilerCompilationKindForMode(mode))) : 0)
+ , compilation(vm->m_perBytecodeProfiler ? adoptRef(new Profiler::Compilation(vm->m_perBytecodeProfiler->ensureBytecodesFor(codeBlock), profilerCompilationKindForMode(mode))) : 0)
, inlineCallFrames(adoptRef(new InlineCallFrameSet()))
, identifiers(codeBlock)
, weakReferences(codeBlock)
@@ -160,7 +160,7 @@
{
return reportCompileTimes()
|| Options::reportTotalCompileTimes()
- || vm.m_perBytecodeProfiler;
+ || (vm && vm->m_perBytecodeProfiler);
}
bool Plan::reportCompileTimes() const
@@ -244,7 +244,7 @@
dataLog("\n");
}
- Graph dfg(vm, *this, longLivedState);
+ Graph dfg(*vm, *this, longLivedState);
if (!parse(dfg)) {
finalizer = std::make_unique<FailedFinalizer>(*this);
@@ -537,9 +537,9 @@
void Plan::reallyAdd(CommonData* commonData)
{
watchpoints.reallyAdd(codeBlock, *commonData);
- identifiers.reallyAdd(vm, commonData);
- weakReferences.reallyAdd(vm, commonData);
- transitions.reallyAdd(vm, commonData);
+ identifiers.reallyAdd(*vm, commonData);
+ weakReferences.reallyAdd(*vm, commonData);
+ transitions.reallyAdd(*vm, commonData);
}
void Plan::notifyCompiling()
@@ -561,7 +561,7 @@
CompilationResult Plan::finalizeWithoutNotifyingCallback()
{
// We will establish new references from the code block to things. So, we need a barrier.
- vm.heap.writeBarrier(codeBlock);
+ vm->heap.writeBarrier(codeBlock);
if (!isStillValid()) {
CODEBLOCK_LOG_EVENT(codeBlock, "dfgFinalize", ("invalidated"));
@@ -660,6 +660,7 @@
void Plan::cancel()
{
+ vm = nullptr;
codeBlock = nullptr;
profiledDFGCodeBlock = nullptr;
mustHandleValues.clear();
Modified: trunk/Source/_javascript_Core/dfg/DFGPlan.h (200932 => 200933)
--- trunk/Source/_javascript_Core/dfg/DFGPlan.h 2016-05-15 22:13:53 UTC (rev 200932)
+++ trunk/Source/_javascript_Core/dfg/DFGPlan.h 2016-05-15 23:08:21 UTC (rev 200933)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2013-2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2013-2016 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -78,7 +78,10 @@
bool canTierUpAndOSREnter() const { return !tierUpAndOSREnterBytecodes.isEmpty(); }
- VM& vm;
+ // Warning: pretty much all of the pointer fields in this object get nulled by cancel(). So, if
+ // you're writing code that is callable on the cancel path, be sure to null check everything!
+
+ VM* vm;
// These can be raw pointers because we visit them during every GC in checkLivenessAndVisitChildren.
CodeBlock* codeBlock;
Modified: trunk/Source/_javascript_Core/dfg/DFGSafepoint.cpp (200932 => 200933)
--- trunk/Source/_javascript_Core/dfg/DFGSafepoint.cpp 2016-05-15 22:13:53 UTC (rev 200932)
+++ trunk/Source/_javascript_Core/dfg/DFGSafepoint.cpp 2016-05-15 23:08:21 UTC (rev 200933)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2014 Apple Inc. All rights reserved.
+ * Copyright (C) 2014, 2016 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -47,7 +47,8 @@
}
Safepoint::Safepoint(Plan& plan, Result& result)
- : m_plan(plan)
+ : m_vm(plan.vm)
+ , m_plan(plan)
, m_didCallBegin(false)
, m_result(result)
{
@@ -114,11 +115,12 @@
m_plan.cancel();
m_result.m_didGetCancelled = true;
+ m_vm = nullptr;
}
-VM& Safepoint::vm() const
+VM* Safepoint::vm() const
{
- return m_plan.vm;
+ return m_vm;
}
} } // namespace JSC::DFG
Modified: trunk/Source/_javascript_Core/dfg/DFGSafepoint.h (200932 => 200933)
--- trunk/Source/_javascript_Core/dfg/DFGSafepoint.h 2016-05-15 22:13:53 UTC (rev 200932)
+++ trunk/Source/_javascript_Core/dfg/DFGSafepoint.h 2016-05-15 23:08:21 UTC (rev 200933)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2014 Apple Inc. All rights reserved.
+ * Copyright (C) 2014, 2016 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -72,9 +72,10 @@
bool isKnownToBeLiveDuringGC();
void cancel();
- VM& vm() const;
+ VM* vm() const; // May return null if we've been cancelled.
private:
+ VM* m_vm;
Plan& m_plan;
Vector<Scannable*> m_scannables;
bool m_didCallBegin;
Modified: trunk/Source/_javascript_Core/dfg/DFGWorklist.cpp (200932 => 200933)
--- trunk/Source/_javascript_Core/dfg/DFGWorklist.cpp 2016-05-15 22:13:53 UTC (rev 200932)
+++ trunk/Source/_javascript_Core/dfg/DFGWorklist.cpp 2016-05-15 23:08:21 UTC (rev 200933)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2013, 2014 Apple Inc. All rights reserved.
+ * Copyright (C) 2013-2014, 2016 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -80,7 +80,7 @@
LockHolder 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)
+ if (iter->value->vm == &vm)
return true;
}
return false;
@@ -129,7 +129,7 @@
bool allAreCompiled = true;
PlanMap::iterator end = m_plans.end();
for (PlanMap::iterator iter = m_plans.begin(); iter != end; ++iter) {
- if (&iter->value->vm != &vm)
+ if (iter->value->vm != &vm)
continue;
if (iter->value->stage != Plan::Ready) {
allAreCompiled = false;
@@ -150,7 +150,7 @@
LockHolder locker(m_lock);
for (size_t i = 0; i < m_readyPlans.size(); ++i) {
RefPtr<Plan> plan = m_readyPlans[i];
- if (&plan->vm != &vm)
+ if (plan->vm != &vm)
continue;
if (plan->stage != Plan::Ready)
continue;
@@ -212,7 +212,7 @@
LockHolder locker(m_lock);
for (PlanMap::iterator iter = m_plans.begin(); iter != m_plans.end(); ++iter) {
Plan* plan = iter->value.get();
- if (&plan->vm != &vm)
+ if (plan->vm != &vm)
continue;
plan->rememberCodeBlocks();
}
@@ -239,7 +239,7 @@
LockHolder locker(m_lock);
for (PlanMap::iterator iter = m_plans.begin(); iter != m_plans.end(); ++iter) {
Plan* plan = iter->value.get();
- if (&plan->vm != vm)
+ if (plan->vm != vm)
continue;
plan->checkLivenessAndVisitChildren(visitor);
}
@@ -251,7 +251,7 @@
for (unsigned i = m_threads.size(); i--;) {
ThreadData* data = ""
Safepoint* safepoint = data->m_safepoint;
- if (safepoint && &safepoint->vm() == vm)
+ if (safepoint && safepoint->vm() == vm)
safepoint->checkLivenessAndVisitChildren(visitor);
}
}
@@ -263,7 +263,7 @@
HashSet<CompilationKey> deadPlanKeys;
for (PlanMap::iterator iter = m_plans.begin(); iter != m_plans.end(); ++iter) {
Plan* plan = iter->value.get();
- if (&plan->vm != &vm)
+ if (plan->vm != &vm)
continue;
if (plan->isKnownToBeLiveDuringGC())
continue;
@@ -296,7 +296,7 @@
Safepoint* safepoint = data->m_safepoint;
if (!safepoint)
continue;
- if (&safepoint->vm() != &vm)
+ if (safepoint->vm() != &vm)
continue;
if (safepoint->isKnownToBeLiveDuringGC())
continue;
@@ -365,9 +365,9 @@
if (Options::verboseCompilationQueue())
dataLog(*this, ": Compiling ", plan->key(), " asynchronously\n");
- RELEASE_ASSERT(!plan->vm.heap.isCollecting());
+ RELEASE_ASSERT(!plan->vm->heap.isCollecting());
plan->compileInThread(longLivedState, data);
- RELEASE_ASSERT(plan->stage == Plan::Cancelled || !plan->vm.heap.isCollecting());
+ RELEASE_ASSERT(plan->stage == Plan::Cancelled || !plan->vm->heap.isCollecting());
{
LockHolder locker(m_lock);
@@ -377,7 +377,7 @@
}
plan->notifyCompiled();
}
- RELEASE_ASSERT(!plan->vm.heap.isCollecting());
+ RELEASE_ASSERT(!plan->vm->heap.isCollecting());
}
{
Modified: trunk/Source/_javascript_Core/ftl/FTLJITFinalizer.cpp (200932 => 200933)
--- trunk/Source/_javascript_Core/ftl/FTLJITFinalizer.cpp 2016-05-15 22:13:53 UTC (rev 200932)
+++ trunk/Source/_javascript_Core/ftl/FTLJITFinalizer.cpp 2016-05-15 23:08:21 UTC (rev 200933)
@@ -83,7 +83,7 @@
m_plan.codeBlock->setJITCode(jitCode);
if (m_plan.compilation)
- m_plan.vm.m_perBytecodeProfiler->addCompilation(m_plan.codeBlock, m_plan.compilation);
+ m_plan.vm->m_perBytecodeProfiler->addCompilation(m_plan.codeBlock, m_plan.compilation);
return true;
}
