Changeset 200933 in webkit


Ignore:
Timestamp:
May 15, 2016, 4:08:21 PM (9 years ago)
Author:
fpizlo@apple.com
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):

Location:
trunk/Source/JavaScriptCore
Files:
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/ChangeLog

    r200928 r200933  
     12016-05-15  Filip Pizlo  <fpizlo@apple.com>
     2
     3        DFG::Plan shouldn't read from its VM once it's been cancelled
     4        https://bugs.webkit.org/show_bug.cgi?id=157726
     5
     6        Reviewed by Saam Barati.
     7       
     8        Plan::vm was a reference, not a pointer, and so wasn't nulled by Plan::cancel(). So, a
     9        cancelled plan may have a dangling pointer to a VM: we could delete the VM after cancelling
     10        the plan.
     11       
     12        Prior to http://trac.webkit.org/changeset/200705, this was probably fine because nobody
     13        would read Plan::vm if the plan was cancelled. But r200705 changed that. It was a hard
     14        regression to spot because usually a cancelled plan will still refer to a valid VM.
     15       
     16        This change fixes the regression and makes it a lot easier to spot the regression in the
     17        future. Plan::vm is now a pointer and we null it in Plan::cancel(). Now if you make this
     18        mistake, you will get a crash anytime the Plan is cancelled, not just anytime the plan is
     19        cancelled and the VM gets deleted. Also, it's now very clear what to do when you want to
     20        use Plan::vm on the cancel path: you can null-check vm; if it's null, assume the worst.
     21       
     22        Because we null the VM of a cancelled plan, we cannot have Safepoint::vm() return the
     23        plan's VM anymore. That's because when we cancel a plan that is at a safepoint, we use the
     24        safepoint's VM to determine whether this is one of our safepoints *after* the plan is
     25        already cancelled. So, Safepoint now has its own copy of m_vm, and that copy gets nulled
     26        when the Safepoint is cancelled. The Safepoint's m_vm will be nulled moments after Plan's
     27        vm gets nulled (see Worklist::removeDeadPlans(), which has a cancel path for Plans in one
     28        loop and a cancel path for Safepoints in the loop after it).
     29
     30        * dfg/DFGJITFinalizer.cpp:
     31        (JSC::DFG::JITFinalizer::finalizeCommon):
     32        * dfg/DFGPlan.cpp:
     33        (JSC::DFG::Plan::Plan):
     34        (JSC::DFG::Plan::computeCompileTimes):
     35        (JSC::DFG::Plan::reportCompileTimes):
     36        (JSC::DFG::Plan::compileInThreadImpl):
     37        (JSC::DFG::Plan::reallyAdd):
     38        (JSC::DFG::Plan::notifyCompiling):
     39        (JSC::DFG::Plan::finalizeWithoutNotifyingCallback):
     40        (JSC::DFG::Plan::cancel):
     41        * dfg/DFGPlan.h:
     42        (JSC::DFG::Plan::canTierUpAndOSREnter):
     43        * dfg/DFGSafepoint.cpp:
     44        (JSC::DFG::Safepoint::cancel):
     45        (JSC::DFG::Safepoint::vm):
     46        * dfg/DFGSafepoint.h:
     47        * dfg/DFGWorklist.cpp:
     48        (JSC::DFG::Worklist::isActiveForVM):
     49        (JSC::DFG::Worklist::waitUntilAllPlansForVMAreReady):
     50        (JSC::DFG::Worklist::removeAllReadyPlansForVM):
     51        (JSC::DFG::Worklist::rememberCodeBlocks):
     52        (JSC::DFG::Worklist::visitWeakReferences):
     53        (JSC::DFG::Worklist::removeDeadPlans):
     54        (JSC::DFG::Worklist::runThread):
     55        * ftl/FTLJITFinalizer.cpp:
     56        (JSC::FTL::JITFinalizer::finalizeFunction):
     57
    1582016-05-15  Yusuke Suzuki  <utatane.tea@gmail.com>
    259

  • trunk/Source/JavaScriptCore/dfg/DFGJITFinalizer.cpp

    r200658 r200933  
    9292   
    9393    if (m_plan.compilation)
    94         m_plan.vm.m_perBytecodeProfiler->addCompilation(m_plan.codeBlock, m_plan.compilation);
     94        m_plan.vm->m_perBytecodeProfiler->addCompilation(m_plan.codeBlock, m_plan.compilation);
    9595   
    9696    if (!m_plan.willTryToTierUp)

  • trunk/Source/JavaScriptCore/dfg/DFGPlan.cpp

    r200705 r200933  
    139139    CompilationMode mode, unsigned osrEntryBytecodeIndex,
    140140    const Operands<JSValue>& mustHandleValues)
    141     : vm(*passedCodeBlock->vm())
     141    : vm(passedCodeBlock->vm())
    142142    , codeBlock(passedCodeBlock)
    143143    , profiledDFGCodeBlock(profiledDFGCodeBlock)
    … …  
    145145    , osrEntryBytecodeIndex(osrEntryBytecodeIndex)
    146146    , mustHandleValues(mustHandleValues)
    147     , compilation(codeBlock->vm()->m_perBytecodeProfiler ? adoptRef(new Profiler::Compilation(codeBlock->vm()->m_perBytecodeProfiler->ensureBytecodesFor(codeBlock), profilerCompilationKindForMode(mode))) : 0)
     147    , compilation(vm->m_perBytecodeProfiler ? adoptRef(new Profiler::Compilation(vm->m_perBytecodeProfiler->ensureBytecodesFor(codeBlock), profilerCompilationKindForMode(mode))) : 0)
    148148    , inlineCallFrames(adoptRef(new InlineCallFrameSet()))
    149149    , identifiers(codeBlock)
    … …  
    161161    return reportCompileTimes()
    162162        || Options::reportTotalCompileTimes()
    163         || vm.m_perBytecodeProfiler;
     163        || (vm && vm->m_perBytecodeProfiler);
    164164}
    165165
    … …  
    245245    }
    246246   
    247     Graph dfg(vm, *this, longLivedState);
     247    Graph dfg(*vm, *this, longLivedState);
    248248   
    249249    if (!parse(dfg)) {
    … …  
    538538{
    539539    watchpoints.reallyAdd(codeBlock, *commonData);
    540     identifiers.reallyAdd(vm, commonData);
    541     weakReferences.reallyAdd(vm, commonData);
    542     transitions.reallyAdd(vm, commonData);
     540    identifiers.reallyAdd(*vm, commonData);
     541    weakReferences.reallyAdd(*vm, commonData);
     542    transitions.reallyAdd(*vm, commonData);
    543543}
    544544
    … …  
    562562{
    563563    // We will establish new references from the code block to things. So, we need a barrier.
    564     vm.heap.writeBarrier(codeBlock);
     564    vm->heap.writeBarrier(codeBlock);
    565565   
    566566    if (!isStillValid()) {
    … …  
    661661void Plan::cancel()
    662662{
     663    vm = nullptr;
    663664    codeBlock = nullptr;
    664665    profiledDFGCodeBlock = nullptr;

  • trunk/Source/JavaScriptCore/dfg/DFGPlan.h

    r200531 r200933  
    11/*
    2  * Copyright (C) 2013-2015 Apple Inc. All rights reserved.
     2 * Copyright (C) 2013-2016 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    7979    bool canTierUpAndOSREnter() const { return !tierUpAndOSREnterBytecodes.isEmpty(); }
    8080   
    81     VM& vm;
     81    // Warning: pretty much all of the pointer fields in this object get nulled by cancel(). So, if
     82    // you're writing code that is callable on the cancel path, be sure to null check everything!
     83   
     84    VM* vm;
    8285
    8386    // These can be raw pointers because we visit them during every GC in checkLivenessAndVisitChildren.

  • trunk/Source/JavaScriptCore/dfg/DFGSafepoint.cpp

    r169595 r200933  
    11/*
    2  * Copyright (C) 2014 Apple Inc. All rights reserved.
     2 * Copyright (C) 2014, 2016 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    4848
    4949Safepoint::Safepoint(Plan& plan, Result& result)
    50     : m_plan(plan)
     50    : m_vm(plan.vm)
     51    , m_plan(plan)
    5152    , m_didCallBegin(false)
    5253    , m_result(result)
    … …  
    115116    m_plan.cancel();
    116117    m_result.m_didGetCancelled = true;
     118    m_vm = nullptr;
    117119}
    118120
    119 VM& Safepoint::vm() const
     121VM* Safepoint::vm() const
    120122{
    121     return m_plan.vm;
     123    return m_vm;
    122124}
    123125

  • trunk/Source/JavaScriptCore/dfg/DFGSafepoint.h

    r167897 r200933  
    11/*
    2  * Copyright (C) 2014 Apple Inc. All rights reserved.
     2 * Copyright (C) 2014, 2016 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    7373    void cancel();
    7474   
    75     VM& vm() const;
     75    VM* vm() const; // May return null if we've been cancelled.
    7676
    7777private:
     78    VM* m_vm;
    7879    Plan& m_plan;
    7980    Vector<Scannable*> m_scannables;

  • trunk/Source/JavaScriptCore/dfg/DFGWorklist.cpp

    r198477 r200933  
    11/*
    2  * Copyright (C) 2013, 2014 Apple Inc. All rights reserved.
     2 * Copyright (C) 2013-2014, 2016 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    8181    PlanMap::const_iterator end = m_plans.end();
    8282    for (PlanMap::const_iterator iter = m_plans.begin(); iter != end; ++iter) {
    83         if (&iter->value->vm == &vm)
     83        if (iter->value->vm == &vm)
    8484            return true;
    8585    }
    … …  
    130130        PlanMap::iterator end = m_plans.end();
    131131        for (PlanMap::iterator iter = m_plans.begin(); iter != end; ++iter) {
    132             if (&iter->value->vm != &vm)
     132            if (iter->value->vm != &vm)
    133133                continue;
    134134            if (iter->value->stage != Plan::Ready) {
    … …  
    151151    for (size_t i = 0; i < m_readyPlans.size(); ++i) {
    152152        RefPtr<Plan> plan = m_readyPlans[i];
    153         if (&plan->vm != &vm)
     153        if (plan->vm != &vm)
    154154            continue;
    155155        if (plan->stage != Plan::Ready)
    … …  
    213213    for (PlanMap::iterator iter = m_plans.begin(); iter != m_plans.end(); ++iter) {
    214214        Plan* plan = iter->value.get();
    215         if (&plan->vm != &vm)
     215        if (plan->vm != &vm)
    216216            continue;
    217217        plan->rememberCodeBlocks();
    … …  
    240240        for (PlanMap::iterator iter = m_plans.begin(); iter != m_plans.end(); ++iter) {
    241241            Plan* plan = iter->value.get();
    242             if (&plan->vm != vm)
     242            if (plan->vm != vm)
    243243                continue;
    244244            plan->checkLivenessAndVisitChildren(visitor);
    … …  
    252252        ThreadData* data = m_threads[i].get();
    253253        Safepoint* safepoint = data->m_safepoint;
    254         if (safepoint && &safepoint->vm() == vm)
     254        if (safepoint && safepoint->vm() == vm)
    255255            safepoint->checkLivenessAndVisitChildren(visitor);
    256256    }
    … …  
    264264        for (PlanMap::iterator iter = m_plans.begin(); iter != m_plans.end(); ++iter) {
    265265            Plan* plan = iter->value.get();
    266             if (&plan->vm != &vm)
     266            if (plan->vm != &vm)
    267267                continue;
    268268            if (plan->isKnownToBeLiveDuringGC())
    … …  
    297297        if (!safepoint)
    298298            continue;
    299         if (&safepoint->vm() != &vm)
     299        if (safepoint->vm() != &vm)
    300300            continue;
    301301        if (safepoint->isKnownToBeLiveDuringGC())
    … …  
    366366                dataLog(*this, ": Compiling ", plan->key(), " asynchronously\n");
    367367       
    368             RELEASE_ASSERT(!plan->vm.heap.isCollecting());
     368            RELEASE_ASSERT(!plan->vm->heap.isCollecting());
    369369            plan->compileInThread(longLivedState, data);
    370             RELEASE_ASSERT(plan->stage == Plan::Cancelled || !plan->vm.heap.isCollecting());
     370            RELEASE_ASSERT(plan->stage == Plan::Cancelled || !plan->vm->heap.isCollecting());
    371371           
    372372            {
    … …  
    378378                plan->notifyCompiled();
    379379            }
    380             RELEASE_ASSERT(!plan->vm.heap.isCollecting());
     380            RELEASE_ASSERT(!plan->vm->heap.isCollecting());
    381381        }
    382382

  • trunk/Source/JavaScriptCore/ftl/FTLJITFinalizer.cpp

    r200658 r200933  
    8484
    8585    if (m_plan.compilation)
    86         m_plan.vm.m_perBytecodeProfiler->addCompilation(m_plan.codeBlock, m_plan.compilation);
     86        m_plan.vm->m_perBytecodeProfiler->addCompilation(m_plan.codeBlock, m_plan.compilation);
    8787   
    8888    return true;
Note: See TracChangeset for help on using the changeset viewer.