Skip to content

Commit

Permalink
Bug 1816158 - Part 2: Require no GC when giving out references to the…
Browse files Browse the repository at this point in the history 
… realm's debugger vector r=sfink

To prevent any other instances of this problme we can update the getDebuggers()
methods on the global and the realm to require no GC.

Depends on D169701

Differential Revision: https://phabricator.services.mozilla.com/D169702
  • Loading branch information
@jonco3
jonco3 committed Feb 14, 2023
1 parent 661875c commit e7e5cc9
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 53 deletions.
13 changes: 9 additions & 4 deletions js/src/debugger/DebugAPI-inl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

#include "debugger/DebugAPI.h"

#include "gc/GC.h"
#include "vm/GeneratorObject.h"
#include "vm/PromiseObject.h" // js::PromiseObject

Expand Down Expand Up @@ -45,22 +46,26 @@ void DebugAPI::onNewGlobalObject(JSContext* cx, Handle<GlobalObject*> 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);
}
}

/* static */
bool DebugAPI::onLogAllocationSite(JSContext* cx, JSObject* obj,
Handle<SavedFrame*> 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 */
Expand Down
10 changes: 8 additions & 2 deletions js/src/debugger/DebugAPI.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -352,10 +356,12 @@ class DebugAPI {
static void slowPathOnNewGlobalObject(JSContext* cx,
Handle<GlobalObject*> 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, Handle<SavedFrame*> 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,
const jsbytecode* pc, bool ok);
Expand Down
91 changes: 47 additions & 44 deletions js/src/debugger/Debugger.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -739,7 +739,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())) {
Expand Down Expand Up @@ -795,7 +795,8 @@ bool DebuggerList<HookIsEnabledFun>::init(JSContext* cx) {
// Make a copy of the list, since the original is mutable and we will be
// calling into arbitrary JS.
Handle<GlobalObject*> 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()))) {
Expand Down Expand Up @@ -919,18 +920,22 @@ 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;
}
}
}
}
Expand Down Expand Up @@ -2624,7 +2629,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()) {
Expand Down Expand Up @@ -2807,7 +2813,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)) {
Expand All @@ -2824,7 +2831,8 @@ void DebugAPI::slowPathNotifyParticipatesInGC(uint64_t majorGCNumber,

/* static */
Maybe<double> DebugAPI::allocationSamplingProbability(GlobalObject* global) {
Realm::DebuggerVector& dbgs = global->getDebuggers();
JS::AutoAssertNoGC nogc;
Realm::DebuggerVector& dbgs = global->getDebuggers(nogc);
if (dbgs.empty()) {
return Nothing();
}
Expand Down Expand Up @@ -2854,23 +2862,13 @@ Maybe<double> DebugAPI::allocationSamplingProbability(GlobalObject* global) {
bool DebugAPI::slowPathOnLogAllocationSite(JSContext* cx, HandleObject obj,
Handle<SavedFrame*> frame,
mozilla::TimeStamp when,
Realm::DebuggerVector& dbgs) {
Realm::DebuggerVector& dbgs,
const gc::AutoSuppressGC& nogc) {
MOZ_ASSERT(!dbgs.empty());
mozilla::DebugOnly<Realm::DebuggerVectorEntry*> 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<GCVector<JSObject*>> activeDebuggers(cx, GCVector<JSObject*>(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,
Expand Down Expand Up @@ -3283,8 +3281,7 @@ template <typename FrameFn>
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());
Expand All @@ -3301,8 +3298,7 @@ void Debugger::forEachOnStackOrSuspendedDebuggerFrame(
cx, frame.isGeneratorFrame() ? GetGeneratorObjectForFrame(cx, frame)
: nullptr);

// 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;

DebuggerFrame* frameObj = nullptr;
Expand Down Expand Up @@ -3556,7 +3552,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();
Expand Down Expand Up @@ -3863,7 +3860,9 @@ void DebugAPI::slowPathTraceGeneratorFrame(JSTracer* tracer,
lock.emplace(marker->runtime());
}

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 =
Expand Down Expand Up @@ -3933,7 +3932,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");
}
}
Expand Down Expand Up @@ -4533,7 +4533,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)) {
Expand All @@ -4553,7 +4553,7 @@ bool Debugger::CallData::removeAllDebuggees() {
dbg->removeDebuggeeGlobal(cx->gcContext(), global, &e, 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;
}
}
Expand Down Expand Up @@ -4762,7 +4762,8 @@ bool Debugger::addDebuggeeGlobal(JSContext* cx, Handle<GlobalObject*> 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)) {
Expand Down Expand Up @@ -4793,7 +4794,8 @@ bool Debugger::addDebuggeeGlobal(JSContext* cx, Handle<GlobalObject*> 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;
Expand Down Expand Up @@ -4912,7 +4914,8 @@ void Debugger::removeDebuggeeGlobal(JS::GCContext* gcx, 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
Expand Down Expand Up @@ -4951,7 +4954,7 @@ void Debugger::removeDebuggeeGlobal(JS::GCContext* gcx, GlobalObject* global,
Debugger::removeAllocationsTracking(*global);
}

if (global->realm()->getDebuggers().empty()) {
if (!global->realm()->hasDebuggers()) {
global->realm()->unsetIsDebuggee();
} else {
global->realm()->updateDebuggerObservesAllExecution();
Expand Down
6 changes: 4 additions & 2 deletions js/src/vm/GlobalObject.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -986,9 +986,11 @@ class GlobalObject : public NativeObject {

static bool initStandardClasses(JSContext* cx, Handle<GlobalObject*> global);

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() { return data().forOfPICChain; }
static NativeObject* getOrCreateForOfPICObject(JSContext* cx,
Expand Down
5 changes: 4 additions & 1 deletion js/src/vm/Realm.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -660,7 +660,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.,
Expand Down

0 comments on commit e7e5cc9

Please sign in to comment.