SpiderMonkey's algorithm for deciding whether a Debugger can be garbage collected is needlessly complex, and should be simplified.

If a Debugger has an onDebuggerStatement, onExceptionUnwind, onNewScript, or onEnterFrame hook set, and it has live debuggees, then it would be incorrect to collect the Debugger even if it is not otherwise reachable. GC should not have observable side effects, and removing the Debugger would cause the hooks not to fire.

At present, we implement this behavior by incorporating Debugger-specific behavior into the garbage collector's weak marking loop:

• js::gc::GCRuntime::markWeakReferences repeatedly calls js::DebugAPI::markIteratively.

• markIteratively scans all debuggee realms in the runtime, looking for marked globals.

  • If a marked global is a debuggee, then we iterate over its Debuggers.

    • If a Debugger is not marked, but has any of the hooks mentioned above set (according to Debugger::hasAnyLiveHooks), then we mark it.

• If the above process caused any new Debuggers to be marked, then we make another iteration around the loop.

The effect of all this is equivalent to having a strong cross-compartment reference from each debuggee realm to each of its Debuggers that has any hooks set. We can implement this as follows:

• Let Realm::traceGlobal iterate over the the Realm's DebuggerVector:

  • If a Debugger has any of the hooks mentioned above set, then it should be traced. (Debuggers without hooks set should not be traced!)

• Since all interactions between the debugger and the rest of SpiderMonkey should go through DebugAPI, we'll need a new DebugAPI method for traceGlobal to use that takes a Debugger* and returns true if it has any of those hooks set.

• Debugger::hasAnyLiveHooks should no longer check for those hooks.

At present, Debugger::hasAnyLiveHooks also takes care of looking for suspended generator frames with hooks set, which also must cause their owning Debuggers to be retained, so we can't delete that function entirely. However, this change leaves that function with a misleading name; perhaps Debugger::hasSuspendedGeneratorHooks would be better.

Hey Jim, I'd love to chat up about this tomorrow. Before I dive into questions though, I want to make sure I followed roughly what you were saying. Is this what you had in mind? https://phabricator.services.mozilla.com/D51097

I'm running into two issues and I want your thoughts:

1. If traceGlobal in the Realm traces the edge into Debugger, it fails some assertions because it isn't in the cross-compartment map. Is that something that I should be explicitly adding to the map now, or should we be editing DebugAPI::edgeIsInDebuggerWeakmap (and probably renaming it), so that these cross-compartment links work? The patch above has that logic, but not sure it's the right way to do it.

2. There seems to be an issue with the order of marking and sweeping and I'm at a bit of a loss. For generator frames, we only want the to hold a strong link to the Debugger.Frame as long as there is as the AbstractGeneratorObject is alive. What should be responsible for that? It looks like, previously, this was done as a side-effect of

forEachWeakMap([trc](auto& weakMap) { weakMap.trace(trc); });

because by the time that executed, the traversal will have already marked the generator, so marking the WeakMap will also mark the DebuggerFrame.

Because we've moved the call to Debugger::trace call from markIteratively to traceGlobal, it seems that the AbstractGeneratorObject has not yet been marked, meaning that nothing ever marks the Debugger.Frame. That is causing js/src/jit-test/tests/debug/Frame-onStep-async-gc-01.js to fail with my current patch.

(In reply to Logan Smyth [:loganfsmyth] from comment #2)

1. If traceGlobal in the Realm traces the edge into Debugger, it fails some assertions because it isn't in the cross-compartment map. Is that something that I should be explicitly adding to the map now, or should we be editing DebugAPI::edgeIsInDebuggerWeakmap (and probably renaming it), so that these cross-compartment links work? The patch above has that logic, but not sure it's the right way to do it.

Yes, we are replacing a situation where one thing's liveness has been indirectly arranged to ensure another thing's liveness with a simple owning edge, so that's going to trip tests that are concerned with edges, hmm.

Changing edgeIsInDebuggerWeakmap is certainly one approach. (Agree about the need to rename.)

Another would be for Realm::debuggees_ to hold cross-compartment wrappers to the Debuggers' JSObjects. Then all uses of that array would need to unwrap. But I think unwrapping is forbidden during GC.

Another issue I noticed: I think the type of Realm::DebuggerVector may need to change. WeakHeapPtr<Debugger*> ensures that any read of that pointer calls Debugger::readBarrier to mark the referent (if you don't know why this behavior is usually needed for weak pointers, see the "GC Barriers" SMDOC comment, or ask me) and that is obviously not the right behavior for this use case: we're trying to decide whether to mark it at all.

It would be smart to talk to jonco about how best to handle this. He's on London time.

2. There seems to be an issue with the order of marking and sweeping and I'm at a bit of a loss. For generator frames, we only want the to hold a strong link to the Debugger.Frame as long as there is as the AbstractGeneratorObject is alive. What should be responsible for that? It looks like, previously, this was done as a side-effect of

forEachWeakMap([trc](auto& weakMap) { weakMap.trace(trc); });

because by the time that executed, the traversal will have already marked the generator, so marking the WeakMap will also mark the DebuggerFrame.

WeakMap marking is very special. Let's talk about this live at some point.

Because we've moved the call to Debugger::trace call from markIteratively to traceGlobal, it seems that the AbstractGeneratorObject has not yet been marked, meaning that nothing ever marks the Debugger.Frame. That is causing js/src/jit-test/tests/debug/Frame-onStep-async-gc-01.js to fail with my current patch.

Yes, this is supposed to magically work out.

(In reply to Jim Blandy :jimb from comment #3)

Another would be for Realm::debuggees_ to hold cross-compartment wrappers to the Debuggers' JSObjects. Then all uses of that array would need to unwrap. But I think unwrapping is forbidden during GC.

It's not forbidden but you need to use js::UncheckedUnwrapWithoutExpose (which doesn't have a gray-unmarking read barrier). I'd say that using CCWs for this is a good way forward.

(In reply to Jon Coppeard (:jonco) from comment #5)

It's not forbidden but you need to use js::UncheckedUnwrapWithoutExpose (which doesn't have a gray-unmarking read barrier). I'd say that using CCWs for this is a good way forward.

Oh. I should probably use that in Breakpoint, instead of including a separate Debugger* field.

Okay, I managed to catch jonco this morning. Here's how we manage the realm's DebuggerVector:

DebuggerVector should be a simple js::Vector<js::WeakHeapPtr<JSObject*>>, holding CCWs of Debugger JS objects.

When a Realm is traced, we traverse its DebuggerVector (not tracing, just dereferences), call js::UncheckedUnwrapWithoutExpose to cross the CCW, call Debugger::fromJSObject to get the actual Debugger*, and check whether it has hooks set. If it does, then we call TraceEdge on that DebuggerVector element. This marks the Debugger.

When a Realm is swept, we must call IsAboutToBeFinalized on each element of the DebuggerVector, and remove those elements for which it returns true from the vector. The elements that we did not call TraceEdge on earlier, but that are nonetheless not being finalized (presumably because they are otherwise reachable), get their pointers fixed up by IsAboutToBeFinalized.

Jonco adds:

A good place to do the sweeping would be from GCRuntime::sweepDebuggerOnMainThread, where it already does per-realm sweeping by calling sweepDebugEnvironments on realms.

(In reply to Jim Blandy :jimb from comment #7)

DebuggerVector should be a simple js::Vector<js::WeakHeapPtr<JSObject*>>, holding CCWs of Debugger JS objects.

I know I said this would work but I've spotted a problem. CCWs are not kept alive while the target is alive but only if we trace pointers to them. If we don't trace one of these it may die (and be removed from the list) even if the target debugger is still present, which is not what we want to happen here AIUI.

How is this looking now that bug 1597925 has landed?

I think this should be all set to land now. I actually missed that Jim had okayed it with his last review, so was thinking he needed to do another pass. Given how close we are to release now though, I'll probably just aim to land this once the release is cut, since it's not missing-critical to anything.

https://hg.mozilla.org/mozilla-central/rev/0414d05a1653