832103 - Crash [@ PropertyAccess<(PropertyAccessKind)1>] or [@ js::types::TypeCompartment::resolvePending] or "Assertion failure: hasAllFlags(OBJECT_FLAG_DYNAMIC_MASK),"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

RegExp("").exec()
Object.defineProperty(this, "x", {
    get: function() {
        return new Array
    }
})
Object.defineProperty(this, "y", {
    get: function() {
        return [function() {}, 0, 0, 0, 0, 0, 0]
    }
})
r = RegExp("");
uneval(undefined)
with({
    b: gczeal(9, 2)
});
r = /()/;
y.sort(function(j) {
    if (j) {
        a =
        new
        Array
    } else {
        x.v()
    }
})

asserts js debug shell on m-c changeset ce9cdd801a73 with -a at Assertion failure: hasAllFlags(OBJECT_FLAG_DYNAMIC_MASK), and crashes js opt shell at PropertyAccess<(PropertyAccessKind)1> with js::types::TypeCompartment::resolvePending on the stack.

s-s and assuming sec-critical because weird memory address 0x7ffff653200000 is being accessed.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   118821:6acc72608961
user:        Terrence Cole
date:        Fri Nov 09 09:45:25 2012 -0800
summary:     Bug 811060 - Move DeflateString out of jsstr and make it Typey; r=Waldo

Changeset 6acc72608961 only landed on m-c on January 15, so this should only affect nightlies (Firefox 21).

Comment 1

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

Terrence, do you think bug 811060 is a possible regressor?

Comment 2

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

On hindsight, I think the testcase can be reduced a little bit more, but I'll leave it to others here...

Comment 3

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 02e12a80aef9).

Comment 4

[Christian Holler (:decoder)]

JSBugMon: Fix Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   119272:7b531a62b114
user:        Brian Hackett
date:        Fri Jan 18 09:23:28 2013 -0700
summary:     Bug 772820 - Disallow GCs during script analysis or compilation, r=billm.

This iteration took 97.646 seconds to run.

Comment 5

[Christian Holler (:decoder)]

Brian, did the rev in comment 4 fix this issue?

Comment 6

[Brian Hackett [Laid off!]]

No idea.  These GC tests are super fragile and bug 772820 can affect when they occur.

Comment 7

[Terrence Cole [:terrence]]

Attachment: v0: add test to jit-tests

I have to second Brian here: I can't see how either of these patches would have impacted this other than random GC timing changes. We might as well just add the test to the suite so that if it shows up again we'll know and can take a look at it then.

Comment 8

[Terrence Cole [:terrence]]

I managed to reproduce this on the given revision and it appears to just be very random -- not surprising since the problems is we're accessing a dead (background finalized) thing. It appears to always flow out of the stack (|lval|, |regs.sp[-1]|, etc) and it appears to be flowing out of JM -- JM does the last assignment to the stack slot before the next, crashing access. I was still not able to reproduce on tip, however.

Brian, do you know of any recent rooting changes at the boundary of JM that would have fixed this?

Comment 9

[Brian Hackett [Laid off!]]

Comment on attachment 709770 [details] [diff] [review]
v0: add test to jit-tests

New tests don't need review.

Comment 10

[Terrence Cole [:terrence]]

Sorry for the bother, I totally forgot about that rule!

https://hg.mozilla.org/integration/mozilla-inbound/rev/36e03cf9cb41

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/36e03cf9cb41

(Not sure if this needs resolving at this point or not...)

Comment 12

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

Let's assume the vague possibility that bug 772820 fixed it.

Comment 13

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

VERIFIED because in-testsuite+ - the test landed.