Boris Zbarsky [:bzbarsky] Reporter
	Description • 16 years ago

This is the testcase from bug 460050 comment 70.  Output, with current tip m-c opt shell:

mozilla% ./js/src/opt-obj/js ~/bar.js
starting init() 
test 2 (findChainTouching...) run time: 186
test 3 (both) run time: 795
mozilla% ./js/src/opt-obj/js -j ~/bar.js
starting init() 
test 2 (findChainTouching...) run time: 87
/Users/bzbarsky/bar.js:140: TypeError: counts[chainId] is undefined

I get similar behavior in a debug shell.

	Andreas Gal :gal Assignee
	Comment 9 • 16 years ago

The callee gets burned into the FrameInfo, so when we restore from within the second closure, we restore the wrong callee.

    fi->callee = JSVAL_TO_OBJECT(fval);
    fi->block = fp->blockChain;
    fi->pc = fp->regs->pc;

	Brendan Eich [:brendan]
	Comment 10 • 16 years ago

Why is callee in FrameInfo at all, when it's on the native stack and tracked by the tracker? If we need to synthesize frames by restoring the correct callee, why can't we get that object ref from the native stack?

/be

	Aaron McBride
	Comment 11 • 16 years ago

I didn't see the problem using the first 17 line version on my build (changeset:  28465:c5c18dde8bb9 Mac), so I tried re-reducing starting at the 43 line version.

To me, it looks like the counts array that's inside the anonymous function (lines 23-24) isn't the same array as on the outside (lines 26-27).

Another weird thing is that line 8 does something.  If I comment it out using strict mode and JIT, I get: strict warning: reference to undefined property counts[i.chainId].

	Aaron McBride
	Comment 12 • 16 years ago

Here's the output that I get running this without and with JIT:
--start--
Mac-mini:GoBug aaron$ ~/mozilla/js/src/js -s -f 17b_lines_am.js 
in: 1
in: 1
in: 1
out: 1
in: 2
in: 2
in: 2
out: 2
Mac-mini:GoBug aaron$ ~/mozilla/js/src/js -s -j -f 17b_lines_am.js 
in: 1
in: 1
in: 1
out: 1
in: 1
in: 1
in: 1
out: 2
--end--

	Brendan Eich [:brendan]
	Comment 14 • 16 years ago

Comment on attachment 378251 [details] [diff] [review]
patch

>@@ -3529,17 +3535,17 @@ js_SynthesizeFrame(JSContext* cx, const 
>     /* Claim space for the stack frame and initialize it. */
>     JSInlineFrame* newifp = (JSInlineFrame *) newsp;
>     newsp += nframeslots;
> 
>     newifp->frame.callobj = NULL;
>     newifp->frame.argsobj = NULL;
>     newifp->frame.varobj = NULL;
>     newifp->frame.script = script;
>-    newifp->frame.callee = fi.callee;
>+    newifp->frame.callee = fi.callee; /* Roll with a potential wrong callee for now. */

s/potential/&ly/

Could cite bug 471425 here with a FIXME comment. r=me with that. Worse is better!

/be

	Brendan Eich [:brendan]
	Comment 15 • 16 years ago

Comment on attachment 378251 [details] [diff] [review]
patch

Whoops, just noticed that the synthesized frame's scopeChain was computed from the wrong callee, and that will make for bad security and scope bugs!

/be

	Brendan Eich [:brendan]
	Comment 18 • 16 years ago

Comment on attachment 378255 [details] [diff] [review]
v2, I wish I could attach patches right

>+    newifp->frame.callee = fi.callee; /* Roll with a potential wrong callee for now. */

Use // style comment here, as you did later?

>-    newifp->frame.scopeChain = OBJ_GET_PARENT(cx, fi.callee);
>+    newifp->frame.scopeChain = NULL; // will be updated in FlushNativeStackFrame

...

>@@ -4587,34 +4596,34 @@ LeaveTree(InterpState& state, VMSideExit
>         JS_ASSERT(ti->nGlobalTypes() > innermost->numGlobalSlots);
>         globalTypeMap = (uint8*)alloca(ngslots * sizeof(uint8));
>         memcpy(globalTypeMap, getGlobalTypeMap(innermost), innermost->numGlobalSlots);
>         memcpy(globalTypeMap + innermost->numGlobalSlots,
>                ti->globalTypeMap() + innermost->numGlobalSlots,
>                ti->nGlobalTypes() - innermost->numGlobalSlots);
>     }
> 
>+    /* write back native stack frame */
>+#ifdef DEBUG
>+    int slots =
>+#endif
>+        FlushNativeStackFrame(cx, innermost->calldepth,
>+                              getStackTypeMap(innermost),
>+                              stack, NULL);
>+    JS_ASSERT(unsigned(slots) == innermost->numStackSlots);
>+
>+    if (innermost->nativeCalleeWord)
>+        SynthesizeSlowNativeFrame(cx, innermost);
>+
>     /* write back interned globals */
>     double* global = (double*)(&state + 1);
>     FlushNativeGlobalFrame(cx, ngslots, gslots, globalTypeMap, global);
>     JS_ASSERT(*(uint64*)&global[STOBJ_NSLOTS(JS_GetGlobalForObject(cx, cx->fp->scopeChain))] ==
>               0xdeadbeefdeadbeefLL);
> 
>-    /* write back native stack frame */
>-#ifdef DEBUG
>-    int slots =
>-#endif
>-        FlushNativeStackFrame(cx, innermost->calldepth,
>-                              getStackTypeMap(innermost),
>-                              stack, NULL);
>-    JS_ASSERT(unsigned(slots) == innermost->numStackSlots);
>-
>-    if (innermost->nativeCalleeWord)
>-        SynthesizeSlowNativeFrame(cx, innermost);
>-

Comment the reason for doing FlushNativeStackFrame before FlushNativeGlobalFrame.

Pre-existing nit: shouldn't the former by plural-Frames?

/be

	Aaron McBride
	Comment 19 • 16 years ago

I applied the patch from comment 13 to changeset: 28527:aac8141341bf.

The first testcaes "The testcase" gives me these results:
Mac-mini:GoBug aaron$ ~/mozilla/js/src/js -f the_testcase.js 
starting init() 
test 2 (findChainTouching...) run time: 263
test 3 (both) run time: 1461
Mac-mini:GoBug aaron$ ~/mozilla/js/src/js -j -f the_testcase.js 
starting init() 
test 2 (findChainTouching...) run time: 120
test 3 (both) run time: 1033

It looks like the original error is gone.

The following test cases now produce the same output with or without JIT.
 reduced testcase
 17 lines   (274 bytes)
 17 lines   (256 bytes)
 38 lines based on 43 lines
 15 lines based on the second 17 lines testcase

	Andreas Gal :gal Assignee
	Comment 20 • 16 years ago

Words are not enough to express how much we want to block on this. Bad correctness bug.

	Andreas Gal :gal Assignee
	Comment 23 • 16 years ago

Both try server runs died for unclear reasons (timeout/infrastructure problems I believe). Maybe we get more lucky on the actual tree. I don't want to wait another day.

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

	Andreas Gal :gal Assignee
	Comment 24 • 16 years ago

backed out

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

	Andreas Gal :gal Assignee
	Comment 25 • 16 years ago

re-landed

http://hg.mozilla.org/tracemonkey/rev/8f6c242a75ff

	Andreas Gal :gal Assignee
	Comment 26 • 16 years ago

backed out again

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

	Andreas Gal :gal Assignee
	Comment 27 • 16 years ago

Fired off a try server build that flips around the order in which stacks and globals are written back, which this patch does. I want to see whether that has any side effects.

	Andreas Gal :gal Assignee
	Comment 28 • 16 years ago

Just flipping around the reconstruction code seems to have passed the try server.

	Andreas Gal :gal Assignee
	Comment 29 • 16 years ago

Only set scopeChain to parent(callee) if we actually synthesized that frame, otherwise we might trample over some with object or similar monstrosities.

	Andreas Gal :gal Assignee
	Comment 30 • 16 years ago

Passed try server. This better sticks this time since its 4am and I really don't feel like backing it out again.

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

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 31 • 16 years ago

Looks like it stuck, woo! Onward to m-c and 1.9.1!

	Robert Sayre
	Comment 32 • 16 years ago

http://hg.mozilla.org/mozilla-central/rev/ca3db3beaa41

	Robert Sayre
	Comment 33 • 16 years ago

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/554965ebd0f1

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 34 • 16 years ago

Verified fixed by running the xpshell test with the following debug builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre)
Gecko/20090522 Minefield/3.6a1pre ID:20090522133810

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre)
Gecko/20090522 Shiretoko/3.5pre ID:20090522153422