821842 - compartment mismatch in nsXBLBinding::DoInitJSClass during print preview

Status: RESOLVED DUPLICATE of bug 817342

(Core :: XBL, defect)

Description

[Andrew McCreight [:mccr8]]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Andrew McCreight [:mccr8]]

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 [1] 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?

[1] http://dxr.mozilla.org/mozilla-central/content/xbl/src/nsXBLProtoImpl.cpp.html#l91

Comment 3

[Bobby Holley (:bholley)]

Ugh, this is bug 733636. This implicitly passes aAllowWrapping=false.

Comment 4

[Bobby Holley (:bholley)]

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.

Comment 5

[Andrew McCreight [:mccr8]]

Yeah, I was just thinking that would be useful for bug 791896.

Comment 6

[Andrew McCreight [:mccr8]]

This appears to be fairly common now that the other compartment checker bug was fixed. There were 15 on the 12-16 build.

Comment 7

[Andrew McCreight [:mccr8]]

(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...

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: manually eliminate default arg

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

In the meantime, this is crashing users in opt builds.  Are we planning to ship opt compartment checks in the final release of 20?

Comment 10

[Andrew McCreight [:mccr8]]

No, Bill set them up with some kind of thing where they autodisable in beta and release, or something like that.

Comment 11

[Andrew McCreight [:mccr8]]

The regression range in bug 825401 suggests that this could be a regression from CPG, which would make sense.

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: manually eliminate default arg, v2

Comment 13

[Andrew McCreight [:mccr8]]

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.

Comment 14

[Andrew McCreight [:mccr8]]

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.

Comment 15

[Bobby Holley (:bholley)]

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.

Comment 16

[Andrew McCreight [:mccr8]]

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.

Comment 17

[Andrew McCreight [:mccr8]]

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

Comment 18

[Andrew McCreight [:mccr8]]

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.

Comment 20

[Andrew McCreight [:mccr8]]

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.

Comment 21

[Andrew McCreight [:mccr8]]

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.

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected