493657 - TM: Wrong callee is restored when side-exiting from a trace

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: The testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: With smaller iteration counts; these are the minimal ones that show the bug

Comment 3

[Andreas Gal :gal]

Attachment: reduced testcase

Comment 4

[Andreas Gal :gal]

Attachment: further reduced with output

Comment 5

[Jesse Ruderman]

Attachment: reduced to 43 lines

Comment 6

[Andreas Gal :gal]

Attachment: 25 lines

Comment 7

[Andreas Gal :gal]

Attachment: 17 lines

Comment 8

[Andreas Gal :gal]

Attachment: 17 lines

Comment 9

[Andreas Gal :gal]

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;

Comment 10

[Brendan Eich [:brendan]]

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

Comment 11

[Aaron McBride]

Attachment: 38 lines based on 43 lines

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].

Comment 12

[Aaron McBride]

Attachment: 15 lines based on the second 17 lines testcase

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--

Comment 13

[Andreas Gal :gal]

Attachment: patch

Comment 14

[Brendan Eich [:brendan]]

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

Comment 15

[Brendan Eich [:brendan]]

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

Comment 16

[Andreas Gal :gal]

Attachment: v2

Comment 17

[Andreas Gal :gal]

Attachment: v2, I wish I could attach patches right

Comment 18

[Brendan Eich [:brendan]]

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

Comment 19

[Aaron McBride]

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

Comment 20

[Andreas Gal :gal]

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

Comment 23

[Andreas Gal :gal]

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

Comment 24

[Andreas Gal :gal]

backed out

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

Comment 25

[Andreas Gal :gal]

re-landed

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

Comment 26

[Andreas Gal :gal]

backed out again

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

Comment 27

[Andreas Gal :gal]

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.

Comment 28

[Andreas Gal :gal]

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

Comment 29

[Andreas Gal :gal]

Attachment: v3

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

Comment 30

[Andreas Gal :gal]

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

Comment 31

[Mike Shaver (:shaver emeritus)]

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

Comment 32

[Robert Sayre]

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

Comment 33

[Robert Sayre]

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

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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