The compiler thread should not adjust Identifier refCounts.
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 7 Dec 2019 06:07:56 +0000 (06:07 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 7 Dec 2019 06:07:56 +0000 (06:07 +0000)
https://bugs.webkit.org/show_bug.cgi?id=204919
<rdar://problem/57426861>

Reviewed by Saam Barati.

JSTests:

* stress/compiler-thread-should-not-ref-identifiers.js: Added.

Source/JavaScriptCore:

1. Previously, in the compiler thread, we would get a Symbol uid via
   Symbol::privateName().uid().  Symbol::privateName() returns a copy of its
   PrivateName, which in turn results in ref'ing the underlying SymbolImpl.
   This results in a race between the mutator and compiler threads to adjust the
   SymbolImpl's refCount, which may result in corruption.

   This patch fixes this by adding Symbol::uid() which return the underlying
   SymbolImpl without ref'ing it.

2. Previously, in the compiler thread, we also create Box<Identifier> via its
   copy constructor.  The original Box<Identifier> is instantiated in the mutator.
   The Box<Identifier> refs its internal Data, which is ThreadSafeRefCounted and
   shared by all Box<Identifier> for the same underlying Identifier.
   This ensures that the compiler thread does not ref the underlying Identifier.

   However, when the Box<Identifier> is destructed, it will also check if it holds
   the last ref to its internal Data.  If so, it will destruct its Data, and the
   Identifier that it embeds.  This results in the compiler thread trying to deref
   the StringImpl referenced by the Identifier in a race against the mutator.

   This patch fixes this by ensuring that for any Box<Identifier> instance used
   by the compiler thread, we will register another instance in the DFG::Plan
   m_identifiersKeptAliveForCleanUp list, and let the mutator destruct that
   Box<Identifier> later in the mutator.  This ensures that the compiler thread
   will never see the last reference to a Box<Identifier>'s internal Data and
   avoid the race.

3. This patch also fixes the DFG::Worklist code to ensure that a DFG::Plan is
   always destructed in the mutator, even if the Plan was cancelled.

   This, in turn, enables us to assert that the Plan is never destructed in the
   compiler thread.

* bytecode/GetByStatus.cpp:
(JSC::GetByStatus::computeFor):
(JSC::GetByStatus::computeForStubInfoWithoutExitSiteFeedback):
* bytecode/GetByStatus.h:
* debugger/Debugger.cpp:
(JSC::Debugger::detach):
* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::parseGetById):
(JSC::DFG::ByteCodeParser::parseBlock):
* dfg/DFGConstantFoldingPhase.cpp:
(JSC::DFG::ConstantFoldingPhase::foldConstants):
* dfg/DFGPlan.cpp:
(JSC::DFG::Plan::~Plan):
(JSC::DFG::Plan::computeCompileTimes const):
(JSC::DFG::Plan::cancel):
* dfg/DFGPlan.h:
(JSC::DFG::Plan::unnukedVM const):
(JSC::DFG::Plan::keepAliveIdentifier):
(JSC::DFG::Plan::nuke):
(JSC::DFG::Plan::unnuke):
* dfg/DFGSafepoint.cpp:
(JSC::DFG::Safepoint::cancel):
* dfg/DFGWorklist.cpp:
(JSC::DFG::Worklist::deleteCancelledPlansForVM):
(JSC::DFG::Worklist::removeAllReadyPlansForVM):
(JSC::DFG::Worklist::removeDeadPlans):
(JSC::DFG::Worklist::removeNonCompilingPlansForVM):
* dfg/DFGWorklist.h:
* runtime/Symbol.h:

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

15 files changed:
JSTests/ChangeLog
JSTests/stress/compiler-thread-should-not-ref-identifiers.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/GetByStatus.cpp
Source/JavaScriptCore/bytecode/GetByStatus.h
Source/JavaScriptCore/debugger/Debugger.cpp
Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp
Source/JavaScriptCore/dfg/DFGPlan.cpp
Source/JavaScriptCore/dfg/DFGPlan.h
Source/JavaScriptCore/dfg/DFGSafepoint.cpp
Source/JavaScriptCore/dfg/DFGWorklist.cpp
Source/JavaScriptCore/dfg/DFGWorklist.h
Source/JavaScriptCore/runtime/Symbol.h

index d2b8174..0dbe32c 100644 (file)
@@ -1,3 +1,13 @@
+2019-12-06  Mark Lam  <mark.lam@apple.com>
+
+        The compiler thread should not adjust Identifier refCounts.
+        https://bugs.webkit.org/show_bug.cgi?id=204919
+        <rdar://problem/57426861>
+
+        Reviewed by Saam Barati.
+
+        * stress/compiler-thread-should-not-ref-identifiers.js: Added.
+
 2019-12-04  Yusuke Suzuki  <ysuzuki@apple.com>
 
         [JSC] AI should convert IsCellWithType to constant when Structure set is finite
diff --git a/JSTests/stress/compiler-thread-should-not-ref-identifiers.js b/JSTests/stress/compiler-thread-should-not-ref-identifiers.js
new file mode 100644 (file)
index 0000000..6ce2b44
--- /dev/null
@@ -0,0 +1,15 @@
+//@ slow!
+//@ runDefault
+
+for (let j = 0; j < 500; j++) {
+    runString(`
+
+let a = Symbol('a');
+let o = { [a]: 1 };
+
+for (let i = 0; i < 10000; ++i) {
+    Object.getOwnPropertySymbols(o);
+    o[a];
+}
+`);
+}
index 31cc0bb..9a0ba0e 100644 (file)
@@ -1,3 +1,76 @@
+2019-12-06  Mark Lam  <mark.lam@apple.com>
+
+        The compiler thread should not adjust Identifier refCounts.
+        https://bugs.webkit.org/show_bug.cgi?id=204919
+        <rdar://problem/57426861>
+
+        Reviewed by Saam Barati.
+
+        1. Previously, in the compiler thread, we would get a Symbol uid via
+           Symbol::privateName().uid().  Symbol::privateName() returns a copy of its
+           PrivateName, which in turn results in ref'ing the underlying SymbolImpl.
+           This results in a race between the mutator and compiler threads to adjust the
+           SymbolImpl's refCount, which may result in corruption.
+
+           This patch fixes this by adding Symbol::uid() which return the underlying
+           SymbolImpl without ref'ing it.
+
+        2. Previously, in the compiler thread, we also create Box<Identifier> via its
+           copy constructor.  The original Box<Identifier> is instantiated in the mutator.
+           The Box<Identifier> refs its internal Data, which is ThreadSafeRefCounted and
+           shared by all Box<Identifier> for the same underlying Identifier.
+           This ensures that the compiler thread does not ref the underlying Identifier.
+
+           However, when the Box<Identifier> is destructed, it will also check if it holds
+           the last ref to its internal Data.  If so, it will destruct its Data, and the
+           Identifier that it embeds.  This results in the compiler thread trying to deref
+           the StringImpl referenced by the Identifier in a race against the mutator.
+
+           This patch fixes this by ensuring that for any Box<Identifier> instance used
+           by the compiler thread, we will register another instance in the DFG::Plan
+           m_identifiersKeptAliveForCleanUp list, and let the mutator destruct that
+           Box<Identifier> later in the mutator.  This ensures that the compiler thread
+           will never see the last reference to a Box<Identifier>'s internal Data and
+           avoid the race.
+
+        3. This patch also fixes the DFG::Worklist code to ensure that a DFG::Plan is
+           always destructed in the mutator, even if the Plan was cancelled.
+
+           This, in turn, enables us to assert that the Plan is never destructed in the
+           compiler thread.
+
+        * bytecode/GetByStatus.cpp:
+        (JSC::GetByStatus::computeFor):
+        (JSC::GetByStatus::computeForStubInfoWithoutExitSiteFeedback):
+        * bytecode/GetByStatus.h:
+        * debugger/Debugger.cpp:
+        (JSC::Debugger::detach):
+        * dfg/DFGAbstractInterpreterInlines.h:
+        (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::parseGetById):
+        (JSC::DFG::ByteCodeParser::parseBlock):
+        * dfg/DFGConstantFoldingPhase.cpp:
+        (JSC::DFG::ConstantFoldingPhase::foldConstants):
+        * dfg/DFGPlan.cpp:
+        (JSC::DFG::Plan::~Plan):
+        (JSC::DFG::Plan::computeCompileTimes const):
+        (JSC::DFG::Plan::cancel):
+        * dfg/DFGPlan.h:
+        (JSC::DFG::Plan::unnukedVM const):
+        (JSC::DFG::Plan::keepAliveIdentifier):
+        (JSC::DFG::Plan::nuke):
+        (JSC::DFG::Plan::unnuke):
+        * dfg/DFGSafepoint.cpp:
+        (JSC::DFG::Safepoint::cancel):
+        * dfg/DFGWorklist.cpp:
+        (JSC::DFG::Worklist::deleteCancelledPlansForVM):
+        (JSC::DFG::Worklist::removeAllReadyPlansForVM):
+        (JSC::DFG::Worklist::removeDeadPlans):
+        (JSC::DFG::Worklist::removeNonCompilingPlansForVM):
+        * dfg/DFGWorklist.h:
+        * runtime/Symbol.h:
+
 2019-12-06  Yusuke Suzuki  <ysuzuki@apple.com>
 
         [JSC] Put JSModuleNamespaceObject in IsoSubspace
index 2005576..0c9b192 100644 (file)
@@ -113,7 +113,7 @@ GetByStatus GetByStatus::computeFromLLInt(CodeBlock* profiledBlock, BytecodeInde
     return result;
 }
 
-GetByStatus GetByStatus::computeFor(CodeBlock* profiledBlock, ICStatusMap& map, BytecodeIndex bytecodeIndex, ExitFlag didExit, CallLinkStatus::ExitSiteData callExitSiteData, TrackIdentifiers trackIdentifiers)
+GetByStatus GetByStatus::computeFor(CodeBlock* profiledBlock, ICStatusMap& map, BytecodeIndex bytecodeIndex, ExitFlag didExit, CallLinkStatus::ExitSiteData callExitSiteData, TrackIdentifiers trackIdentifiers, IdentifierKeepAlive& keepAlive)
 {
     ConcurrentJSLocker locker(profiledBlock->m_lock);
 
@@ -121,7 +121,7 @@ GetByStatus GetByStatus::computeFor(CodeBlock* profiledBlock, ICStatusMap& map,
 
 #if ENABLE(DFG_JIT)
     result = computeForStubInfoWithoutExitSiteFeedback(
-        locker, profiledBlock, map.get(CodeOrigin(bytecodeIndex)).stubInfo, callExitSiteData, trackIdentifiers);
+        locker, profiledBlock, map.get(CodeOrigin(bytecodeIndex)).stubInfo, callExitSiteData, trackIdentifiers, keepAlive);
     
     if (didExit)
         return result.slowVersion();
@@ -167,7 +167,7 @@ GetByStatus::GetByStatus(const ModuleNamespaceAccessCase& accessCase)
 }
 
 GetByStatus GetByStatus::computeForStubInfoWithoutExitSiteFeedback(
-    const ConcurrentJSLocker& locker, CodeBlock* profiledBlock, StructureStubInfo* stubInfo, CallLinkStatus::ExitSiteData callExitSiteData, TrackIdentifiers trackIdentifiers)
+    const ConcurrentJSLocker& locker, CodeBlock* profiledBlock, StructureStubInfo* stubInfo, CallLinkStatus::ExitSiteData callExitSiteData, TrackIdentifiers trackIdentifiers, IdentifierKeepAlive& keepAlive)
 {
     StubInfoSummary summary = StructureStubInfo::summary(stubInfo);
     if (!isInlineable(summary))
@@ -186,6 +186,7 @@ GetByStatus GetByStatus::computeForStubInfoWithoutExitSiteFeedback(
         if (structure->takesSlowPathInDFGForImpureProperty())
             return GetByStatus(JSC::slowVersion(summary), *stubInfo);
         Box<Identifier> identifier = stubInfo->getByIdSelfIdentifier();
+        keepAlive(identifier);
         UniquedStringImpl* uid = identifier->impl();
         RELEASE_ASSERT(uid);
         if (trackIdentifiers == TrackIdentifiers::No)
@@ -210,6 +211,7 @@ GetByStatus GetByStatus::computeForStubInfoWithoutExitSiteFeedback(
             const AccessCase& access = list->at(0);
             switch (access.type()) {
             case AccessCase::ModuleNamespaceLoad:
+                keepAlive(access.identifier());
                 return GetByStatus(access.as<ModuleNamespaceAccessCase>());
             default:
                 break;
@@ -293,8 +295,12 @@ GetByStatus GetByStatus::computeForStubInfoWithoutExitSiteFeedback(
                 } }
 
                 ASSERT((AccessCase::Miss == access.type() || access.isCustom()) == (access.offset() == invalidOffset));
-                GetByIdVariant variant(
-                    trackIdentifiers == TrackIdentifiers::Yes ? access.identifier() : Box<Identifier>(nullptr), StructureSet(structure), complexGetStatus.offset(),
+                Box<Identifier> identifier;
+                if (trackIdentifiers == TrackIdentifiers::Yes) {
+                    identifier = access.identifier();
+                    keepAlive(identifier);
+                }
+                GetByIdVariant variant(identifier, StructureSet(structure), complexGetStatus.offset(),
                     complexGetStatus.conditionSet(), WTFMove(callLinkStatus),
                     intrinsicFunction,
                     customAccessorGetter,
@@ -329,7 +335,7 @@ GetByStatus GetByStatus::computeForStubInfoWithoutExitSiteFeedback(
 
 GetByStatus GetByStatus::computeFor(
     CodeBlock* profiledBlock, ICStatusMap& baselineMap,
-    ICStatusContextStack& icContextStack, CodeOrigin codeOrigin, TrackIdentifiers trackIdentifiers)
+    ICStatusContextStack& icContextStack, CodeOrigin codeOrigin, TrackIdentifiers trackIdentifiers, IdentifierKeepAlive& keepAlive)
 {
     BytecodeIndex bytecodeIndex = codeOrigin.bytecodeIndex();
     CallLinkStatus::ExitSiteData callExitSiteData = CallLinkStatus::computeExitSiteData(profiledBlock, bytecodeIndex);
@@ -344,7 +350,7 @@ GetByStatus GetByStatus::computeFor(
                 // inlined and not-inlined.
                 GetByStatus baselineResult = computeFor(
                     profiledBlock, baselineMap, bytecodeIndex, didExit,
-                    callExitSiteData, trackIdentifiers);
+                    callExitSiteData, trackIdentifiers, keepAlive);
                 baselineResult.merge(result);
                 return baselineResult;
             }
@@ -358,7 +364,7 @@ GetByStatus GetByStatus::computeFor(
             {
                 ConcurrentJSLocker locker(context->optimizedCodeBlock->m_lock);
                 result = computeForStubInfoWithoutExitSiteFeedback(
-                    locker, context->optimizedCodeBlock, status.stubInfo, callExitSiteData, trackIdentifiers);
+                    locker, context->optimizedCodeBlock, status.stubInfo, callExitSiteData, trackIdentifiers, keepAlive);
             }
             if (result.isSet())
                 return bless(result);
@@ -368,7 +374,7 @@ GetByStatus GetByStatus::computeFor(
             return bless(*status.getStatus);
     }
     
-    return computeFor(profiledBlock, baselineMap, bytecodeIndex, didExit, callExitSiteData, trackIdentifiers);
+    return computeFor(profiledBlock, baselineMap, bytecodeIndex, didExit, callExitSiteData, trackIdentifiers, keepAlive);
 }
 
 GetByStatus GetByStatus::computeFor(const StructureSet& set, UniquedStringImpl* uid)
index e133eaa..abe5f08 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2012-2018 Apple Inc. All rights reserved.
+ * Copyright (C) 2012-2019 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -90,8 +90,14 @@ public:
         , m_wasSeenInJIT(wasSeenInJIT)
     {
     }
-    
-    static GetByStatus computeFor(CodeBlock* baselineBlock, ICStatusMap& baselineMap, ICStatusContextStack& dfgContextStack, CodeOrigin, TrackIdentifiers);
+
+    // If we use a Box<Identifier> in the compiler thread, we need to keep the
+    // Identifier alive and only deref it in the mutator thread later. This
+    // ensures that the compiler thread doesn't race against the mutator in
+    // adjusting the Identifier's refCount.
+    using IdentifierKeepAlive = std::function<void(Box<Identifier>)>;
+
+    static GetByStatus computeFor(CodeBlock* baselineBlock, ICStatusMap& baselineMap, ICStatusContextStack& dfgContextStack, CodeOrigin, TrackIdentifiers, IdentifierKeepAlive&);
     static GetByStatus computeFor(const StructureSet&, UniquedStringImpl*);
 
     State state() const { return m_state; }
@@ -126,7 +132,7 @@ public:
     bool finalize(VM&); // Return true if this gets to live.
 
     bool appendVariant(const GetByIdVariant&);
-    
+
     void dump(PrintStream&) const;
 
     Box<Identifier> singleIdentifier() const;
@@ -137,10 +143,10 @@ private:
 #if ENABLE(JIT)
     GetByStatus(const ModuleNamespaceAccessCase&);
     static GetByStatus computeForStubInfoWithoutExitSiteFeedback(
-        const ConcurrentJSLocker&, CodeBlock* profiledBlock, StructureStubInfo*, CallLinkStatus::ExitSiteData, TrackIdentifiers);
+        const ConcurrentJSLocker&, CodeBlock* profiledBlock, StructureStubInfo*, CallLinkStatus::ExitSiteData, TrackIdentifiers, IdentifierKeepAlive&);
 #endif
     static GetByStatus computeFromLLInt(CodeBlock*, BytecodeIndex, TrackIdentifiers);
-    static GetByStatus computeFor(CodeBlock*, ICStatusMap&, BytecodeIndex, ExitFlag, CallLinkStatus::ExitSiteData, TrackIdentifiers);
+    static GetByStatus computeFor(CodeBlock*, ICStatusMap&, BytecodeIndex, ExitFlag, CallLinkStatus::ExitSiteData, TrackIdentifiers, IdentifierKeepAlive&);
 
     struct ModuleNamespaceData {
         JSModuleNamespaceObject* m_moduleNamespaceObject { nullptr };
index dda6d05..409075d 100644 (file)
@@ -174,6 +174,8 @@ void Debugger::detach(JSGlobalObject* globalObject, ReasonForDetach reason)
     // since there's no point in staying paused once a window closes.
     // We know there is an entry scope, otherwise, m_currentCallFrame would be null.
     VM& vm = globalObject->vm();
+    JSLockHolder locker(vm);
+
     if (m_isPaused && m_currentCallFrame && vm.entryScope->globalObject() == globalObject) {
         m_currentCallFrame = nullptr;
         m_pauseOnCallFrame = nullptr;
index 220b0fd..5d7e696 100644 (file)
@@ -3700,7 +3700,7 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
                     break;
                 }
             } else if (childConstant.isSymbol()) {
-                if (&jsCast<Symbol*>(childConstant)->privateName().uid() == uid) {
+                if (&jsCast<Symbol*>(childConstant)->uid() == uid) {
                     m_state.setShouldTryConstantFolding(true);
                     break;
                 }
index dcc03ae..a9243ef 100644 (file)
@@ -4734,11 +4734,14 @@ void ByteCodeParser::parseGetById(const Instruction* currentInstruction)
         m_graph.m_shouldSkipIC.add(node);
         return;
     }
-    
+
+    GetByStatus::IdentifierKeepAlive keepAlive = [&] (Box<Identifier> identifier) {
+        m_graph.m_plan.keepAliveIdentifier(WTFMove(identifier));
+    };
     GetByStatus getByStatus = GetByStatus::computeFor(
         m_inlineStackTop->m_profiledBlock,
         m_inlineStackTop->m_baselineMap, m_icContextStack,
-        currentCodeOrigin(), GetByStatus::TrackIdentifiers::No);
+        currentCodeOrigin(), GetByStatus::TrackIdentifiers::No, keepAlive);
 
     handleGetById(
         bytecode.m_dst, prediction, base, identifierNumber, getByStatus, type, opcodeLength);
@@ -5639,7 +5642,11 @@ void ByteCodeParser::parseBlock(unsigned limit)
             Node* base = get(bytecode.m_base);
             Node* property = get(bytecode.m_property);
             bool shouldCompileAsGetById = false;
-            GetByStatus getByStatus = GetByStatus::computeFor(m_inlineStackTop->m_profiledBlock, m_inlineStackTop->m_baselineMap, m_icContextStack, currentCodeOrigin(), GetByStatus::TrackIdentifiers::Yes);
+            GetByStatus::IdentifierKeepAlive keepAlive = [&] (Box<Identifier> identifier) {
+                m_graph.m_plan.keepAliveIdentifier(WTFMove(identifier));
+            };
+            GetByStatus getByStatus = GetByStatus::computeFor(m_inlineStackTop->m_profiledBlock, m_inlineStackTop->m_baselineMap, m_icContextStack, currentCodeOrigin(), GetByStatus::TrackIdentifiers::Yes, keepAlive);
+
             unsigned identifierNumber = 0;
 
             if (!m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadIdent)
index e881378..3c0b20c 100644 (file)
@@ -328,7 +328,7 @@ private:
                         }
                     } else if (childConstant.isSymbol()) {
                         Symbol* symbol = jsCast<Symbol*>(childConstant);
-                        constantUid = &symbol->privateName().uid();
+                        constantUid = &symbol->uid();
                     }
                 }
 
index d41a3d4..3aece84 100644 (file)
@@ -79,6 +79,7 @@
 #include "ProfilerDatabase.h"
 #include "TrackedReferences.h"
 #include "VMInlines.h"
+#include <wtf/Threading.h>
 
 #if ENABLE(FTL_JIT)
 #include "FTLCapabilities.h"
@@ -154,13 +155,14 @@ Plan::Plan(CodeBlock* passedCodeBlock, CodeBlock* profiledDFGCodeBlock,
 
 Plan::~Plan()
 {
+    RELEASE_ASSERT(unnukedVM()->currentThreadIsHoldingAPILock());
 }
 
 bool Plan::computeCompileTimes() const
 {
     return reportCompileTimes()
         || Options::reportTotalCompileTimes()
-        || (m_vm && m_vm->m_perBytecodeProfiler);
+        || unnukedVM()->m_perBytecodeProfiler;
 }
 
 bool Plan::reportCompileTimes() const
@@ -693,7 +695,30 @@ bool Plan::isKnownToBeLiveDuringGC()
 
 void Plan::cancel()
 {
-    m_vm = nullptr;
+    RELEASE_ASSERT(m_stage != Cancelled);
+
+    // When we cancel a plan, there's a chance that the compiler thread may have
+    // already ref'ed it and is working on it. This results in a race where the
+    // compiler thread may hold the last reference to the plan. We require that
+    // the plan is only destructed on the mutator thread because we piggy back
+    // on the plan to ensure certain other objects are only destructed on the
+    // mutator thread. For example, see Plan::m_identifiersKeptAliveForCleanUp.
+    //
+    // To ensure that the plan is destructed on the mutator and not the compiler
+    // thread, the compiler thread will enqueue any cancelled plan it sees in
+    // the worklist's m_cancelledPlansPendingDestruction. The mutator will later
+    // delete the cancelled plans in m_cancelledPlansPendingDestruction.
+    // However, a mutator should only delete cancelled plans that belong to its
+    // VM. Hence, a cancelled plan needs to keep its m_vm pointer to let the
+    // mutator know which VM it belongs to.
+    // Ref: See Worklist::deleteCancelledPlansForVM().
+    ASSERT(m_vm);
+
+    // Nuke the VM pointer so that pointer comparisons against it will fail.
+    // We rely on VM pointer comparison in many places to filter out Cancelled
+    // plans.
+    m_vm = nuke(m_vm);
+
     m_codeBlock = nullptr;
     m_profiledDFGCodeBlock = nullptr;
     m_mustHandleValues.clear();
index a6acb25..8233730 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013-2018 Apple Inc. All rights reserved.
+ * Copyright (C) 2013-2019 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -38,6 +38,7 @@
 #include "Operands.h"
 #include "ProfilerCompilation.h"
 #include "RecordedStatuses.h"
+#include <wtf/Box.h>
 #include <wtf/HashMap.h>
 #include <wtf/ThreadSafeRefCounted.h>
 
@@ -82,6 +83,7 @@ public:
     void cleanMustHandleValuesIfNecessary();
 
     VM* vm() const { return m_vm; }
+    VM* unnukedVM() const { return unnuke(m_vm); }
 
     CodeBlock* codeBlock() { return m_codeBlock; }
 
@@ -115,6 +117,12 @@ public:
     DeferredCompilationCallback* callback() const { return m_callback.get(); }
     void setCallback(Ref<DeferredCompilationCallback>&& callback) { m_callback = WTFMove(callback); }
 
+    void keepAliveIdentifier(Box<Identifier> identifier)
+    {
+        if (identifier)
+            m_identifiersKeptAliveForCleanUp.append(WTFMove(identifier));
+    }
+
 private:
     bool computeCompileTimes() const;
     bool reportCompileTimes() const;
@@ -129,6 +137,14 @@ private:
     // 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!
 
+#if CPU(ADDRESS64)
+    static constexpr uintptr_t s_nukeBit = 1ull << 63;
+#else
+    static constexpr uintptr_t s_nukeBit = 1ull;
+#endif
+    static VM* nuke(VM* vm) { return bitwise_cast<VM*>(bitwise_cast<uintptr_t>(vm) | s_nukeBit); }
+    static VM* unnuke(VM* vm) { return bitwise_cast<VM*>(bitwise_cast<uintptr_t>(vm) & ~s_nukeBit); }
+
     CompilationMode m_mode;
 
     VM* m_vm;
@@ -165,6 +181,7 @@ private:
     Stage m_stage;
 
     RefPtr<DeferredCompilationCallback> m_callback;
+    Vector<Box<Identifier>, 16> m_identifiersKeptAliveForCleanUp;
 
     MonotonicTime m_timeBeforeFTL;
 };
index 0ef0318..a25df94 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2014-2018 Apple Inc. All rights reserved.
+ * Copyright (C) 2014-2019 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -113,7 +113,7 @@ void Safepoint::cancel()
     RELEASE_ASSERT(m_didCallBegin);
     RELEASE_ASSERT(!m_result.m_didGetCancelled); // We cannot get cancelled twice because subsequent GCs will think that we're alive and they will not do anything to us.
     
-    m_plan.cancel();
+    RELEASE_ASSERT(m_plan.stage() == Plan::Cancelled);
     m_result.m_didGetCancelled = true;
     m_vm = nullptr;
 }
index cd3845d..8db94a5 100644 (file)
@@ -96,12 +96,14 @@ protected:
     WorkResult work() override
     {
         WorkScope workScope(*this);
-        
+
         LockHolder locker(m_data.m_rightToRun);
         {
             LockHolder locker(*m_worklist.m_lock);
-            if (m_plan->stage() == Plan::Cancelled)
+            if (m_plan->stage() == Plan::Cancelled) {
+                m_worklist.m_cancelledPlansPendingDestruction.append(WTFMove(m_plan));
                 return WorkResult::Continue;
+            }
             m_plan->notifyCompiling();
         }
         
@@ -110,7 +112,7 @@ protected:
         
         // There's no way for the GC to be safepointing since we own rightToRun.
         if (m_plan->vm()->heap.worldIsStopped()) {
-            dataLog("Heap is stoped but here we are! (1)\n");
+            dataLog("Heap is stopped but here we are! (1)\n");
             RELEASE_ASSERT_NOT_REACHED();
         }
         m_plan->compileInThread(&m_data);
@@ -123,8 +125,10 @@ protected:
         
         {
             LockHolder locker(*m_worklist.m_lock);
-            if (m_plan->stage() == Plan::Cancelled)
+            if (m_plan->stage() == Plan::Cancelled) {
+                m_worklist.m_cancelledPlansPendingDestruction.append(WTFMove(m_plan));
                 return WorkResult::Continue;
+            }
             
             m_plan->notifyReady();
             
@@ -133,9 +137,8 @@ protected:
                 dataLog(": Compiled ", m_plan->key(), " asynchronously\n");
             }
             
-            m_worklist.m_readyPlans.append(m_plan);
-
             RELEASE_ASSERT(!m_plan->vm()->heap.worldIsStopped());
+            m_worklist.m_readyPlans.append(WTFMove(m_plan));
             m_worklist.m_planCompiled.notifyAll();
         }
         
@@ -301,10 +304,40 @@ void Worklist::waitUntilAllPlansForVMAreReady(VM& vm)
     }
 }
 
+void Worklist::deleteCancelledPlansForVM(LockHolder&, VM& vm)
+{
+    RELEASE_ASSERT(vm.currentThreadIsHoldingAPILock());
+#if !ASSERT_DISABLED
+    HashSet<RefPtr<Plan>> removedPlans;
+#endif
+
+    for (size_t i = 0; i < m_cancelledPlansPendingDestruction.size(); ++i) {
+        RefPtr<Plan> plan = m_cancelledPlansPendingDestruction[i];
+        if (plan->unnukedVM() != &vm)
+            continue;
+        m_cancelledPlansPendingDestruction[i--] = m_cancelledPlansPendingDestruction.last();
+        m_cancelledPlansPendingDestruction.removeLast();
+#if !ASSERT_DISABLED
+        removedPlans.add(plan);
+#endif
+    }
+
+#if !ASSERT_DISABLED
+    while (!removedPlans.isEmpty()) {
+        RefPtr<Plan> plan = removedPlans.takeAny();
+        RELEASE_ASSERT(plan->stage() == Plan::Cancelled);
+        RELEASE_ASSERT(plan->refCount() == 1);
+    }
+#endif
+}
+
 void Worklist::removeAllReadyPlansForVM(VM& vm, Vector<RefPtr<Plan>, 8>& myReadyPlans)
 {
     DeferGC deferGC(vm.heap);
     LockHolder locker(*m_lock);
+    ASSERT(vm.currentThreadIsHoldingAPILock());
+
+    deleteCancelledPlansForVM(locker, vm);
     for (size_t i = 0; i < m_readyPlans.size(); ++i) {
         RefPtr<Plan> plan = m_readyPlans[i];
         if (plan->vm() != &vm)
@@ -407,6 +440,8 @@ void Worklist::removeDeadPlans(VM& vm)
     {
         LockHolder locker(*m_lock);
         HashSet<CompilationKey> deadPlanKeys;
+        bool isInMutator = vm.currentThreadIsHoldingAPILock();
+
         for (PlanMap::iterator iter = m_plans.begin(); iter != m_plans.end(); ++iter) {
             Plan* plan = iter->value.get();
             if (plan->vm() != &vm)
@@ -420,8 +455,12 @@ void Worklist::removeDeadPlans(VM& vm)
             deadPlanKeys.add(plan->key());
         }
         if (!deadPlanKeys.isEmpty()) {
-            for (HashSet<CompilationKey>::iterator iter = deadPlanKeys.begin(); iter != deadPlanKeys.end(); ++iter)
-                m_plans.take(*iter)->cancel();
+            for (HashSet<CompilationKey>::iterator iter = deadPlanKeys.begin(); iter != deadPlanKeys.end(); ++iter) {
+                RefPtr<Plan> plan = m_plans.take(*iter);
+                plan->cancel();
+                if (!isInMutator)
+                    m_cancelledPlansPendingDestruction.append(plan);
+            }
             Deque<RefPtr<Plan>> newQueue;
             while (!m_queue.isEmpty()) {
                 RefPtr<Plan> plan = m_queue.takeFirst();
@@ -457,6 +496,9 @@ void Worklist::removeNonCompilingPlansForVM(VM& vm)
     LockHolder locker(*m_lock);
     HashSet<CompilationKey> deadPlanKeys;
     Vector<RefPtr<Plan>> deadPlans;
+    ASSERT(vm.currentThreadIsHoldingAPILock());
+
+    deleteCancelledPlansForVM(locker, vm);
     for (auto& entry : m_plans) {
         Plan* plan = entry.value.get();
         if (plan->vm() != &vm)
index 82473a7..744d60f 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013-2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2013-2019 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -93,6 +93,7 @@ private:
     static void threadFunction(void* argument);
     
     void removeAllReadyPlansForVM(VM&, Vector<RefPtr<Plan>, 8>&);
+    void deleteCancelledPlansForVM(LockHolder&, VM&);
 
     void dump(const AbstractLocker&, PrintStream&) const;
     
@@ -113,6 +114,7 @@ private:
     // Used to quickly find which plans have been compiled and are ready to
     // be completed.
     Vector<RefPtr<Plan>, 16> m_readyPlans;
+    Vector<RefPtr<Plan>, 4> m_cancelledPlansPendingDestruction;
     Ref<AutomaticThreadCondition> m_planEnqueued;
     Condition m_planCompiled;
 
index ab64534..530fe52 100644 (file)
@@ -55,6 +55,7 @@ public:
     static Symbol* createWithDescription(VM&, const String&);
     JS_EXPORT_PRIVATE static Symbol* create(VM&, SymbolImpl& uid);
 
+    SymbolImpl& uid() const { return m_privateName.uid(); }
     PrivateName privateName() const { return m_privateName; }
     String descriptiveString() const;
     String description() const;