From 1300cd70bd4e7f5290cf92363756ad3b37826018 Mon Sep 17 00:00:00 2001 From: Jon Coppeard Date: Tue, 14 Feb 2023 10:32:47 +0000 Subject: [PATCH] Part 2: Require no GC when giving out references to the realm's debugger vector Conflict:NA Reference:https://hg.mozilla.org/integration/autoland/rev/eb7a5d363182c55a363fe46defe670f060928e76 --- js/src/debugger/DebugAPI-inl.h | 13 +++-- js/src/debugger/DebugAPI.h | 10 +++- js/src/debugger/Debugger.cpp | 94 ++++++++++++++++++---------------- js/src/vm/GlobalObject.h | 6 ++- js/src/vm/Realm.h | 5 +- 5 files changed, 74 insertions(+), 54 deletions(-) diff --git a/js/src/debugger/DebugAPI-inl.h b/js/src/debugger/DebugAPI-inl.h index bbd6714247..5e23e34259 100644 --- a/js/src/debugger/DebugAPI-inl.h +++ b/js/src/debugger/DebugAPI-inl.h @@ -7,6 +7,7 @@ #ifndef debugger_DebugAPI_inl_h #define debugger_DebugAPI_inl_h +#include "gc/GC.h" #include "debugger/DebugAPI.h" #include "vm/GeneratorObject.h" @@ -45,9 +46,10 @@ void DebugAPI::onNewGlobalObject(JSContext* cx, Handle global) { /* static */ void DebugAPI::notifyParticipatesInGC(GlobalObject* global, uint64_t majorGCNumber) { - Realm::DebuggerVector& dbgs = global->getDebuggers(); + JS::AutoAssertNoGC nogc; + Realm::DebuggerVector& dbgs = global->getDebuggers(nogc); if (!dbgs.empty()) { - slowPathNotifyParticipatesInGC(majorGCNumber, dbgs); + slowPathNotifyParticipatesInGC(majorGCNumber, dbgs, nogc); } } @@ -55,12 +57,15 @@ void DebugAPI::notifyParticipatesInGC(GlobalObject* global, bool DebugAPI::onLogAllocationSite(JSContext* cx, JSObject* obj, HandleSavedFrame frame, mozilla::TimeStamp when) { - Realm::DebuggerVector& dbgs = cx->global()->getDebuggers(); + // slowPathOnLogAllocationSite creates GC things so we must suppress GC here. + gc::AutoSuppressGC nogc(cx); + + Realm::DebuggerVector& dbgs = cx->global()->getDebuggers(nogc); if (dbgs.empty()) { return true; } RootedObject hobj(cx, obj); - return slowPathOnLogAllocationSite(cx, hobj, frame, when, dbgs); + return slowPathOnLogAllocationSite(cx, hobj, frame, when, dbgs, nogc); } /* static */ diff --git a/js/src/debugger/DebugAPI.h b/js/src/debugger/DebugAPI.h index 9792a8948f..375b3a68dd 100644 --- a/js/src/debugger/DebugAPI.h +++ b/js/src/debugger/DebugAPI.h @@ -21,6 +21,10 @@ class AbstractGeneratorObject; class DebugScriptMap; class PromiseObject; +namespace gc { +class AutoSuppressGC; +} // namespace gc + /** * DebugAPI::onNativeCall allows the debugger to call callbacks just before * some native functions are to be executed. It also allows the hooks @@ -349,10 +353,12 @@ class DebugAPI { static void slowPathOnNewGlobalObject(JSContext* cx, Handle global); static void slowPathNotifyParticipatesInGC(uint64_t majorGCNumber, - JS::Realm::DebuggerVector& dbgs); + JS::Realm::DebuggerVector& dbgs, + const JS::AutoRequireNoGC& nogc); [[nodiscard]] static bool slowPathOnLogAllocationSite( JSContext* cx, HandleObject obj, HandleSavedFrame frame, - mozilla::TimeStamp when, JS::Realm::DebuggerVector& dbgs); + mozilla::TimeStamp when, JS::Realm::DebuggerVector& dbgs, + const gc::AutoSuppressGC& nogc); [[nodiscard]] static bool slowPathOnLeaveFrame(JSContext* cx, AbstractFramePtr frame, jsbytecode* pc, bool ok); diff --git a/js/src/debugger/Debugger.cpp b/js/src/debugger/Debugger.cpp index 56a172c457..7f5756e338 100644 --- a/js/src/debugger/Debugger.cpp +++ b/js/src/debugger/Debugger.cpp @@ -755,7 +755,7 @@ static bool DebuggerExists( // explicitly. JS::AutoSuppressGCAnalysis nogc; - for (Realm::DebuggerVectorEntry& entry : global->getDebuggers()) { + for (Realm::DebuggerVectorEntry& entry : global->getDebuggers(nogc)) { // Callbacks should not create new references to the debugger, so don't // use a barrier. This allows this method to be called during GC. if (predicate(entry.dbg.unbarrieredGet())) { @@ -805,7 +805,8 @@ bool DebuggerList::init(JSContext* cx) { // Make a copy of the list, since the original is mutable and we will be // calling into arbitrary JS. Handle global = cx->global(); - for (Realm::DebuggerVectorEntry& entry : global->getDebuggers()) { + JS::AutoAssertNoGC nogc; + for (Realm::DebuggerVectorEntry& entry : global->getDebuggers(nogc)) { Debugger* dbg = entry.dbg; if (dbg->isHookCallAllowed(cx) && hookIsEnabled(dbg)) { if (!debuggers.append(ObjectValue(*dbg->toJSObject()))) { @@ -928,20 +929,24 @@ bool DebugAPI::slowPathOnResumeFrame(JSContext* cx, AbstractFramePtr frame) { // frame is observable. FrameIter iter(cx); MOZ_ASSERT(iter.abstractFramePtr() == frame); - for (Realm::DebuggerVectorEntry& entry : frame.global()->getDebuggers()) { - Debugger* dbg = entry.dbg; - if (Debugger::GeneratorWeakMap::Ptr generatorEntry = - dbg->generatorFrames.lookup(genObj)) { - DebuggerFrame* frameObj = generatorEntry->value(); - MOZ_ASSERT(&frameObj->unwrappedGenerator() == genObj); - if (!dbg->frames.putNew(frame, frameObj)) { - ReportOutOfMemory(cx); - return false; - } - if (!frameObj->resume(iter)) { - return false; - } - } + { + JS::AutoAssertNoGC nogc; + for (Realm::DebuggerVectorEntry& entry : + frame.global()->getDebuggers(nogc)) { + Debugger* dbg = entry.dbg; + if (Debugger::GeneratorWeakMap::Ptr generatorEntry = + dbg->generatorFrames.lookup(genObj)) { + DebuggerFrame* frameObj = generatorEntry->value(); + MOZ_ASSERT(&frameObj->unwrappedGenerator() == genObj); + if (!dbg->frames.putNew(frame, frameObj)) { + ReportOutOfMemory(cx); + return false; + } + if (!frameObj->resume(iter)) { + return false; + } + } + } } terminateDebuggerFramesGuard.release(); @@ -2606,7 +2611,8 @@ bool DebugAPI::onSingleStep(JSContext* cx) { uint32_t liveStepperCount = 0; uint32_t suspendedStepperCount = 0; JSScript* trappingScript = iter.script(); - for (Realm::DebuggerVectorEntry& entry : cx->global()->getDebuggers()) { + JS::AutoAssertNoGC nogc; + for (Realm::DebuggerVectorEntry& entry : cx->global()->getDebuggers(nogc)) { Debugger* dbg = entry.dbg; for (Debugger::FrameMap::Range r = dbg->frames.all(); !r.empty(); r.popFront()) { @@ -2789,7 +2795,8 @@ void DebugAPI::slowPathOnNewGlobalObject(JSContext* cx, /* static */ void DebugAPI::slowPathNotifyParticipatesInGC(uint64_t majorGCNumber, - Realm::DebuggerVector& dbgs) { + Realm::DebuggerVector& dbgs, + const JS::AutoRequireNoGC& nogc) { for (Realm::DebuggerVector::Range r = dbgs.all(); !r.empty(); r.popFront()) { if (!r.front().dbg.unbarrieredGet()->debuggeeIsBeingCollected( majorGCNumber)) { @@ -2806,7 +2813,8 @@ void DebugAPI::slowPathNotifyParticipatesInGC(uint64_t majorGCNumber, /* static */ Maybe DebugAPI::allocationSamplingProbability(GlobalObject* global) { - Realm::DebuggerVector& dbgs = global->getDebuggers(); + JS::AutoAssertNoGC nogc; + Realm::DebuggerVector& dbgs = global->getDebuggers(nogc); if (dbgs.empty()) { return Nothing(); } @@ -2836,23 +2844,13 @@ Maybe DebugAPI::allocationSamplingProbability(GlobalObject* global) { bool DebugAPI::slowPathOnLogAllocationSite(JSContext* cx, HandleObject obj, HandleSavedFrame frame, mozilla::TimeStamp when, - Realm::DebuggerVector& dbgs) { + Realm::DebuggerVector& dbgs, + const gc::AutoSuppressGC& nogc) { MOZ_ASSERT(!dbgs.empty()); mozilla::DebugOnly begin = dbgs.begin(); - // Root all the Debuggers while we're iterating over them; - // appendAllocationSite calls Compartment::wrap, and thus can GC. - // - // SpiderMonkey protocol is generally for the caller to prove that it has - // rooted the stuff it's asking you to operate on (i.e. by passing a - // Handle), but in this case, we're iterating over a global's list of - // Debuggers, and globals only hold their Debuggers weakly. - Rooted> activeDebuggers(cx, GCVector(cx)); - for (auto p = dbgs.begin(); p < dbgs.end(); p++) { - if (!activeDebuggers.append(p->dbg->object)) { - return false; - } - } + // GC is suppressed so we can iterate over the debuggers; appendAllocationSite + // calls Compartment::wrap, and thus could GC. for (auto p = dbgs.begin(); p < dbgs.end(); p++) { // The set of debuggers had better not change while we're iterating, @@ -3269,8 +3267,7 @@ template void Debugger::forEachOnStackDebuggerFrame(AbstractFramePtr frame, const JS::AutoRequireNoGC& nogc, FrameFn fn) { - // GC is disallowed because it may mutate the vector we are iterating. - for (Realm::DebuggerVectorEntry& entry : frame.global()->getDebuggers()) { + for (Realm::DebuggerVectorEntry& entry : frame.global()->getDebuggers(nogc)) { Debugger* dbg = entry.dbg; if (FrameMap::Ptr frameEntry = dbg->frames.lookup(frame)) { fn(dbg, frameEntry->value()); @@ -3288,7 +3285,7 @@ void Debugger::forEachOnStackOrSuspendedDebuggerFrame( cx, frame.isGeneratorFrame() ? GetGeneratorObjectForFrame(cx, frame) : nullptr); - for (Realm::DebuggerVectorEntry& entry : frame.global()->getDebuggers()) { + for (Realm::DebuggerVectorEntry& entry : frame.global()->getDebuggers(nogc)) { Debugger* dbg = entry.dbg; DebuggerFrame* frameObj = nullptr; @@ -3521,7 +3518,8 @@ bool Debugger::cannotTrackAllocations(const GlobalObject& global) { /* static */ bool DebugAPI::isObservedByDebuggerTrackingAllocations( const GlobalObject& debuggee) { - for (Realm::DebuggerVectorEntry& entry : debuggee.getDebuggers()) { + JS::AutoAssertNoGC nogc; + for (Realm::DebuggerVectorEntry& entry : debuggee.getDebuggers(nogc)) { // Use unbarrieredGet() to prevent triggering read barrier while // collecting, this is safe as long as dbg does not escape. Debugger* dbg = entry.dbg.unbarrieredGet(); @@ -3821,7 +3819,9 @@ void DebugAPI::slowPathTraceGeneratorFrame(JSTracer* tracer, return; } - for (Realm::DebuggerVectorEntry& entry : generator->realm()->getDebuggers()) { + JS::AutoAssertNoGC nogc; + for (Realm::DebuggerVectorEntry& entry : + generator->realm()->getDebuggers(nogc)) { Debugger* dbg = entry.dbg.unbarrieredGet(); if (Debugger::GeneratorWeakMap::Ptr entry = @@ -3891,7 +3891,8 @@ void Debugger::trace(JSTracer* trc) { /* static */ void DebugAPI::traceFromRealm(JSTracer* trc, Realm* realm) { - for (Realm::DebuggerVectorEntry& entry : realm->getDebuggers()) { + JS::AutoAssertNoGC nogc; + for (Realm::DebuggerVectorEntry& entry : realm->getDebuggers(nogc)) { TraceEdge(trc, &entry.debuggerLink, "realm debugger"); } } @@ -4468,7 +4469,7 @@ bool Debugger::CallData::removeDebuggee() { // Only update the realm if there are no Debuggers left, as it's // expensive to check if no other Debugger has a live script or frame // hook on any of the current on-stack debuggee frames. - if (global->getDebuggers().empty() && !obs.add(global->realm())) { + if (!global->hasDebuggers() && !obs.add(global->realm())) { return false; } if (!updateExecutionObservability(cx, obs, NotObserving)) { @@ -4489,7 +4490,7 @@ bool Debugger::CallData::removeAllDebuggees() { FromSweep::No); // See note about adding to the observable set in removeDebuggee. - if (global->getDebuggers().empty() && !obs.add(global->realm())) { + if (!global->hasDebuggers() && !obs.add(global->realm())) { return false; } } @@ -4698,7 +4699,8 @@ bool Debugger::addDebuggeeGlobal(JSContext* cx, Handle global) { // Find all realms containing debuggers debugging realm's global object. // Add those realms to visited. if (realm->isDebuggee()) { - for (Realm::DebuggerVectorEntry& entry : realm->getDebuggers()) { + JS::AutoAssertNoGC nogc; + for (Realm::DebuggerVectorEntry& entry : realm->getDebuggers(nogc)) { Realm* next = entry.dbg->object->realm(); if (std::find(visited.begin(), visited.end(), next) == visited.end()) { if (!visited.append(next)) { @@ -4729,7 +4731,8 @@ bool Debugger::addDebuggeeGlobal(JSContext* cx, Handle global) { } // (1) - auto& globalDebuggers = global->getDebuggers(); + JS::AutoAssertNoGC nogc; + auto& globalDebuggers = global->getDebuggers(nogc); if (!globalDebuggers.append(Realm::DebuggerVectorEntry(this, debuggeeLink))) { ReportOutOfMemory(cx); return false; @@ -4847,7 +4850,8 @@ void Debugger::removeDebuggeeGlobal(JSFreeOp* fop, GlobalObject* global, } } - auto& globalDebuggersVector = global->getDebuggers(); + JS::AutoAssertNoGC nogc; + auto& globalDebuggersVector = global->getDebuggers(nogc); // The relation must be removed from up to three places: // globalDebuggersVector and debuggees for sure, and possibly the @@ -4886,7 +4890,7 @@ void Debugger::removeDebuggeeGlobal(JSFreeOp* fop, GlobalObject* global, Debugger::removeAllocationsTracking(*global); } - if (global->realm()->getDebuggers().empty()) { + if (!global->realm()->hasDebuggers()) { global->realm()->unsetIsDebuggee(); } else { global->realm()->updateDebuggerObservesAllExecution(); diff --git a/js/src/vm/GlobalObject.h b/js/src/vm/GlobalObject.h index dddffcf7a9..0f535eb13f 100644 --- a/js/src/vm/GlobalObject.h +++ b/js/src/vm/GlobalObject.h @@ -857,9 +857,11 @@ class GlobalObject : public NativeObject { Handle global, const JSFunctionSpec* builtins); - Realm::DebuggerVector& getDebuggers() const { - return realm()->getDebuggers(); + // Disallow GC as it may mutate the vector. + Realm::DebuggerVector& getDebuggers(const JS::AutoRequireNoGC& nogc) const { + return realm()->getDebuggers(nogc); } + bool hasDebuggers() const { return realm()->hasDebuggers(); } inline NativeObject* getForOfPICObject() { Value forOfPIC = getReservedSlot(FOR_OF_PIC_CHAIN); diff --git a/js/src/vm/Realm.h b/js/src/vm/Realm.h index 1f8852befc..bfedf5d6de 100644 --- a/js/src/vm/Realm.h +++ b/js/src/vm/Realm.h @@ -706,7 +706,10 @@ class JS::Realm : public JS::shadow::Realm { void setIsDebuggee(); void unsetIsDebuggee(); - DebuggerVector& getDebuggers() { return debuggers_; }; + DebuggerVector& getDebuggers(const JS::AutoRequireNoGC& nogc) { + return debuggers_; + }; + bool hasDebuggers() const { return !debuggers_.empty(); } // True if this compartment's global is a debuggee of some Debugger // object with a live hook that observes all execution; e.g., -- 2.33.0