488816 - TM: Crash [@ 0x000fdf5b]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

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

eval("\
  (function(){\
    for(y in [{},{},{}])\
    ((function(){\
      for(let y in [0])\
      y = this\
    })())\
  })()\
");

crashes opt and debug js shells with -j at a scary location (setting security-sensitive).

autoBisect shows this is probably related to bug 488203 :

The first bad revision is:
changeset:   27199:bf37cd061e3c
user:        Andreas Gal
date:        Tue Apr 14 19:52:09 2009 -0700
summary:     Properly compute 'this' object on trace and wrap if necessary (488203, r=mrbkap).

===

Exception Type:  EXC_BAD_ACCESS (SIGBUS)
Exception Codes: KERN_PROTECTION_FAILURE at 0x0000000000000000
Crashed Thread:  0

Thread 0 Crashed:
0   ???                           	0x000fdf5b 0 + 1040219
1   ???                           	0xbfffed98 0 + 3221220760
2   js-opt-tm-intelmac            	0x000a825b js_RecordLoopEdge(JSContext*, TraceRecorder*, unsigned int&) + 519
3   js-opt-tm-intelmac            	0x000a836e js_MonitorLoopEdge(JSContext*, unsigned int&) + 58
4   js-opt-tm-intelmac            	0x0003ecef js_Interpret + 43919
5   js-opt-tm-intelmac            	0x00043796 js_Execute + 572
6   js-opt-tm-intelmac            	0x0004f121 obj_eval(JSContext*, JSObject*, unsigned int, long*, long*) + 1911
7   js-opt-tm-intelmac            	0x00043f81 js_Invoke + 1719
8   js-opt-tm-intelmac            	0x0003479e js_Interpret + 1598
9   js-opt-tm-intelmac            	0x00043796 js_Execute + 572
10  js-opt-tm-intelmac            	0x0000c90a JS_ExecuteScript + 60
11  js-opt-tm-intelmac            	0x00004257 Process(JSContext*, JSObject*, char*, int) + 1559
12  js-opt-tm-intelmac            	0x0000680f main + 863
13  js-opt-tm-intelmac            	0x0000210b _start + 209
14  js-opt-tm-intelmac            	0x00002039 start + 41

Comment 1

[Andreas Gal :gal]

Confirmed with TM tip. Good test case + reduction. Great work Gary.

Comment 2

[Andreas Gal :gal]

Attachment: patch

Comment 3

[Andreas Gal :gal]

Attachment: slightly less broken english in the comments

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 373415 [details] [diff] [review]
slightly less broken english in the comments

>+    /*
>+     * In global code, bake in the global object as 'this' object.
>+     */
>+    if (!cx->fp->callee) {
>         JS_ASSERT(callDepth == 0);
>+        JSObject* thisObj = js_ComputeThisForFrame(cx, cx->fp);
>+        if (!thisObj)
>+            ABORT_TRACE("js_ComputeThis failed");
>+        JS_ASSERT(JSVAL_IS_OBJECT(cx->fp->argv[-1]));
>         this_ins = INS_CONSTPTR(thisObj);

Blank line here.

>+        /*
>+         * We don't have argv[-1] in global code, so we don't update the tracker here.
>+         */
>+        return true;
>+    }

Blank line here.

>+    /*
>+     * Traces type-specialize between null and objects, so if we currently see a null
>+     * value in argv[-1], this trace will only match if we see null at runtime as well.
>+     * Bake in the global object as 'this' object, updating the tracker as well. We
>+     * can only detect this condition prior to calling js_ComputeThisForFrame, since it
>+     * updates the interpreter's copy of argv[-1].
>+     */
>+    if (JSVAL_IS_NULL(cx->fp->argv[-1])) {
>+        JSObject* thisObj = js_ComputeThisForFrame(cx, cx->fp);
>+        if (!thisObj)
>+            ABORT_TRACE("js_ComputeThis failed");

Where's that bug on propagating errors to the interpreter? Graydon mentioned the control flow graph complexity of jstracer.cpp being very high, when looking into the general problem.

>+        JS_ASSERT(JSVAL_IS_OBJECT(cx->fp->argv[-1]));

You want to exclude null here, so !JSVAL_IS_PRIMITIVE(cx->fp->argv[-1]).

>+        JS_ASSERT(thisObj == globalObj);
>+        this_ins = INS_CONSTPTR(thisObj);
>+        set(&cx->fp->argv[-1], this_ins);
>         return true;
>     }
>     this_ins = get(&cx->fp->argv[-1]);

Blank line here.

/be

Comment 5

[Andreas Gal :gal]

Attachment: v3

Comment 6

[Andreas Gal :gal]

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

Please remove security flag. The underlying patch never made it into trunk.

Comment 7

[Robert Sayre]

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

Comment 8

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e0e2c0525fb8

Comment 9

[Christian Holler (:decoder)]

Bug in removed tracer code, setting in-testsuite- flag.