738841 - "Assertion failure: allocated()," with TypeInference disabled

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

try {
    for (let z = 0; z < 1; ++evalcx("[]", newGlobal("new-compartment"))) {}
} catch (e) {}
try {
    for (y in [schedulegc(58)]) {
        b
    }
} catch (e) {}
try {
    e
} catch (e) {}
try {
    (function() {
        h
    }())
} catch (e) {}
try {
    (function() {
        this.m.f = function() {}
    }())
} catch (e) {}
try {
    t()
} catch (e) {}
try {
    p
} catch (e) {}
try {
    gc()
    p
} catch (e) {}
try {
    (function() {
        for (var v of m) {}
    }())
} catch (e) {}
try {
    m
} catch (e) {}
try {
    (function() {
        {
            print(new function(q)("", s))
            let u
        }
    }())
} catch (e) {}

asserts 64-bit js debug shell on m-c changeset 6470fe2fc4de with -m and -a at Assertion failure: allocated(),

Setting s-s because gc is on the stack.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   87736:fbef6a165cf8
user:        Bill McCloskey
date:        Fri Feb 10 18:32:08 2012 -0800
summary:     Bug 723313 - Stop using conservative stack scanner for VM stack marking (r=luke,bhackett)

Comment 1

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

One needs to pass the testcase in as a CLI argument.

Related to bug 738846?

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This does appear to be the same issue as bug 738846. I'll dupe when I know for sure.

We crash on this line:
    print(new function(q)("", s))

While we're in the middle of running JSOP_NEW, we do a GC and crash. We crash while marking a stack slot which is supposed to contain undefined, but contains garbage. The problem seems to be that we're not syncing the stack slot in JSOP_NEW. Here is the relevant code:

    /*
     * 'this' does not need to be synced for constructing. :FIXME: is it
     * possible that one of the arguments is directly copying the 'this'
     * entry (something like 'new x.f(x)')?
     */
    if (callingNew) {
        frame.discardFe(origThis);

        /*
         * If inference is enabled, the 'this' value of the pushed frame always
         * needs to be coherent. If a GC gets triggered before the callee can
         * fill in the slot (i.e. the GC happens on constructing the 'new'
         * object or the call object for a heavyweight callee), it needs to be
         * able to read the 'this' value to tell whether newScript constraints
         * will need to be regenerated afterwards.
         */
        if (cx->typeInferenceEnabled())
            masm.storeValue(NullValue(), frame.addressOf(origThis));
    }

If I just comment out this entire block, the test case runs fine. However, I don't totally understand the comment about TI. Is it important that we have NULL there, or is it only important that there be some valid GC thing there?

Comment 3

[Brian Hackett [Laid off!]]

(In reply to Bill McCloskey (:billm) from comment #2)
> If I just comment out this entire block, the test case runs fine. However, I
> don't totally understand the comment about TI. Is it important that we have
> NULL there, or is it only important that there be some valid GC thing there?

I think TI will want there to be a non-object value there, so that it doesn't get confused if it resets the 'this' object's properties while in the call prologue (type changes which happen before constructing the new object).  See TypeObject::clearNewScript.  It should be fine to just remove the cx->typeInferenceEnabled() test, so that the sync happens whether TI is on or off.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix

Thanks.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6b7b4b9f235

This affects Firefox 13, but it does not affect people who have TI enabled (i.e., virtually everyone). I'm not going to request approval.

Comment 7

[Daniel Veditz [:dveditz]]

Calling this "moderate" because it's a non-default configuration.

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f6b7b4b9f235

Comment 9

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug738841.js.