465214 - crash [@ js_FlushJITCache(JSContext*) ] trying to detach tab with jit.chrome turned on, or visiting walmart.com

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Steffen Wilberg]

Since bug 225680 landed, you can detach tabs by dragging them out of the tab bar, which will open a new window with that tab, preserving its session history.

But with javascript.options.jit.chrome set to true, I crash:
http://crash-stats.mozilla.com/report/index/c96034bc-0798-4f86-ba68-880720081116

Comment 1

[Steffen Wilberg]

Another way to hit this bug is to visit http://www.walmart.com/. This is independent from the jit.chrome pref, it crashes even with this set to false.

Comment 2

[Diego]

Moving to all/all according to:

http://crash-stats.mozilla.com/report/list?range_value=2&range_unit=weeks&version=Firefox%3A3.1b2pre&signature=js_FlushJITCache%28JSContext*%29

Comment 3

[Sylvain Pasche]

I can't reproduce the crash on Mac, but I found this regression range on Linux:

2008-11-15-02-mozilla-central ok
2008-11-16-02-mozilla-central crash

Comment 4

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

I had the same crash on Bugzilla some minutes ago while entering and selecting a new keyword (checkin-needed).

Here the crash report:
http://crash-stats.mozilla.com/report/index/d38b8525-8369-44f1-b7f3-773592081124

The first four frames are identical:
0  	js3250.dll  	js_FlushJITCache  	 js/src/jstracer.cpp:3984
1 	js3250.dll 	js_MonitorLoopEdge 	js/src/jstracer.cpp:3684
2 	js3250.dll 	js_Interpret 	js/src/jsinterp.cpp:3087
3 	js3250.dll 	js_Invoke 	js/src/jsinterp.cpp:1331

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Andreas, this is the apply bug that makes the js_ReconstructStackDepth assert fail.

Comment 6

[Andreas Gal :gal]

Attachment: partial patch, switching laptops

Comment 7

[Andreas Gal :gal]

Attachment: imacro file, hg queues sucks

Comment 8

[Andreas Gal :gal]

Attachment: v3

Comment 9

[Andreas Gal :gal]

The imacro code was generated with this script:

var stack;

function stk() {
    var x = "#";
    for (i in stack)
	x += " " + stack[i];
    return x;
}

function x(code, pop, push) {
    while (pop--)
	stack.pop();
    stack = stack.concat(push);
    print("    " + (code + "                                      ").slice(0,40) + stk());
}

function u(n) {
    var nn = ((n == 0) ? "zero" : ((n == 1) ? "one" : ("int8 " + n)));
    x("    dup", 0, ["arr"]); 
    x("    " + nn, 0, [n]);
    x("    getelem", 2, ["arg"+n]);
    x("    swap", 2, ["arg"+n, "arr"]);
}

print(".igroup apply JSOP_APPLY");
for (var args = 1; args <= 8; ++args) {
    stack = ["arr"];
    x(".imacro unpack" + args, 0, []);
    for (var n = 0; n < args; ++n)
	u(n);
    x("    pop", 1, []);
    x("    apply " + n, n, []);
    x("    stop", 0, []);
    x(".end", 0, []);
    print("");
}
print(".end");

Comment 10

[Andreas Gal :gal]

Attachment: with imacros we no longer need to stay on trace in the interpreter

Comment 11

[Andreas Gal :gal]

Attachment: v5

Comment 12

[Andreas Gal :gal]

Attachment: test coverage

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 353614 [details] [diff] [review]
test coverage

>+++ b/js/src/imacro_asm.js.in
>@@ -100,16 +100,18 @@ function immediate(op) {
>     let info = op.info;
>     if (info.flags.indexOf("JOF_ATOM") >= 0) {
>         if (/^(?:void|object|function|string|number|boolean)$/.test(op.imm1))
>             return "0, COMMON_TYPE_ATOM_INDEX(JSTYPE_" + op.imm1.toUpperCase() + ")";
>         return "0, COMMON_ATOM_INDEX(" + op.imm1 + ")";
>     }
>     if (info.flags.indexOf("JOF_JUMP") >= 0)
>         return ((op.target >> 8) & 0xff) + ", " + (op.target & 0xff);
>+    if (info.flags.indexOf("JOF_INT8") >= 0)
>+	return (op.imm1 & 0xff);

Nit: no tabs in files (indentation varying gives away the hard tab).

>+} apply_imacros = {
>+    {
>+/* 0*/  JSOP_PICK, 3,

Is this how Forth defines pick? The operand is one less than the distance from stack fencepost, i.e. the exact distance from topmost live stack item's index to the one picked?

>+/* 2*/  JSOP_POP,
>+/* 3*/  JSOP_POP,
>+/* 4*/  JSOP_CALL, 0, 0,
>+/* 7*/  JSOP_STOP,
>+    },
>+    {
>+/* 0*/  JSOP_PICK, 3,
>+/* 2*/  JSOP_POP,
>+/* 3*/  JSOP_DUP,
>+/* 4*/  JSOP_ZERO,
>+/* 5*/  JSOP_GETELEM,
>+/* 6*/  JSOP_SWAP,
>+/* 7*/  JSOP_POP,

SWAP;POP is a waste -- why was the value POPped needed at all?

In other words, why DUP the array here, we don't need it again (same for last GETELEM in inductive case)?

>+++ b/js/src/jsinterp.cpp
>@@ -3333,16 +3333,24 @@ js_Interpret(JSContext *cx)
. . .
>+          BEGIN_CASE(JSOP_PICK)
>+            i = jsuint(GET_INT8(regs.pc));

No need for jsuint cast here, but if you want to treat the immediate as a uint8, use JOF_UINT8 as its format and call GET_UINT8 here.

i is jsint, which we want (signed int for pointer arithmetic with sign extension).

>+            JS_ASSERT(regs.sp - (i+1) >= StackBase(fp));
>+            lval = regs.sp[-(i+1)];
>+            memcpy(regs.sp - (i+1), regs.sp - i, sizeof(jsval)*i);

Use memmove, as per memcpy(3), "If s1 and s2 overlap, behavior is undefined."

The repeated (i+1) suggests commoning via ++i; before first such expression, requiring regs.sp - (i - 1) in memmove's second arg. But why not make the immediate be distance from top of stack fencepost, not topmost value's index? Then no add is required, just the one subtract for memmove's second arg.

I think this makes the operand more intuitive too, but YMMV (my mental stack model has sp as fencepost, not topmost index).

>           BEGIN_CASE(JSOP_CALL)
>           BEGIN_CASE(JSOP_EVAL)
>+          BEGIN_CASE(JSOP_APPLY)

Hurray for code reduction!

> static inline jsdouble JS_FASTCALL
> math_ceil_kernel(jsdouble x)
> {
>-#if defined(XP_MACOSX) || defined(DARWIN)
>+#ifdef __APPLE__
>     if (x < 0 && x > -1.0) 
>         return js_copysign(0, -1);
> #endif    

Trailing space after #endif pre-existing nit.

Is this for another bug?

>+/*
>+ * Pick an element from the stack.
>+ */
>+OPDEF(JSOP_PICK,          201,"pick",        NULL,    2,  1,  0,  0,  JOF_INT8)

JOF_UINT8 and talk in the comment about the immediate counting from sp fencepost.

> JS_REQUIRES_STACK bool
>+TraceRecorder::record_JSOP_PICK()
>+{
>+    jsval* sp = cx->fp->regs->sp;
>+    jsint n = jsuint(GET_INT8(cx->fp->regs->pc));
>+    JS_ASSERT(sp - (n+1) >= StackBase(cx->fp));
>+    LIns* top = tracker.get(sp - (n+1));
>+    for (jsint i = 0; i < n; ++i)
>+        tracker.set(sp - (n+1) + i, tracker.get(sp - n + i));

Similar comments here to those for the interpreter case, although lack of tracker memmove makes it less of an issue to save an add.

>@@ -6574,173 +6588,122 @@ TraceRecorder::interpretedFunctionCall(j
> }
> 
> JS_REQUIRES_STACK bool
> TraceRecorder::record_JSOP_CALL()
> {
>     return functionCall(false, GET_ARGC(cx->fp->regs->pc));
> }
> 
>+static jsbytecode* apply_imacro_table[] = {
>+        apply_imacros.apply0,
>+        apply_imacros.apply1,
>+        apply_imacros.apply2,
>+        apply_imacros.apply3,
>+        apply_imacros.apply4,
>+        apply_imacros.apply5,
>+        apply_imacros.apply6,
>+        apply_imacros.apply7,
>+        apply_imacros.apply8
>+};
>+
>+static jsbytecode* call_imacro_table[] = {
>+        apply_imacros.call0,
>+        apply_imacros.call1,
>+        apply_imacros.call2,
>+        apply_imacros.call3,
>+        apply_imacros.call4,
>+        apply_imacros.call5,
>+        apply_imacros.call6,
>+        apply_imacros.call7,
>+        apply_imacros.call8
>+};
>+

Nit: indentation (c-basic-offset) is 4 not 8.

>-    /* 
>+    /*
>+     * We don't trace apply and call with a primitive this.

s/this/'this'/ and say something like "... with a primitive 'this' (which is the first positional parameter)."

>+     */
>+    if (argc > 0 && JSVAL_IS_PRIMITIVE(vp[2]))
>+        return record_JSOP_CALL();

See js_fun_apply and js_fun_call in jsfun.cpp -- both do this:

    /* Convert the first arg to 'this' and skip over it. */
    if (!JSVAL_IS_PRIMITIVE(vp[2]))
        obj = JSVAL_TO_OBJECT(vp[2]);
    else if (!js_ValueToObject(cx, vp[2], &obj))
        return JS_FALSE;

but keep on going, so I don't think it's right to turn apply and call with primitive first arg as calls.

>+    
>+    /*
>+     * Guard on the identity of this, which is the function we
..12345678901234567890123456789012345678901234567890123456789012345678901234567890
>+     * are applying.

Comment fits in 80 columns without wrapping. But to match previous comment cited above, use 'this' to set off the common pronoun a bit.

>+     */
>+    if (!guardCallee(vp[1]))
>+        return false;
>+
>+    /*
>      * If this is apply, and the argument is too long and would need
>-     * a separate stack chunk, do a heavy-weight apply. 
>+     * a separate stack chunk, we can't trace through the apply.

Is this comment out of date? We are guaranteed enough stack due to imacro_asm-generated js_opcode2extra budgeting.

Comment wrapping in vim via Qj or whatever respect tw=79 ;-). Last time for this nit, but it applies to comments below too.

>+        length = jsuint(aobj->fslots[JSSLOT_ARRAY_LENGTH]);
>+        
>+        /*
>+         * We only trace apply calls with a certain number of arguments.

s/only trace/trace only/

Maybe move length assignment down to below comment?

>+         */
>+        if (length > (sizeof(apply_imacro_table)/sizeof(jsbytecode*)+1))

Why the +1 hiding at the end, adding 1 to the division result?

Normally there'd be the ob. nit: uncrunch binary operators by putting a space on either side here -- also don't overparenthesize outer (+) expression. But better than nit-picking: use the standard JS_ARRAY_LENGTH macro.

>+            ABORT_TRACE("too many arguments to apply");
>+        
>         /*
>          * Make sure the array has the same length at runtime.
>          */
>-        length = jsuint(aobj->fslots[JSSLOT_ARRAY_LENGTH]);
>         guard(true, lir->ins2i(LIR_eq,
>                                stobj_get_fslot(aobj_ins, JSSLOT_ARRAY_LENGTH),
>                                length), 
>                                BRANCH_EXIT);

BRANCH_EXIT arg is misindented, because lir->ins2i(...) is misplaced: each overflow arg should start on its own line in the same column as the first (true) arg.

>+    if (argc > (sizeof(call_imacro_table)/sizeof(jsbytecode*)+1))

Ditto above questions and comments.

/be

Comment 14

[Andreas Gal :gal]

I will take care of the nits. I will pull out the ceil fix.

There is no GET_UINT8.

Comment 15

[Brendan Eich [:brendan]]

(In reply to comment #13)
> >+    if (argc > 0 && JSVAL_IS_PRIMITIVE(vp[2]))
> >+        return record_JSOP_CALL();
> 
> See js_fun_apply and js_fun_call in jsfun.cpp -- both do this:

Duh, I was confused -- Andreas set me straight on IRC. We want to call the js_fun_apply or js_fun_call native in this hard case.

No GET_UINT8? Well, casting GET_INT8 result to jsuint is still wrong, if anyone wanted to use 128 or greater as the immediate value. Just use regs.pc[1] ;-).

/be

Comment 16

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/95b210c2fc92

Comment 17

[Andreas Gal :gal]

This is the function we start tracing in. When coming out of that tree we incorrectly write back an Object as a JSString:

function () {
    this.lastError = null;
    var K = [], E = this.subscribers.length;
    if (!E && this.silent) {
        return true;
    }
    var I = [].slice.call(arguments, 0), G = true, D, J = false;
    if (!this.silent) {
    }
    var C = this.subscribers.slice(), A = YAHOO.util.Event.throwErrors;
    for (D = 0; D < E; ++D) {
        var M = C[D];
        if (!M) {
            J = true;
        } else {
            if (!this.silent) {
            }
            var L = M.getScope(this.scope);
            if (this.signature == YAHOO.util.CustomEvent.FLAT) {
                var B = null;
                if (I.length > 0) {
                    B = I[0];
                }
                try {
                    G = M.fn.call(L, B, M.obj);
                } catch (F) {
                    this.lastError = F;
                    if (A) {
                        throw F;
                    }
                }
            } else {
                try {
                    G = M.fn.call(L, this.type, I, M.obj);
                } catch (H) {
                    this.lastError = H;
                    if (A) {
                        throw H;
                    }
                }
            }
            if (false === G) {
                if (!this.silent) {
                }
                break;
            }
        }
    }
    return G !== false;
}

Comment 18

[Andreas Gal :gal]

The bailout we are botching in is one level down in this code:

function (O, N, R) {
    var T = N[0], S = this.mouseOutEvent, P = this.mouseOverEvent, Q = this.keyDownEvent;
    if (T > 0) {
        if (!this._bHideDelayEventHandlersAssigned) {
            S.subscribe(this._execHideDelay);
            P.subscribe(this._cancelHideDelay);
            Q.subscribe(this._cancelHideDelay);
            this._bHideDelayEventHandlersAssigned = true;
        }
    } else {
        S.unsubscribe(this._execHideDelay);
        P.unsubscribe(this._cancelHideDelay);
        Q.unsubscribe(this._cancelHideDelay);
        this._bHideDelayEventHandlersAssigned = false;
    }
}

I am pretty confident this is inside one of the call()s in the outer function.

Comment 19

[Andreas Gal :gal]

We are exiting when reading this.mouseOutEvent due to a shape mismatch. The incorrect value gets generated into stack2 as a string, instead of being an Object (potentially a Call Object).

Comment 20

[Andreas Gal :gal]

Reduced shell testcase:

function g(x,a,b,c) {
    x.f();
}

var x = [];
x.f = function() { }

var y = [];
y.f = function() { }

var z = [x,x,x,x,x,y,y,y,y,y];

for (var i = 0; i < 10; ++i)
    g.call(this, z[i], "", true, 1);

Comment 21

[Andreas Gal :gal]

function g(x,a) {
    x.f();
}

var x = [];
x.f = function() { }

var y = [];
y.f = function() { }

var z = [x,x,x,x,x,y,y,y,y,y];

for (var i = 0; i < 10; ++i)
    g.call(this, z[i], "");

Comment 22

[Andreas Gal :gal]

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

Comment 23

[Brendan Eich [:brendan]]

Comment on attachment 353614 [details] [diff] [review]
test coverage

>+TraceRecorder::record_JSOP_PICK()
>+{
>+    jsval* sp = cx->fp->regs->sp;
>+    jsint n = jsuint(GET_INT8(cx->fp->regs->pc));
>+    JS_ASSERT(sp - (n+1) >= StackBase(cx->fp));
>+    LIns* top = tracker.get(sp - (n+1));
>+    for (jsint i = 0; i < n; ++i)
>+        tracker.set(sp - (n+1) + i, tracker.get(sp - n + i));
>+    tracker.set(&sp[-1], top);

Should have seen this, we found it by debugging -- the calls must go through TraceRecorder::set, not tracker.set, in order to generate LIR.

We landed with that fix, but sayrer and graydon point to orange on Mac and Linux (some tests, maybe not too many).

It's clear too that the trace-test.js coverage here was not actually testing apply and call, since the above bug would result in JSOP_PICK resulting in no generated stack moves.

Need to sort all this out, possibly with back-out and retry. For now we'll try diagnose in place. Andreas is traveling today, so I will help.

/be

Comment 24

[Brendan Eich [:brendan]]

Fixed a few warnings I should have caught during review (if I had done a recorded review in this bug :-P).

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

/be

Comment 25

[Brendan Eich [:brendan]]

The orange was something pulled from m-c, not anything here. This patch sticks!

/be

Comment 27

[Robert Sayre]

merged to mc

Comment 28

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/30ddbdedf2fc
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4947c640f395

Comment 29

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/30ddbdedf2fc reintroduced the too much recursion error in the browser test js1_5/GC/regress-348532.js <http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=js1_5%2FGC%2Fregress-348532.js;language=type;text/javascript> with or without jit on at least linux and mozilla-1.9.1. I don't see the error on mozilla central or tracemonkey. Has a changeset been missed?

Comment 30

[Robert Sayre]

(In reply to comment #29)
> I don't see the error
> on mozilla central or tracemonkey. Has a changeset been missed?

1.9.1 is behind in merging by a week or so still. Are you testing with latest m-c and tm?

Comment 31

[Bob Clary [:bc] (inactive)]

(In reply to comment #30)
> (In reply to comment #29)
> > I don't see the error
> > on mozilla central or tracemonkey. Has a changeset been missed?
> 
> 1.9.1 is behind in merging by a week or so still. Are you testing with latest
> m-c and tm?

yes.

Comment 32

[Bob Clary [:bc] (inactive)]

test included in js1_8_1/trace/trace-test.js 
http://hg.mozilla.org/mozilla-central/rev/8f967a7729e2

Comment 33

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

I'm not able to reproduce this crash anymore with the given testcase. There is even no crash with this stack reported within the last 3 weeks on 1.9.1 and trunk. Further the test passes to.

Based on this data I verify this bug on trunk and 1.9.1