Closed Bug 790865 Opened 7 years ago Closed 7 years ago
"Assertion failure: (thing)->compartment()->is
Collecting()" with bfcache, canvas, gcslice
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.
Assignee: general → wmccloskey
Status: NEW → ASSIGNED
Attachment #661041 - Flags: review?
Attachment #661041 - Flags: review? → review?(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?
Attachment #661041 - Flags: review?(terrence) → review+
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.
(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.
(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.
Can we add a reliable testcase for this to the testsuite? Maybe when bug 829349 is through?
mass remove verifyme requests greater than 4 months old
You need to log in before you can comment on or make changes to this bug.