DFG::Plan shouldn't read from its VM once it's been cancelled
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 15 May 2016 23:08:21 +0000 (23:08 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 15 May 2016 23:08:21 +0000 (23:08 +0000)
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):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGJITFinalizer.cpp
Source/JavaScriptCore/dfg/DFGPlan.cpp
Source/JavaScriptCore/dfg/DFGPlan.h
Source/JavaScriptCore/dfg/DFGSafepoint.cpp
Source/JavaScriptCore/dfg/DFGSafepoint.h
Source/JavaScriptCore/dfg/DFGWorklist.cpp
Source/JavaScriptCore/ftl/FTLJITFinalizer.cpp

index 5beaab2..0079cb8 100644 (file)
@@ -1,3 +1,60 @@
+2016-05-15  Filip Pizlo  <fpizlo@apple.com>
+
+        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  <utatane.tea@gmail.com>
 
         Modernize Intl constructors; using InternalFunction::createSubclassStructure
index d673c94..5391158 100644 (file)
@@ -91,7 +91,7 @@ void JITFinalizer::finalizeCommon()
 #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;
index d3e095e..355cb4e 100644 (file)
@@ -138,13 +138,13 @@ Profiler::CompilationKind profilerCompilationKindForMode(CompilationMode mode)
 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 @@ bool Plan::computeCompileTimes() const
 {
     return reportCompileTimes()
         || Options::reportTotalCompileTimes()
-        || vm.m_perBytecodeProfiler;
+        || (vm && vm->m_perBytecodeProfiler);
 }
 
 bool Plan::reportCompileTimes() const
@@ -244,7 +244,7 @@ Plan::CompilationPath Plan::compileInThreadImpl(LongLivedState& longLivedState)
         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 @@ bool Plan::isStillValid()
 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 @@ void Plan::notifyReady()
 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 @@ bool Plan::isKnownToBeLiveDuringGC()
 
 void Plan::cancel()
 {
+    vm = nullptr;
     codeBlock = nullptr;
     profiledDFGCodeBlock = nullptr;
     mustHandleValues.clear();
index 0cbfa0a..1a9ef7e 100644 (file)
@@ -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 @@ struct Plan : public ThreadSafeRefCounted<Plan> {
 
     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;
index 11ba5ad..550212e 100644 (file)
@@ -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 @@ bool Safepoint::Result::didGetCancelled()
 }
 
 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 @@ void Safepoint::cancel()
     
     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
index 96f4b8e..34cfaa6 100644 (file)
@@ -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 @@ public:
     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;
index 9d7e68a..4f566b8 100644 (file)
@@ -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 @@ bool Worklist::isActiveForVM(VM& vm) const
     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 @@ void Worklist::waitUntilAllPlansForVMAreReady(VM& vm)
         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 @@ void Worklist::removeAllReadyPlansForVM(VM& vm, Vector<RefPtr<Plan>, 8>& myReady
     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 @@ void Worklist::rememberCodeBlocks(VM& vm)
     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 @@ void Worklist::visitWeakReferences(SlotVisitor& visitor)
         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 @@ void Worklist::visitWeakReferences(SlotVisitor& visitor)
     for (unsigned i = m_threads.size(); i--;) {
         ThreadData* data = m_threads[i].get();
         Safepoint* safepoint = data->m_safepoint;
-        if (safepoint && &safepoint->vm() == vm)
+        if (safepoint && safepoint->vm() == vm)
             safepoint->checkLivenessAndVisitChildren(visitor);
     }
 }
@@ -263,7 +263,7 @@ void Worklist::removeDeadPlans(VM& vm)
         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 @@ void Worklist::removeDeadPlans(VM& vm)
         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 @@ void Worklist::runThread(ThreadData* data)
             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 @@ void Worklist::runThread(ThreadData* data)
                 }
                 plan->notifyCompiled();
             }
-            RELEASE_ASSERT(!plan->vm.heap.isCollecting());
+            RELEASE_ASSERT(!plan->vm->heap.isCollecting());
         }
 
         {
index e66c89f..00a88f0 100644 (file)
@@ -83,7 +83,7 @@ bool JITFinalizer::finalizeFunction()
     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;
 }