687966 - Use JSScript, not script objects, as keys in Debugger maps

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

This is another leftover from the bug 674251. The Debugger still uses script object, not script itself, as keys in its weak map for held scripts or when checking for live scripts. For the bug 684529 this should be fixed.

Comment 1

[Igor Bukanov]

Attachment: v1

The patch merges Debugger::(held|nonHeld)Scripts into single Debugger::scripts with JSScript as keys and changes the code to use IsAboutToBeFinalized() on JSScript instances, not script objects. This allowed to eliminate all the code related to non-held support.

The patch still uses the script object in the debugger. Now it became effectively a way to get the global object associated with the script. Removal of that is for the bug 684529.

Comment 2

[Igor Bukanov]

Jason: review ping?

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 561596 [details] [diff] [review]
v1

Great simplification. I've been looking forward to this. Thank
you.

>     if (xdr->mode == JSXDR_DECODE) {
>+        JS_ASSERT(!script->compileAndGo);

Thanks for adding this.

In vm/Debugger.cpp, Debugger::init:
>     bool ok = (frames.init() &&
>                objects.init() &&
>                debuggees.init() &&
>-               heldScripts.init() &&
>-               nonHeldScripts.init());
>+               scripts.init());

While you're in here, could you please kill the extra parentheses around
the right-hand side? House style doesn't overparenthesize here. (Mea
culpa. Old Python habit, reinforced by emacs's auto-indentation.)

>+Debugger::slowPathOnNewScript(JSContext *cx, JSScript *script, JSObject *obj,
>+                              GlobalObject *compileAndGoGlobal)
> {

Here please JS_ASSERT(script->compileAndGo == !!compileAndGoGlobal);

Assertions that document the contract are good.

>+ * JSScripts' lifetimes is managed by the garbage collector. Debugger.Script
>+ * instances for scripts are strong references to the script.
>+ * Debugger::scripts weakly maps debuggee scripts to the Debugger.Script
>+ * objects. For a dead script its entry in Debugger::heldScripts will be
>+ * cleaned up by the standard weak table code.
>  */

You can just delete this whole comment. All this stuff is either pretty obvious or already said in comments elsewhere.

If you keep it, change heldScripts to scripts.

In DebuggerScript_check:
>     /*
>      * Check for Debugger.Script.prototype, which is of class DebuggerScript_class
>      * but whose holding object is undefined.
>      */
>     if (thisobj->getReservedSlot(JSSLOT_DEBUGSCRIPT_HOLDER).isUndefined()) {
>         JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_INCOMPATIBLE_PROTO,
>                              clsname, fnname, "prototype object");
>         return NULL;
>     }

Please change this to check that GetScriptReferent(thisobj) is NULL
instead of checking the holder slot, and change the comment to say
"...which is of class DebuggerScript_class but has no referent."

>+DebuggerScript_getChildScripts(JSContext *cx, uintN argc, Value *vp)
> {
>     THIS_DEBUGSCRIPT_SCRIPT(cx, argc, vp, "get live", args, obj, script);

Another jorendorff typo: the string here should be "getChildScripts", not "get live".

> DebuggerScript_getLineOffsets(JSContext *cx, uintN argc, Value *vp)
> {
>-    THIS_DEBUGSCRIPT_LIVE_SCRIPT(cx, argc, vp, "getAllOffsets", args, obj, script);
>+    THIS_DEBUGSCRIPT_SCRIPT(cx, argc, vp, "getAllOffsets", args, obj, script);

Same here.

> DebuggerScript_clearAllBreakpoints(JSContext *cx, uintN argc, Value *vp)
> {
>-    THIS_DEBUGSCRIPT_LIVE_SCRIPT(cx, argc, vp, "clearBreakpoint", args, obj, script);
>+    THIS_DEBUGSCRIPT_SCRIPT(cx, argc, vp, "clearBreakpoint", args, obj, script);

And here.

In vm/Debugger.h:
>      * The holder object for script, if known, else NULL.  This is NULL for
>-     * non-held scripts and for JSD1 traps. It is always non-null for JSD2
>+     * cached eval scripts and for JSD1 traps. It is always non-null for JSD2
>      * breakpoints in held scripts.

"held scripts" should just say "other scripts" I guess.

Obviously we want to get rid of JSSLOT_DEBUGSCRIPT_HOLDER and
GetScriptHolder; do you have that in a later patch? It has to be done
before Script objects can be killed off.

It will be pretty easy. With the DebuggerScript_check change I requested
above, the only use I see is in setBreakpoint, to check that the script
is actually "observed" (i.e. we're debugging that global). We could keep
the global around instead of the Script object / uncloned Function
object.

Comment 4

[Igor Bukanov]

(In reply to Jason Orendorff [:jorendorff] from comment #3)
> In DebuggerScript_check:
> >     /*
> >      * Check for Debugger.Script.prototype, which is of class DebuggerScript_class
> >      * but whose holding object is undefined.
> >      */
> >     if (thisobj->getReservedSl ot(JSSLOT_DEBUGSCRIPT_HOLDER).isUndefined()) {
> >         JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_INCOMPATIBLE_PROTO,
> >                              clsname, fnname, "prototype object");
> >         return NULL;
> >     }
> 
> Please change this to check that GetScriptReferent(thisobj) is NULL
> instead of checking the holder slot, and change the comment to say
> "...which is of class DebuggerScript_class but has no referent."

This is done in the patch for bug 684529.

Comment 5

[Igor Bukanov]

http://hg.mozilla.org/integration/mozilla-inbound/rev/83df5d290530

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/83df5d290530