Closed Bug 821842 Opened 7 years ago Closed 7 years ago
compartment mismatch in ns
XBLBinding::Do Init JSClass during print preview
I looked at all of the compartment checker crashes, and aside from various URL binding crashes, this was the only other one I saw. The stacks look like this: 0 js::CompartmentChecker::fail js/src/jscntxtinlines.h:204 1 JS_GetObjectId js/src/jsapi.cpp:3361 2 nsXBLBinding::DoInitJSClass content/xbl/src/nsXBLBinding.cpp:1382 3 nsRuleNode::ComputeUserInterfaceData layout/style/nsRuleNode.cpp:4062 4 XPCJSContextStack::Push js/xpconnect/src/XPCJSContextStack.cpp:102 5 XPCJSRuntime::Get js/xpconnect/src/xpcprivate.h:667 6 nsXPConnect::Pop js/xpconnect/src/nsXPConnect.cpp:2127 7 nsContentUtils::WrapNative content/base/src/nsContentUtils.cpp:6076 8 nsXBLProtoImpl::InitTargetObjects content/xbl/src/nsXBLProtoImpl.cpp:133 9 nsCOMPtr<nsIRDFXMLSerializer>::nsCOMPtr<nsIRDFXMLSerializer> obj-firefox/dist/include/nsCOMPtr.h:538 10 nsXBLProtoImpl::InstallImplementation content/xbl/src/nsXBLProtoImpl.cpp:70 11 nsBindingManager::SetBinding content/xbl/src/nsBindingManager.cpp:551 12 nsXBLBinding::InstallImplementation content/xbl/src/nsXBLBinding.cpp:1064 13 nsXBLService::LoadBindings content/xbl/src/nsXBLService.cpp:563 14 je_malloc The three instances of this crash that I found: https://crash-stats.mozilla.com/report/index/088f9c7e-cabc-4587-933d-3ae042121214 https://crash-stats.mozilla.com/report/index/df0c92cb-916e-4b6c-869d-513e62121214 https://crash-stats.mozilla.com/report/index/7142209e-e469-4ce5-ab54-f9c0b2121214
That stack looks completely bogus... Assuming that the top several frames are right, though, looks like JS_GetPrototype does NOT assert on compartment mismatches (should it?) so presumably the args to DoInitJSClass were already in the wrong compartments. The only aller is nsXBLPrototypeBinding::InitClass. Afaict nothing really guarantees that the args to InitClass are same-compartment; either the callers need to ensure that, or InitClass itself does.
Here's another crash with a saner looking stack: https://crash-stats.mozilla.com/report/index/c25eea71-cf54-4057-9e08-3b68a2121214 0 js::CompartmentChecker::fail js/src/jscntxtinlines.h:204 1 JS_GetObjectId js/src/jsapi.cpp:3361 2 nsXBLBinding::DoInitJSClass content/xbl/src/nsXBLBinding.cpp:1382 3 nsXBLProtoImpl::InitTargetObjects content/xbl/src/nsXBLProtoImpl.cpp:133 4 nsXBLProtoImpl::InstallImplementation content/xbl/src/nsXBLProtoImpl.cpp:70 5 nsXBLService::LoadBindings content/xbl/src/nsXBLService.cpp:563 As you say, the only caller of DoInitJSClass is InitClass, so PGO probably just mashed the latter into the former. nsXBLProtoImpl::InitTargetObjects is in fact a caller of InitClass, and shows up in all 4 stacks, so presumably that is the place that is causing problems. The relevant code on that path  looks like: rv = nsContentUtils::WrapNative(jscontext, global, aBoundElement, &v, getter_AddRefs(wrapper)); [check for failure] rv = aBinding->InitClass(mClassName, jscontext, global, JSVAL_TO_OBJECT(v), aTargetClassObject); I would have thought that wrapping it would result in something in the same compartment, but I don't really know wrapping stuff very well. I didn't notice any auto compartment stuff happening before that anywhere on the call stack, so maybe that's the problem?  http://dxr.mozilla.org/mozilla-central/content/xbl/src/nsXBLProtoImpl.cpp.html#l91
Ugh, this is bug 733636. This implicitly passes aAllowWrapping=false.
Though actually we probably don't want a security wrapper here, and should just enter the compartment of the global, and assert that the result of WrapNative is same-compartment. Can we expose assertSameCompartment outside the js engine? I always end up doing MOZ_ASSERT(js::IsObjectInContextCompartment(...)), which is nowhere near as nice. And we don't get to turn those on for release builds like we're doing with assertSameCompartment.
Yeah, I was just thinking that would be useful for bug 791896.
This appears to be fairly common now that the other compartment checker bug was fixed. There were 15 on the 12-16 build.
(In reply to Bobby Holley (:bholley) from comment #4) > Can we expose assertSameCompartment outside the js engine? I filed bug 822425 for this. I'm looking at a broader fix for this in bug 733636. We don't want to explicitly mark the dependency because we want to be more subtle about setting the ENABLE_SECURITY flag to true...
Assignee: nobody → continuation
Whiteboard: [to be fixed by bug bug 733636]
I'll just post ongoing work in this bug instead of the public one. Hopefully we can come up with something that is less obvious. Anyways, this eliminates the implicit argument
In the meantime, this is crashing users in opt builds. Are we planning to ship opt compartment checks in the final release of 20?
No, Bill set them up with some kind of thing where they autodisable in beta and release, or something like that.
The regression range in bug 825401 suggests that this could be a regression from CPG, which would make sense.
I audited every callsite of nsContentUtils::WrapNative. I only left the previous false behavior for two places: - nsXBLProtoImplAnonymousMethod::Execute: this immediately enters the compartment of the return value - nsJSContext::JSObjectFromInterface: this has two callers. One does a JS_WrapObject, which I think makes it okay, and the other enters the compartment. For the rest of the sites, the most common reason they look okay is because they involve either wrapping a new object, or an object that isn't wrapper cached. There were a number of places that I couldn't figure out why they are okay, so maybe they weren't. I guess it doesn't matter too much, though.
Setting the allowWrapping flag to "true" in nsXBLProtoImpl::InitTargetObjects seems to break video controls. For instance, parser/htmlparser/tests/mochitest/test_bug709083.html: WARNING: Not same origin error!: file /Users/amccreight/mz/cent/dom/base/nsJSEnvironment.cpp, line 389 TEST-UNEXPECTED-FAIL | unknown test url | uncaught exception - Script error. at chrome://global/content/bindings/videocontrols.xml:0 This is unfortunate because this is the particular place we're getting these cross-compartment errors. I'll investigate.
Hmmm, probably has to do with the fact that aAllowSecurityWrappers in this case applies _both_ to cross-compartment wrappers _and_ same-compartment security wrappers, and the code here is probably getting confused by the resulting SCSW SOW.
Judging by the crash report comments (almost all of them refer to print preview), the stacks, and dholbert's test cases, this seems to be related to print preview.
For instance, in this stack you can see nsDocumentViewer::PrintPreview deep in the stack: https://crash-stats.mozilla.com/report/index/6477eed7-341f-4dc2-8973-d10862130103
Summary: compartment mismatch in nsXBLBinding::DoInitJSClass → compartment mismatch in nsXBLBinding::DoInitJSClass during print preview
I strongly suspect this is a dupe of bug 817342. All the comments involve print preview, which is the same problem as that bug. Additionally, dholbert came up with steps to cause the same stack as this bug, involving print preview. When those STR are run in a debug, rather than Nightly build, they produce the same stacks as bug 817342. My guess is that we have stricter compartment checks in debug builds than we do in Nightly builds, so it simply ends up dying earlier. Downgrading this to sec-moderate because it seems to involve running addon JS during a print preview, which would probably be difficult to exploit. Once the patch for that bug lands, it will be easy to confirm that this signature has gone away. I don't want to explicitly dupe this bug to that one, both so we can track if it goes away, and to hide the connection to compartment problems a bit.
No longer blocks: 825380
Duplicate of this bug: 825380
It is still possible that we're hitting multiple problems here, and something like bug 733636 will also be required, so we'll have to see.
There have been no crashes with this signature since the 1/10 build. The patch for bug 817342 landed first in the 1/11 Nightly, so it seems safe to dupe this.
No longer blocks: compartment-mismatch
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 817342
Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected
You need to log in before you can comment on or make changes to this bug.