Bugzilla

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.

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.