GC can cause debug mode to be turned off, leading to crash




JavaScript Engine
6 years ago
6 years ago


(Reporter: jorendorff, Unassigned)


Other Branch

Firefox Tracking Flags

(Not tracked)




6 years ago
Right now a Debug object can be gc'd if
 * it has no debuggees OR it has no hooks OR its .enabled property is false;
 * it is otherwise unreachable

Debug mode is a little different. A compartment has debug mode on if
 * JSD1 wants debug mode on, due to a JS_SetDebugMode* call
 * the compartment contains any JSD2 debuggee globals

This causes three problems.

1. When a Debug object is GC'd, that can cause debug mode to be turned
   off for one or more compartments. Turning debug mode off at
   unpredictable times is bad because methodjit code invalidated by
   turning off debug mode will just crash.

2. When removeDebuggee is called, that can cause debug mode to be
   turned off for a compartment. Same problem.

3. Setting .enabled to false does not turn off debug mode anywhere, so
   it does not reduce overhead as much as it's intended to.

#2 will be fixed shortly; removeDebuggee will throw rather than cause
debug mode to be turned off for a compartment with running scripts.

To fix #1 and #3, the plan is to change the rules to be like this.

A Debug object can be GC'd if
 * it has no debuggees OR its .enabled property is false;
 * it is otherwise unreachable

A compartment is in debug mode if
 * JSD1 wants debug mode on,
 * an enabled Debug object has debuggees in that compartment.

With these rules, a Debug object that is keeping a compartment in
debug mode can't be GC'd, so GC will never turn off debug mode.

Comment 1

6 years ago
This won't work because GC can collect the last debuggee in a compartment. Then it would need to disable debug mode in that compartment.

New proposal: Make disabling debug mode infallible. Sometimes it'll have to leave debug-mode mjit scripts; trash them during a later GC, whenever it's safe.

Comment 2

6 years ago
Turns out we already walk all mjit scripts in JSCompartment::sweep, so this doesn't even cost much extra.

Comment 3

6 years ago

5 new lines of code.

This adds a test for the scenario described in comment 0. Leaving this bug open since I think we still want a rules change to align what keeps Debug objects alive with what turns on debug mode; it should be a simplification.


6 years ago
Component: JavaScript Debugging/Profiling APIs → JavaScript Engine
Product: Core → Core

Comment 4

6 years ago
This is obsolete. Comment 3 is too vague to be useful, and the actual crash was fixed long ago.
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.