825326 - "Assertion failure: (obj)->compartment()->isGCMarking(),"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

try {
    a = []
    r = /x/
    gczeal(10, 2)()
} catch (e) {}
try {
    (function() {
        r(function() {
            eval()
        })
    })()
} catch (e) {}
try {
    s
} catch (e) {}
a.every()

asserts js debug shell on m-c changeset f2a500997116 with -a at Assertion failure: (obj)->compartment()->isGCMarking(),

s-s because GC seems involved. Assuming sec-critical unless otherwise shown.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   116562:54696b3f852b
user:        Jon Coppeard
date:        Tue Dec 18 13:27:28 2012 +0000
summary:     Bug 820186 - Various crashes/assertions with gczeal(10) and random recursion. r=billm

Tested with --enable-more-deterministic, but I'm not sure if it's needed.

Comment 1

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Potential fix

Comment 3

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

(In reply to Jon Coppeard (:jonco) from comment #2)
> Created attachment 697065 [details] [diff] [review]
> Potential fix

Is the testcase from bug 820186 (the one with the Mersenne Twister) intended to be landed in this patch as well?

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Gary Kwong [:gkw] from comment #3)
> (In reply to Jon Coppeard (:jonco) from comment #2)
> > Created attachment 697065 [details] [diff] [review]
> > Potential fix
> 
> Is the testcase from bug 820186 (the one with the Mersenne Twister) intended
> to be landed in this patch as well?

Well, it should have been in the fix for bug 820186.  But this bug was caused by the fix for that one so I included it here.

Comment 5

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

(In reply to Jon Coppeard (:jonco) from comment #2)
> Created attachment 697065 [details] [diff] [review]
> Potential fix

I verify that this patch does fix the bug. :)

Comment 6

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

> I verify that this patch does fix the bug. :)

In addition, this bug seems to only occur on Mac, it did not reproduce when I tested on 32-bit Linux js shell.

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 697065 [details] [diff] [review]
Potential fix

It sounded like this was ready for review, so I'll mark it thus so it doesn't get lost.

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 697065 [details] [diff] [review]
Potential fix

Review of attachment 697065 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsgc.cpp
@@ +3823,5 @@
>        case SWEEP:
>          for (CompartmentsIter c(rt); !c.done(); c.next()) {
>              c->scheduledForDestruction = false;
>  
> +            if (c->isGCMarking() && c->activeAnalysis && !c->gcTypesMarked) {

Oh, wow. I'm wondering how this worked at all: do we just hit a reset during sweeping in a compartmental GC incredibly infrequently?

Comment 9

[Jon Coppeard (:jonco)]

(In reply to Terrence Cole [:terrence] from comment #8)

Well, this code only got introduced recently.  And we hit the situation where analysis becomes active during GC pretty infrequently anyway, and on top of that it has to be a compartmental GC as well. 

Cheers for the review.

Comment 10

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e5257f5f4a6

Comment 11

[Jon Coppeard (:jonco)]

Backed out and relanded with correct bug number in commit message:

https://hg.mozilla.org/integration/mozilla-inbound/rev/05895b39ed9e

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/05895b39ed9e

Comment 13

[Christian Holler (:decoder)]

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