790865 - "Assertion failure: (thing)->compartment()->isCollecting()" with bfcache, canvas, gcslice

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: testcase

1. Set
     user_pref("javascript.options.mem.gc_incremental", false);
2. Install https://www.squarefree.com/extensions/domFuzzLite3.xpi
3. Load the testcase.

Assertion failure: (thing)->compartment()->isCollecting(), at js/src/gc/Marking.cpp:586

May be related to an intermittent orange, bug 785582.

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

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

Attachment: patch

Nice find, Jesse! We get in trouble here while doing some wrapper rejiggering and we call JSObject::swap. At this point, the value of cx->compartment is not expected to be anything in particular. Then we call ReserveForTradeGuts without entering any compartment. It potentially allocates shapes and types, which go in cx->compartment, and that is likely to be the wrong compartment.

I'm pretty surprised that this bug has lasted as long as it has. It seems to have been introduced with the ObjShrink work (FF11), which was when we tied an object's shape to the number of slots it has. That's what forces us to make new shapes.

The fix is to add an AutoEnterCompartment in ReserveForTradeGuts. The remainder of the patch adds a lot of compartment assertions. Perhaps most significantly, it adds a much more precise compartment checker that runs at the beginning of every GC. It iterates over every GC thing in the heap, looking for pointers outside the current compartment. With only this checker enabled, and without the AutoEnterCompartment, I get an assertion just doing a little browsing around. Very embarrasing.

Hopefully this fix will cause a noticeable downward effect on our crash volume.

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 661041 [details] [diff] [review]
patch

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

This is filled with win.  Would it be possible and worth it to systematically add assertions for cx->compartment == thing->compartment at all JS_ entry points that take gcthing pointers?

Comment 4

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6852b4928efa

Comment 5

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

Comment on attachment 661041 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): ObjShrink (FF11)
User impact if declined: Possible crashes, exploits.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Low (the actual patch is a very safe one-liner).
String or UUID changes made by this patch: None.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6852b4928efa

Comment 7

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/285474e7e7f0
https://hg.mozilla.org/releases/mozilla-beta/rev/5aab8875d036

Comment 8

[Jeff Walden [:Waldo]]

(In reply to Terrence Cole [:terrence] from comment #3)
> Would it be possible and worth it to systematically add assertions
> for cx->compartment == thing->compartment at all JS_ entry points
> that take gcthing pointers?

Did anyone ever follow up on this?  It seems like a really good idea to me.

Comment 9

[Andrew McCreight [:mccr8]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> Did anyone ever follow up on this?  It seems like a really good idea to me.

I just filed bug 829349 on this.

Comment 10

[Christian Holler (:decoder)]

Can we add a reliable testcase for this to the testsuite? Maybe when bug 829349 is through?

Comment 11

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old