Closed Bug 790865 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla18
Tracking Status
firefox15 --- wontfix
firefox16 --- fixed
firefox17 --- fixed
firefox18 --- fixed
firefox-esr10 --- unaffected

People

(Reporter: jruderman, Assigned: billm)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, sec-critical, testcase, Whiteboard: [advisory-tracking+])

Attachments

(3 files)

Attached file 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.
Attached file stack
Group: core-security
Keywords: sec-critical
Attached patch patchSplinter Review
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.
Attachment #661041 - Flags: approval-mozilla-beta?
Attachment #661041 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/6852b4928efa
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Attachment #661041 - Flags: approval-mozilla-beta?
Attachment #661041 - Flags: approval-mozilla-beta+
Attachment #661041 - Flags: approval-mozilla-aurora?
Attachment #661041 - Flags: approval-mozilla-aurora+
Depends on: 791896
Whiteboard: [advisory-tracking+]
Depends on: 806663
Group: core-security
(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
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.