Closed Bug 738841 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla14
Tracking Status
firefox12 --- unaffected
firefox13 --- wontfix
firefox14 --- fixed
firefox-esr10 - unaffected

People

(Reporter: gkw, Assigned: billm)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [sg:moderate][advisory-tracking+] js-triage-needed)

Attachments

(2 files)

Attached file 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)
One needs to pass the testcase in as a CLI argument.

Related to bug 738846?
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?
(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.
Attached patch fixSplinter Review
Thanks.
Assignee: general → wmccloskey
Status: NEW → ASSIGNED
Attachment #609528 - Flags: review?(bhackett1024)
Duplicate of this bug: 738846
Attachment #609528 - Flags: review?(bhackett1024) → review+
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.
Target Milestone: --- → mozilla14
Calling this "moderate" because it's a non-default configuration.
Summary: "Assertion failure: allocated()," → "Assertion failure: allocated()," with TypeInference disabled
Whiteboard: js-triage-needed → [sg:moderate] js-triage-needed
https://hg.mozilla.org/mozilla-central/rev/f6b7b4b9f235
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Whiteboard: [sg:moderate] js-triage-needed → [sg:moderate][advisory-tracking+] js-triage-needed
Group: core-security
A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug738841.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.