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 `Debugger`s. - 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 `Debugger`s 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 `Debugger`s 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. (`Debugger`s 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 `Debugger`s 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.
Bug 1592116 Comment 0 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
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 `Debugger`s. - 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 `Debugger`s 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 `Debugger`s 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. (`Debugger`s 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 `Debugger`s 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.