461801 - Pass eval script containing function through JS debug API so that upvars work in debugger

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Graydon Hoare :graydon]

If the JS debug API places a trap inside an eval script that uses upvars, the hook winds up running with a jsd context that doesn't know about the function that invoked the eval code. Subsequent attempts to, say, decompile the eval code will therefore crash on a null function. We need to propagate the enclosing function through to the jsd context.

This can be triggered on trunk builds by enabling firebug's "script" monitoring and running the sunspider suite. The tofte test (which contains an eval-upvar combo) crashes reliably in the decompiler. A test patch that disables the emitting of upvars "fixes" this, but obviously in an unsatisfying way.

Comment 1

[Graydon Hoare :graydon]

Attachment: initial solution to bug

As per Brendan's suggestion, this patch prepends the caller frame's JSFunction* into the script's object vector (well, the code generator's objectList) when compiling a COMPILE_N_GO script in a non-null frame. So the function becomes "object 0" in such a script. 

The patch further modifies the decomplier to handle the case of a GETUPVAR opcode in a script lacking a function by loading object 0 from the script and loading a local name vector from that function, overwriting the missing bits in the JSPrinter, essentially "attaching" it to the script's function and local name vector on the fly.

The approach works -- I can get through tofte with firebug turned on now -- but I get the feeling I'm playing with fire in this patch, and should probably be providing a lot more assertions and possibly narrowing the cases handled, both on the parser side and the decompiler side. Possibly also a save/restore of the JSPrinter fields I'm writing to in the decompiler. Any suggestions?

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 345194 [details] [diff] [review]
initial solution to bug

>diff -r b0d41235146a js/src/jsopcode.cpp
>--- a/js/src/jsopcode.cpp	Mon Oct 27 18:16:15 2008 -0700
>+++ b/js/src/jsopcode.cpp	Tue Oct 28 16:26:36 2008 -0700
>@@ -2723,6 +2723,13 @@
> 
>               case JSOP_CALLUPVAR:
>               case JSOP_GETUPVAR:
>+
>+                if (!jp->fun)
>+                    JS_GET_SCRIPT_FUNCTION(jp->script, 0, jp->fun);
>+
>+                if (!jp->localNames)
>+                    jp->localNames = js_GetLocalNameArray(cx, jp->fun, &jp->pool);
>+

This seems sound and valid.

>                 i = JS_UPVAR_LOCAL_NAME_START(jp->fun) + GET_UINT16(pc);
>                 if (i >= JS_GET_LOCAL_NAME_COUNT(jp->fun)) {
>                     JSStackFrame *fp;
>@@ -2738,13 +2745,15 @@
>                      * called with an intervening frame on the stack.
>                      */
>                     fp = cx->fp;
>-                    while (!(fp->flags & JSFRAME_EVAL))
>-                        fp = fp->down;
>-                    JS_ASSERT(fp->script == jp->script);
>-                    JS_ASSERT(fp->down->fun == jp->fun);
>-                    JS_ASSERT(FUN_INTERPRETED(jp->fun));
>-                    JS_ASSERT(jp->script != jp->fun->u.i.script);
>-                    JS_ASSERT(jp->script->upvarsOffset != 0);
>+                    if (fp) {
>+                        while (!(fp->flags & JSFRAME_EVAL))
>+                            fp = fp->down;
>+                        JS_ASSERT(fp->script == jp->script);
>+                        JS_ASSERT(fp->down->fun == jp->fun);
>+                        JS_ASSERT(FUN_INTERPRETED(jp->fun));
>+                        JS_ASSERT(jp->script != jp->fun->u.i.script);
>+                        JS_ASSERT(jp->script->upvarsOffset != 0);
>+                    }

All this stuff (including the JSStackFrame *fp decl, which you could move down and initialize) should be #ifdef DEBUG -- my bad, please clean up. Thanks.

> 
>                     uva = JS_SCRIPT_UPVARS(jp->script);
>                     i = GET_UINT16(pc);
>diff -r b0d41235146a js/src/jsparse.cpp
>--- a/js/src/jsparse.cpp	Mon Oct 27 18:16:15 2008 -0700
>+++ b/js/src/jsparse.cpp	Tue Oct 28 16:26:36 2008 -0700
>@@ -543,6 +543,19 @@
>     cg.treeContext.u.scopeChain = scopeChain;
>     cg.staticDepth = TCF_GET_STATIC_DEPTH(tcflags);
> 
>+    if (callerFrame && (tcflags & TCF_COMPILE_N_GO)) {
>+        /* 
>+         * An eval script in a caller frame needs to have its enclosing function 
>+         * captured in case it uses an upvar reference, and someone wishes to 

Nit: trailing whitespace (didn't check elsewhere, but the line above ends with a space).

Nit: s/uses/contains/

>+         * decompile it while running.
>+         */
>+        JSParsedObjectBox *pob;
>+        pob = js_NewParsedObjectBox(cx, &pc, FUN_OBJECT(callerFrame->fun));
>+        pob->emitLink = cg.objectList.lastPob;
>+        cg.objectList.lastPob = pob;
>+        cg.objectList.length++;
>+    }
>+

I call this a fix. The only thought to robusticate it is the one mentioned on IRC: add TCF_EVAL_SCRIPT and use that instead of TCF_COMPILE_N_GO.

/be

Comment 3

[Graydon Hoare :graydon]

Ok. I'm leaning toward *not* using the TCF_EVAL since the decision to emit GETUPVAR appears to be predicated on COMPILE_N_GO and it feels like needless churn. Unless you think there are a lot of COMPILE_N_GO contexts that are *not* eval contexts, that this would be wasting space in?

Comment 4

[Brendan Eich [:brendan]]

Yes, I plus'ed the patch as written. Agree on not splitting flags for no reason. There are non-eval COMPILE_N_GO compilations but these get UPVARs too as you say. So the patch is good to go.

/be

Comment 5

[Graydon Hoare :graydon]

Pushed to tracemonkey repository as revision cf583edfcbbd. Possibly a mistake? On second thought this is affecting m-c too. Apologies, have come to think of tm as "js change staging repository". Bad habit.

Comment 6

[Brendan Eich [:brendan]]

TM will go into m-c today so not to worry. The staging area aspect can be winning, given incomplete test coverage. The downside is failure to sync often enough.

Sorry to say I should have spotted a problem during review, patched here:

http://pastebin.mozilla.org/564044

(good for a day). I'll file a followup bug.

/be

Comment 7

[Graydon Hoare :graydon]

> Sorry to say I should have spotted a problem during review, patched here:
> 
> http://pastebin.mozilla.org/564044

Pushed as "followup correctness fix" with same bug number, as per IRC discussion. Tracemonkey revision 57a81a3ffd37.