753162 - "Assertion failure: cx->compartment == this" with document.write during DOMAttrModified

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Jesse Ruderman]

Attachment: testcase (asserts fatally when loaded)

During DOMAttrModified:

###!!! ASSERTION: nsHTMLDocument::Reset() - Wyciwyg Channel  still exists!: '!mWyciwygChannel', file content/html/document/src/nsHTMLDocument.cpp, line 329

(That first assertion might be bug 675518 or bug 680086.)

After DOMAttrModified:

Assertion failure: cx->compartment == this, at js/src/jscompartment.cpp:124

Comment 1

[Jesse Ruderman]

Attachment: stack traces

Comment 2

[Daniel Veditz [:dveditz]]

CC'ing moz_bug_r_a4 in case this compartment confusion leads to interesting discoveries.

Comment 3

[Bobby Holley (:bholley)]

Ok, the basic problem here is that the code in CrossCompartmentWrapper::call is assuming that the AutoCompartment restored cx->compartment to the same compartment it had when enter() was called, which isn't necessarily the case. Specifically, the function being call()-ed ends up calling document.write, which triggers a call to SetOuterObject on the outer window. This changes cx->globalObject, and so the resetCompartment() that happens in AutoCompartment::leave() sets cx->compartment to the new scope.

The fix is to just wrap with cx->compartment. Patch coming up.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Don't assume that we end up in the same compartment as we started in CrossCompartmentWrapper. v1

Attaching a patch, flagging blake for review.

Comment 5

[Bobby Holley (:bholley)]

Gave this a try push: https://tbpl.mozilla.org/?tree=Try&rev=704b40375e24

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 634862 [details] [diff] [review]
Don't assume that we end up in the same compartment as we started in CrossCompartmentWrapper. v1

IIRC, this has been a longstanding concern... it's always been odd that if I have:

JSAutoRequest ac; ac.enter(cx, compartmentA);

after a call, cx->compartment might not be compartmentA. Will this get better when we remove fake stack frames?

I'm fine with this fix, since it doesn't make anything worse, though.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Blake Kaplan (:mrbkap) from comment #6)
> Will this get better when we remove fake stack frames?

I don't think so. The fundamental issue is that the document moves compartments during the course of the call. There are two invariants that would be nice:
1 - If we enter from compartment A, we leave in compartment A
2 - If we enter in the compartment of document D, we leave in the compartment of document D

AFAICT we can only guarantee one of them. Though I wonder what will happen when cx->globalObject goes away like I've been hearing it will. Will there be a cx->defaultCompartment?

pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/eca1167488ca

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/eca1167488ca

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Bobby Holley (:bholley) from comment #7)
> 1 - If we enter from compartment A, we leave in compartment A
> 2 - If we enter in the compartment of document D, we leave in the
> compartment of document D
> 
> AFAICT we can only guarantee one of them.

Right. The problem is that right now we have a mix of the two options based on whether or not entering the compartment was a no-op or not and whether there were already stack frames on the context. I think that, in general, option 1 is more important since any code that enters compartment A will still only have objects from compartment A lying around (even if they've been brain transplanted), but all this belongs in some other bug, I suppose.

Comment 10

[Daniel Veditz [:dveditz]]

Is this fallout from cpg or does it affect earlier builds as well?

Comment 11

[Bobby Holley (:bholley)]

(In reply to Daniel Veditz [:dveditz] from comment #10)
> Is this fallout from cpg or does it affect earlier builds as well?

The former, I think.

Comment 12

[Daniel Veditz [:dveditz]]

Should we land this in Aurora then?

Comment 13

[Bobby Holley (:bholley)]

(In reply to Daniel Veditz [:dveditz] from comment #12)
> Should we land this in Aurora then?

Yes, probably.

Comment 14

[Bobby Holley (:bholley)]

Comment on attachment 634862 [details] [diff] [review]
Don't assume that we end up in the same compartment as we started in CrossCompartmentWrapper. v1

Nominating per comment 12.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 650353
User impact if declined: Potential security vulnerabilities.
Testing completed (on m-c, etc.): Baking on m-c.
Risk to taking this patch (and alternatives if risky): Not risky. 
String or UUID changes made by this patch: None.

Comment 15

[Alex Keybl [:akeybl]]

Comment on attachment 634862 [details] [diff] [review]
Don't assume that we end up in the same compartment as we started in CrossCompartmentWrapper. v1

[Triage Comment]
Low risk sg:crit fix. Approved for Aurora 15.

Comment 16

[Bobby Holley (:bholley)]

Pushed to m-a: http://hg.mozilla.org/releases/mozilla-aurora/rev/8859b02413f4

Comment 17

[u279076]

Confirmed testcase crashes/asserts 2012-05-08 Firefox 15.0a1 Nightly Debug.

Does not crash:
2012-06-22 Firefox 16.0a1 Nightly Debug
2012-06-26 Firefox 15.0a2 Aurora Debug