576847 - "Assertion failure: slot < StackDepth(jp->script),"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

eval("function f(){}(((f)for(x in function(){}))())")

asserts js debug shell on TM tip without -j at Assertion failure: slot < StackDepth(jp->script), at ../jsopcode.cpp:1376

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000000
0x0014fb5d in JS_Assert (s=0x1ef8d3 "slot < StackDepth(jp->script)", file=0x1ef8c3 "../jsopcode.cpp", ln=1376) at ../jsutil.cpp:77
77          *((int *) NULL) = 0;  /* To continue from here in GDB: "return" then "continue". */
(gdb) bt
#0  0x0014fb5d in JS_Assert (s=0x1ef8d3 "slot < StackDepth(jp->script)", file=0x1ef8c3 "../jsopcode.cpp", ln=1376) at ../jsutil.cpp:77
#1  0x000d58f2 in IsVarSlot (jp=0x40c5b0, pc=0x40c7f8 "V####L\b#####, indexp=0xbfffd8a4) at ../jsopcode.cpp:1376
#2  0x000dbef3 in Decompile (ss=0xbfffe0c8, pc=0x40c7f8 "V####L\b#####, nb=5, nextop=JSOP_NOP) at ../jsopcode.cpp:2890
#3  0x000dd0e1 in Decompile (ss=0xbfffe0c8, pc=0x40c7f1 "\006", nb=30, nextop=JSOP_NOP) at ../jsopcode.cpp:3173
#4  0x000e21d0 in Decompile (ss=0xbfffe374, pc=0x40c98d "#", nb=7, nextop=JSOP_NOP) at ../jsopcode.cpp:4101
#5  0x000e59d0 in DecompileCode (jp=0x40c5b0, script=0x40c940, pc=0x40c98d "#", len=7, pcdepth=0) at ../jsopcode.cpp:4885
#6  0x000e5eec in DecompileExpression (cx=0x809200, script=0x40c940, fun=0x0, pc=0x40c991 ":") at ../jsopcode.cpp:5351
#7  0x000e62a9 in js_DecompileValueGenerator (cx=0x809200, spindex=-2, v=16785760, fallback=0x0) at ../jsopcode.cpp:5212
#8  0x00039c6d in js_ReportValueErrorFlags (cx=0x809200, flags=0, errorNumber=22, spindex=-2, v=16785760, fallback=0x0, arg1=0x0, arg2=0x0) at ../jscntxt.cpp:1849
#9  0x0006cad4 in js_ReportIsNotFunction (cx=0x809200, vp=0x500158, flags=0) at ../jsfun.cpp:2636
#10 0x000bfad4 in js_Call (cx=0x809200, argc=0, vp=0x500158) at ../jsobj.cpp:5653
#11 0x000afc7e in js::callJSFastNative (cx=0x809200, native=0xbfa23 <js_Call>, argc=0, vp=0x500158) at jscntxtinlines.h:349
#12 0x000ab8f8 in callJSNative (cx=0x809200, callOp=0xbfa23 <js_Call>, thisp=0x0, argc=0, argv=0x500160, rval=0x5001a4) at jsinterp.cpp:464
#13 0x000abe42 in Invoke<int (*)(JSContext*, unsigned int, long*)> (cx=0x809200, fun=0x0, script=0x0, native=0xbfa23 <js_Call>, args=@0xbfffeafc, flags=0) at jsinterp.cpp:591
#14 0x000aee73 in js_Invoke (cx=0x809200, args=@0xbfffeafc, flags=0) at jsinterp.cpp:722
#15 0x0009c6a5 in js_Interpret (cx=0x809200) at jsops.cpp:2155
#16 0x000ae3a3 in js_Execute (cx=0x809200, chain=0x1002000, script=0x40c940, down=0x500098, flags=16, result=0x5000e0) at jsinterp.cpp:891
#17 0x000cc64c in obj_eval () at ../jsobj.cpp:1305
#18 0x0009c537 in js_Interpret (cx=0x809200) at jsops.cpp:2145
#19 0x000ae3a3 in js_Execute (cx=0x809200, chain=0x1002000, script=0x40c810, down=0x0, flags=0, result=0xbffff738) at jsinterp.cpp:891
#20 0x00015f79 in JS_ExecuteScript (cx=0x809200, obj=0x1002000, script=0x40c810, rval=0xbffff738) at ../jsapi.cpp:4751
#21 0x000099a5 in Process (cx=0x809200, obj=0x1002000, filename=0x0, forceTTY=0) at ../../shell/js.cpp:522
#22 0x0000a369 in ProcessArgs (cx=0x809200, obj=0x1002000, argv=0xbffff908, argc=0) at ../../shell/js.cpp:843
#23 0x0000a482 in shell (cx=0x809200, argc=0, argv=0xbffff908, envp=0xbffff90c) at ../../shell/js.cpp:5025
#24 0x0000a5a6 in main (argc=0, argv=0xbffff908, envp=0xbffff90c) at ../../shell/js.cpp:5112

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   27205:78a21b8efe1b
user:        Brendan Eich
date:        Wed Apr 15 01:57:13 2009 -0700
summary:     Bug 488015 - Crash [@ js_GetUpvar ] (also bogus JS errors, also probably Crash [@js_Interpret]) (future r=mrbkap, see bug).

Comment 2

[Luke Wagner [:luke]]

There appear to be two bugs.  One is that CompExprTransplanter::transplant uses pn_atom on a JSDefinition (instead of pn_funbox).  The second is that JSDefinitions have their funbox's freed at the end of every top-level statement, which ends up setting pn_funbox to null.

Comment 3

[Luke Wagner [:luke]]

Attachment: fix, perhaps not the best

This patch fixes the two issues described for the (reduced) test case.  I'm not sure the placement of the guard for not recycling definitions is ideal or complete, though.  E.g., instead of not calling RecycleTree for defns, could we just remove the call to UnlinkFunctionBox from RecycleFuncNameKids.  Also, is this the only place we need to watch out for recycling definitions when we oughtn't?  Feel free to steal if you have better ideas.

Comment 5

[Luke Wagner [:luke]]

feedback ping.  Also, feel free to steal since you would have a lot better idea of the proper pinch points for Unlink/Recycle calls.

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 492709 [details] [diff] [review]
fix, perhaps not the best

Jim: Brendan said you had recently come into the know in this area.  The reason this is f? and not r? is that I'm not close to sure that my placement of "if (!pn->pn_defn)" is necessary or sufficient.  Please feel free to steal.

Comment 7

[Jim Blandy :jimb]

Comment on attachment 492709 [details] [diff] [review]
fix, perhaps not the best

>-        RecycleTree(pn, &cg);
>+
>+        if (!pn->pn_defn)
>+            RecycleTree(pn, &cg);

Why is this needed? It seems like, if pn->pn_defn is set, then RecycleTree will carefully recycle the node's children, but avoid recycling the node itself, which seems like the right thing. 


>                 if (dn->pn_pos < root->pn_pos || dn->isPlaceholder()) {
>-                    JSAtomListElement *ale = tc->lexdeps.add(tc->parser, dn->pn_atom);
>+                    JSAtom *funName = dn->pn_funbox->object->getFunctionPrivate()->atom;
>+                    JSAtomListElement *ale = tc->lexdeps.add(tc->parser, funName);

I don't understand what transplant is doing here, but I don't understand how we know that dn is a function definition, and not (say) an ordinary variable definition. Could we add:

    JS_ASSERT(dn->pn_arity == PN_FUNC)

before fetching dn->pn_funbox?

Comment 8

[Jim Blandy :jimb]

(In reply to comment #2)
> There appear to be two bugs.  One is that CompExprTransplanter::transplant uses
> pn_atom on a JSDefinition (instead of pn_funbox).  The second is that
> JSDefinitions have their funbox's freed at the end of every top-level
> statement, which ends up setting pn_funbox to null.

JSDefinitions are for any identifier definition (var x), not just for function definitions. You have to look at pn_arity to decide whether it's a NAME (in which case pn_atom is what you want) or a FUNC (in which case the funbox's object's grandmother's third cousin's atom is what you want).

Comment 9

[Luke Wagner [:luke]]

(In reply to comment #7)
> Comment on attachment 492709 [details] [diff] [review]
> fix, perhaps not the best
> 
> >-        RecycleTree(pn, &cg);
> >+
> >+        if (!pn->pn_defn)
> >+            RecycleTree(pn, &cg);
> 
> Why is this needed? It seems like, if pn->pn_defn is set, then RecycleTree will
> carefully recycle the node's children, but avoid recycling the node itself,
> which seems like the right thing. 

Its been a while since I gdb'ed this mrbkap, but IIRC, even though RecycleTree skips over the node, it mutates a field which is used later.

> >                 if (dn->pn_pos < root->pn_pos || dn->isPlaceholder()) {
> >-                    JSAtomListElement *ale = tc->lexdeps.add(tc->parser, dn->pn_atom);
> >+                    JSAtom *funName = dn->pn_funbox->object->getFunctionPrivate()->atom;
> >+                    JSAtomListElement *ale = tc->lexdeps.add(tc->parser, funName);
> 
> I don't understand what transplant is doing here, but I don't understand how we
> know that dn is a function definition, and not (say) an ordinary variable
> definition. Could we add:
> 
>     JS_ASSERT(dn->pn_arity == PN_FUNC)
> 
> before fetching dn->pn_funbox?

Go for it; I hardly understand this area, I just tracked down the apparent causes since it was unclaimed and a blocker.  It sounds like you know a lot more about the area so, as I propositioned Brendan above (who forwarded to you), please steal.

Comment 10

[Jim Blandy :jimb]

(In reply to comment #9)
> Go for it; I hardly understand this area, I just tracked down the apparent
> causes since it was unclaimed and a blocker.  It sounds like you know a lot
> more about the area so, as I propositioned Brendan above (who forwarded to
> you), please steal.

Okay --- stolen. This is indeed where I've been working the last few weeks.

Comment 11

[Jim Blandy :jimb]

Attachment: Remove unused arguments in generator expression parsing functions.

Some initial cleanup:

We always pass NULL as the |pn1| argument to js::Parser::parenExpr; remove
it. We pass NULL for the |genexp| argument in all but one case; give it a
default value. This allows almost all calls to pass no arguments.

We always pass a freshly allocated PN_UNARY node as |generatorExpr|'s first
argument, and never refer to the node again in the caller; move the
allocation into |generatorExpr| itself. This makes |generatorExpr| a
function that takes an expression |E| and returns the immediate application
of a generator function which consists of the nested loops and conditionals
given by the comprehension tail, with a |yield E| at the center.

Comment 12

[Jim Blandy :jimb]

Attachment: Propagate error returns out of CompExprTransplanter::transplant.

Comment 13

[Jim Blandy :jimb]

Attachment: Don't assume that definition nodes have atoms (functions don't); use the atom we find in the use node.

Needs some tests, but I think this is the right fix.

Comment 14

[Jim Blandy :jimb]

We also flunk this, with and without my patch:

assertEq(eval("function f() {}; var i = (f for (f in [1])); uneval([n for (n in i)])"),
         '["0"]');

Comment 15

[Jim Blandy :jimb]

Attachment: Don't assume that definition nodes have atoms (functions don't); use the atom we find in the use node. Make ordinary placeholder nodes.

This one fixes the original bug, and the one I found. Tests included.

Comment 16

[Jim Blandy :jimb]

I'm testing a patch that changes transplant to simply use MakePlaceholder, instead of repeating its contents in a customized way.

Comment 17

[Jim Blandy :jimb]

Attachment: Use the standard placeholder-making function when re-scoping variable references in generator 'yield' expressions.

CompExprTransplanter::transplant does a bunch of bespoke pointer-fiddling
to take the free variable references in the 'yield' expression and re-scope
them within the comprehension tail clauses. This code incorrectly
constructed placeholders for function references.

The previous patch corrects this, but it would also work to simply use the
same placeholder construction function everybody else does. This patch does
so.

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 505143 [details] [diff] [review]
Don't assume that definition nodes have atoms (functions don't); use the atom we find in the use node. Make ordinary placeholder nodes.

Agh, dumb old bug of mine from Fx3.5 era. Sorry, thanks for fixing.

/be

Comment 19

[Jim Blandy :jimb]

http://hg.mozilla.org/tracemonkey/rev/b07c1abab201

Comment 20

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/073f74dafec7
http://hg.mozilla.org/mozilla-central/rev/268548d24690
http://hg.mozilla.org/mozilla-central/rev/b07c1abab201

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 505168 [details] [diff] [review]
Use the standard placeholder-making function when re-scoping variable references in generator 'yield' expressions.

This is a bit nicer -- thanks. New bug for this patch?

/be

Comment 22

[Jim Blandy :jimb]

Filed as bug 627859.

Comment 23

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.