516 bytes, text/html
33.52 KB, text/plain
9.57 KB, patch
|Details | Diff | Splinter Review|
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
CC'ing moz_bug_r_a4 in case this compartment confusion leads to interesting discoveries.
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.
Attaching a patch, flagging blake for review.
Attachment #634862 - Flags: review?(mrbkap)
Gave this a try push: https://tbpl.mozilla.org/?tree=Try&rev=704b40375e24
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.
Attachment #634862 - Flags: review?(mrbkap) → review+
(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
Assignee: nobody → bobbyholley+bmo
(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.
Is this fallout from cpg or does it affect earlier builds as well?
(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.
Should we land this in Aurora then?
(In reply to Daniel Veditz [:dveditz] from comment #12) > Should we land this in Aurora then? Yes, probably.
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.
Attachment #634862 - Flags: approval-mozilla-aurora?
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.
Attachment #634862 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [sg:high] → [sg:high][advisory-tracking+]
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
You need to log in before you can comment on or make changes to this bug.