Last Comment Bug 665694 - GC can cause debug mode to be turned off, leading to crash
: GC can cause debug mode to be turned off, leading to crash
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
: jsd
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-20 14:04 PDT by Jason Orendorff [:jorendorff]
Modified: 2011-08-16 08:08 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description Jason Orendorff [:jorendorff] 2011-06-20 14:04:43 PDT
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;
AND
 * 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
OR
 * 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;
AND
 * it is otherwise unreachable

A compartment is in debug mode if
 * JSD1 wants debug mode on,
OR
 * 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 Jason Orendorff [:jorendorff] 2011-06-23 08:18:46 PDT
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 Jason Orendorff [:jorendorff] 2011-06-23 08:57:18 PDT
Turns out we already walk all mjit scripts in JSCompartment::sweep, so this doesn't even cost much extra.
Comment 3 Jason Orendorff [:jorendorff] 2011-06-23 10:33:11 PDT
http://hg.mozilla.org/users/jblandy_mozilla.com/jsdbg2/rev/2d2654fc31b2

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.
Comment 4 Jason Orendorff [:jorendorff] 2011-08-16 08:08:07 PDT
This is obsolete. Comment 3 is too vague to be useful, and the actual crash was fixed long ago.

Note You need to log in before you can comment on or make changes to this bug.