I kind of dropped the ball on this, but I'm looking at it again. I have some kind of taxonomy of all the places we call this which I'll put in this bug. The fix for this will live in bug 733636, but please don't mark the dependence.

try run is green (I think slightly different than the attached patch): https://tbpl.mozilla.org/?tree=Try&rev=5cfa8f3f1177

First, here are the things that look like they should be for sure okay: already pass in true - nsContentUtils.cpp - nsFrameMessageManager::ReceiveMessage - nsXMLHttpRequest::GetResponse - nsXBLPrototypeHandler::ExecuteHandler - Navigator::DoNewResolve, - nsTArrayHelpers wrappee is newly created, so it can't have a security wrapper - CustomElementConstructor in nsDocument.cpp - nsGlobalWindow::PostMessageReadStructuredClone - IndexedDatabaseManager::InitWindowless - SmsManager::Send - MobileMessageManager::Send - CreateXMLHttpRequest in XPCComponents.cpp wrappee isn't wrapper cached (mostly nsIDOMBlob or nsIDOMFile) - ArchiveRequest::GetFileResult - ArchiveRequest::GetFilesResult - GetFileHelper::GetSuccessResult - IDBObjectStore::StructuredCloneReadCallback - Read in StructuredCloneUtils.cpp - MmsMessage::GetAttachments (?) - WorkerPrivate structured clone thing - MmsAttachmentDataToJSObject - MobileMessageCallback::NotifySuccess (wraps SmsSegmentInfo) - MobileMessageCursorCallback::NotifyCursorResult (wraps SmsMessage) - InterfaceToJsval in nsDeviceStorage.cpp: two callers. nsIFileToJsval (wraps nsIDOMBlob), and DeviceStorageRequestChild::Recv__delete__ (wraps nsIDOMFile) - SmsRequest::SetSuccessInternal: argument is nsISupports*, but it is only called from two places, with classes that aren't wrappercached. - nsXULTemplateBuilder::InitHTMLTemplateRoot: last two uses (wraps nsIRDFCompositeDataSource and nsIXULTemplateBuilder)

This next set look okay to me as is, but could use some double checking: wrapping into the same compartment so it is okay (?) - nsGlobalWindow::GetContent: gets content, wraps it (?) these seem okay to use with false - nsIDocument::AdoptNode (explicitly passes false, so okay?) - nsIDocument::WrapObject (explicitly passes false, so okay?) - nsXBLProtoImplAnonymousMethod::Execute: calls JS_WrapObject on the result of WrapNative, so I guess that's okay? - nsJSContext::JSObjectFromInterface: "We don't wrap here because we trust the JS engine to wrap the target later." The comment is a little odd. The one caller is nsJSContext::BindCompiledEventHandler, which enters the compartment of the object, so maybe that's okay? - nsJSContext::ConvertSupportsTojsvals: seems to pull a random nsISupports out of an array and wrap it. The only place that calls it is nsJSContext::SetProperty, which goes through and calls JS_WrapValue on everything, so I guess we should pass in false? I haven't tested this one yet.

doesn't look okay, setting to true passes tests: - nsXMLHttpRequest::GetInterface: QIs the XHR to some interface, wraps it, returns it. - xpcJSWeakReference::Get: wraps and returns some random nsISupports. - nsJSContext::AddSupportsPrimitiveTojsval: Grabs random nsISupports and wraps and returns it. Maybe this is only used with nsVariant which isn't wrapper cached? - HelperBase::WrapNative in AsyncConnectionHelper.cpp is just a thin wrapper for WrapNative. Used in various GetSuccessResult methods for IDB stuff (such as IPCOpenDatabaseHelper, IPCSetVersionHelper, and ContinueHelper). I'm not sure why this is okay. - nsXULTemplateBuilder::InitHTMLTemplateRoot: first use wraps an element, it looks like. I'm not sure why that is okay. doesn't look okay, and fliping it to true causes test failures: - nsXBLProtoImpl::InitTargetObjects: almost certainly bad. we wrap a bound element, but it could end up in another compartment. There are actually many compartment failures like this in the wild: it is the most common cause of compartment mismatches on Nightly, going by the last week or so of crash stats. example of failures: https://crash-stats.mozilla.com/report/index/16405524-21fe-404b-a108-b33032140103 https://crash-stats.mozilla.com/report/index/41e0e3c8-7fdc-4504-96a3-921f72140102 https://crash-stats.mozilla.com/report/index/409c1247-6da4-44e0-839a-cf84b2140102 https://crash-stats.mozilla.com/report/index/13c59231-09f5-4fc1-a53e-f96442140101 https://crash-stats.mozilla.com/report/index/4a92f43d-655a-4c8f-bda3-a673f2140101 https://crash-stats.mozilla.com/report/index/8fc17ee5-b58e-4c31-989e-e5abb2140103 https://crash-stats.mozilla.com/report/index/a28f8216-8975-4dfe-8022-885ac2140103 https://crash-stats.mozilla.com/report/index/ea8059e1-56c4-47c4-bb57-6857e2140102 (some of these seem like they are kind of dupes, with similar sets of addons) bholley pointed out this issue in bug 851353, which was fixed another way. But, the fun thing here is that setting this to true causes various failures, in things like: content/events/test/test_focus_disabled.html

I haven't really investigated the failures with InitTargetObjects yet. Anyways, if I could get some feedback on comment 5 and 6, I'd appreciate it, Bobby.

(In reply to Andrew McCreight [:mccr8] from comment #5) > wrapping into the same compartment so it is okay (?) > - nsGlobalWindow::GetContent: gets content, wraps it (?) I don't follow. This seems like it's only safe if |aCx| is currently in the compartment of |content|'s global, which doesn't seem likely. Presumably the callers of GetContent() might be expecting an unwrapped return value, but it doesn't seem like it. In fact, GetScriptableContent looks pretty dangerous as-is... > these seem okay to use with false > - nsIDocument::AdoptNode (explicitly passes false, so okay?) Yes, it seems like it's trying to get a scope. > - nsIDocument::WrapObject (explicitly passes false, so okay?) This is only safe insofar as the document should already be same-compartment with the window. But I think it's playing with fire. Let's remove the explicit |false| and replace it with an assertion that the result is not a cross-compartment wrapper? > - nsXBLProtoImplAnonymousMethod::Execute: calls JS_WrapObject on > the result of WrapNative, so I guess that's okay? Yeah, but it wouldn't hurt to wrap it in the first place. Presumably we'll get that when we flip the defualt? > - nsJSContext::JSObjectFromInterface: "We don't wrap here because we > trust the JS engine to wrap the target later." The comment is a > little odd. The one caller is > nsJSContext::BindCompiledEventHandler, which enters the compartment > of the object, so maybe that's okay? I guess so. > - nsJSContext::ConvertSupportsTojsvals: seems to pull a random > nsISupports out of an array and wrap it. The only place that calls > it is nsJSContext::SetProperty, which goes through and calls > JS_WrapValue on everything, so I guess we should pass in false? > I haven't tested this one yet. Is there any reason to pass false? There's no harm in wrapping things twice, and it makes it less of a footgun.

(In reply to Andrew McCreight [:mccr8] from comment #6) > doesn't look okay, and fliping it to true causes test failures: > - nsXBLProtoImpl::InitTargetObjects: almost certainly bad. we wrap a bound > element, but it could end up in another compartment. There are actually > many > compartment failures like this in the wild: it is the most common cause of > compartment mismatches on Nightly, going by the last week or so of crash > stats. > But, the fun thing here is that setting this to true causes various > failures, in things like: > content/events/test/test_focus_disabled.html Hm. So, the bound element's reflector _should_ be same-compartment with its OwnerDoc's reflector, which in this case should be the compartment of |global|. Of course, in this case, |cx| is in no compartment in particular (which should definitely be fixed - we should unscope the enter of |global|'s compartment). But the only thing that ends after the WrapNative() call is a call to nsXBLPrototypeBinding::InitClass(), which bounces directly to nsXBLBinding::DoInitJSClass(), whose first order of business is to enter the compartment of |global|. So at first glance, this should be ok (though fragile without the change I proposed above). Nevertheless, the fact that you get gets failures when you pass in |true| presumably must mean that there are cases when the element's reflector is not same-compartment with |global|. Can you reproduce it in a debugger? If so, we should be able to figure out what's going wrong. Those JS_GetObjectId crashes are concerning. It looks like JS_GetPrototype doesn't assertSameCompartment, so either |obj| isn't same-compartment with |cx|, or isn't same-compartment with its proto. Given the other enforcement in the engine with respect to proto sets, I'd bet my money on the former. And this, along with your evidence of test failure (and thus behavior change), suggests that we have element that isn't same-compartment with its document, or a document that isn't same-compartment with its GetScopeObject. GetScopeObject should return an inner, right? If it's returning an outer, and there's navigation involved, that would certainly explain it.

(In reply to Bobby Holley (:bholley) from comment #9) > GetScopeObject should return an inner, right? If it's returning an outer, > and there's navigation involved, that would certainly explain it. I wish I knew more about XBL... Yes, it should return the inner, although by default it is set to the JunkScope, and then it has the be set manually to point to the inner (if it's a window). So I can imagine that somewhere it is not set (http://mxr.mozilla.org/mozilla-central/source/content/xbl/src/nsXBLService.cpp#1023 like here... is this OK btw?) or that something calls GetScopeObject before it were set to the right thing and gets the JunkScope accidentally.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #10) > I wish I knew more about XBL... I'm happy to tell you more about it in SF in a few weeks. Just remind me. :P > Yes, it should return the inner, although by > default it is set to the JunkScope, and then it has the be set manually to > point to the inner (if it's a window). So I can imagine that somewhere it is > not set > (http://mxr.mozilla.org/mozilla-central/source/content/xbl/src/nsXBLService. > cpp#1023 like here... is this OK btw?) Yeah, it's ok. The XBL binding document never gets any sort of window or global associated with it (other than an XBL compilation scope, which is specially-managed). > or that something calls > GetScopeObject before it were set to the right thing and gets the JunkScope > accidentally. This is a possibility. Andrew can verify.

> Nevertheless, the fact that you get gets failures when you pass in |true| presumably must mean that there are cases when the element's reflector is not same-compartment with |global|. Looking into this a bit, when we pass "false", the resulting return value is still same-compartment with |global|. When we pass in "true", we're failing somewhere inside Proxy::setPrototypeOf() in a way that ends up setting an exception, which we never handle, which causes an assertion later. I'm currently trying to figure out what the proxy is that we're returning.

The good news is that a try run is green with everything flipped to true except for nsIDocument::AdoptNode and nsXBLProtoImpl::InitTargetObjects. https://tbpl.mozilla.org/?tree=Try&rev=58f0b8771abe I should also audit the other places that pass in false. For instance, there are a number of calls to nsXPConnect::WrapNative, and it always passes in false, which seems questionable, given that the callers are various innocents outside XPConnect. I'll file a followup for that at some point.

(In reply to Andrew McCreight [:mccr8] from comment #12) > > Nevertheless, the fact that you get gets failures when you pass in |true| presumably must mean that there are cases when the element's reflector is not same-compartment with |global|. > > Looking into this a bit, when we pass "false", the resulting return value is > still same-compartment with |global|. Ok. Presumably it's not a proxy? > When we pass in "true", we're failing > somewhere inside Proxy::setPrototypeOf() During wrapping? If so, that's quite odd - if the underlying object is same-compartment with |global|, then the wrap call shouldn't be doing anything complicated. > in a way that ends up setting an > exception, which we never handle, which causes an assertion later. Whatever we end up doing here, we should fix whatever path is failing to propagate errors. > I'm > currently trying to figure out what the proxy is that we're returning. Sounds good.

When passing |true| in, the call to JS_SetPrototype in nsXBLBinding::DoInitJSClass ends up hitting SecurityWrapper<Base>::setPrototypeOf, which sets an exception and fails. So that's where the exception comes from. We don't hit that code when passing in |false|. Putting this block right after the call to InitTargetObjects in nsXBLProtoImpl::InstallImplementation makes content/events/test/test_focus_disabled.html pass with |true| passed in to WrapNative: if (JS_IsExceptionPending(cx)) { JS_ClearPendingException(cx); } ...but various other tests like content/events/test/test_bug238987.html end up failing. They hit the same setPrototypeOf, and look like they are failing in a similar way, but for some reason there's no exception pending at that point. Then I get this error: 0:05.26 TEST-UNEXPECTED-FAIL | unknown test url | uncaught exception - Error: permission denied to unwrap object at :0 0:05.26 TEST-UNEXPECTED-FAIL | unknown test url | uncaught exception - Error: permission denied to unwrap object at :0 and then the test just falls apart. So my guess is that test_focus_disabled.html for some reason doesn't care that nsXBLService::LoadBindings fails, but most tests do. (The uncaught exception looks like the one that setPrototypeOf creates, so maybe that somehow magically gets handled higher up in the stack.)

(In reply to Andrew McCreight [:mccr8] from comment #15) > When passing |true| in, the call to JS_SetPrototype in > nsXBLBinding::DoInitJSClass ends up hitting > SecurityWrapper<Base>::setPrototypeOf, which sets an exception and fails. > So that's where the exception comes from. We don't hit that code when > passing in |false|. This is what I'm confused about - you said that the object returned when passing |false| was same-compartment with global, right? So how is it ending up with any sort of Security Wrapper when we pass |true|? Is it a SOW or something? That could explain it.

In the weird failing case, in XPCConvert::NativeInterface2JSObject is going into the wrapper cached DOM binding case, and flat is null. If we then only call that JS_WrapObject when js::GetContextCompartment(cx) != js::GetObjectCompartment(flat), then at least some of the tests start passing. So the question is, why do we wrap something into the same compartment, and then end up with it being wrapped in a SecurityWrapper<Base>. I'll look at that tomorrow, as I'm not very familiar with the wrapping infrastructure.

> So how is it ending up with any sort of Security Wrapper when we pass |true|? Is it a SOW or something? That could explain it. I'm not sure. How do I figure that out?

(In reply to Andrew McCreight [:mccr8] from comment #18) > > So how is it ending up with any sort of Security Wrapper when we pass |true|? Is it a SOW or something? That could explain it. > > I'm not sure. How do I figure that out? obj->dump() in a debugger will tell you if the thing is a proxy. Once you've done that, you can do js::GetProxyHandler(obj), which should hopefully tell you which handler singleton you're dealing with. (In reply to Andrew McCreight [:mccr8] from comment #17) > If we then only > call that JS_WrapObject when js::GetContextCompartment(cx) != > js::GetObjectCompartment(flat), then at least some of the tests start > passing. Ok, then we're almost certainly dealing with a same-compartment SOW then. So this means that a piece of anonymous content is itself getting bound (in untrusted content). This is kinda wacky, and it's worth considering what we actually want to do in that case. We probably just shouldn't set up the class object at all, since the content scope shouldn't be scripting the NAC anyway (and anything over Xrays from the XBL scope should still resolve the appropriate stuff). How about this: We pass |true|, and then detect if the result passes js::IsWrapper. If it is, we just early-return and don't try to set up the prototype object. Does that work?

I filed bug 960320 for the issue of the exception not being handled properly sometimes.

> How about this: We pass |true|, and then detect if the result passes js::IsWrapper. If it is, we just early-return and don't try to set up the prototype object. Nice! That seems to do the trick. With that, the content/events directory passes, except for a few tests that fail even without any patches.

There are still a number of failures. layout/forms/test/test_textarea_resize.html toolkit/content/tests/widgets/test_audiocontrols_dimensions.html toolkit/content/tests/widgets/test_bug898940.html toolkit/content/tests/widgets/test_videocontrols_audio.html chrome/layout/base/tests/chrome/test_printpreview.xul reftest/tests/layout/reftests/bugs/449149-1a.html I'm not sure what is going on. I looked at one of the things, and it seems like another instance of a SOW, as you end up with "JavaScript error: , line 0: permission denied to unwrap object" if you don't bail out if WrapNative returns a wrapper. I tried returning NS_ERROR_XBL_BLOCKED on failure, as the CSS code that is initializing the XBL behaves differently with that error, but it didn't make a difference. I'll try to dig some more.

It looks like your patch returns NS_ERROR_UNEXPECTED. I meant (though didn't specify) in comment 19 to just return NS_OK.

Hmm. Ok. Once I did that, I had to add a null-check on targetClassObject, returning early in that case. Once that's done, then I get this: 0:05.54 [87149] WARNING: value is wrapper, giving up: file /Users/amccreight/mc/dom/xbl/nsXBLProtoImpl.cpp, line 192 [...the "value is wrapper" warning appears dozens of times...] 0:05.54 [87149] WARNING: Not same origin error!: file /Users/amccreight/mc/dom/base/nsJSEnvironment.cpp, line 483 0:05.54 TEST-UNEXPECTED-FAIL | unknown test url | uncaught exception - Script error. at chrome://global/content/bindings/videocontrols.xml:0 0:05.54 JavaScript error: chrome://global/content/bindings/videocontrols.xml, line 969: this.scrubber.valueChanged is not a function Which kind of looks like it is depending on something actually working.

Heh, so it looks like cody is fighting this exact same issue in bug 825392, where he's trying to hoist NAC reflectors into the XBL scope. We already have XBL prototypes for both the content and the XBL scope, and the functions on the content scope are generally just cross-compartment wrappers to the functions in the XBL scope. So I think we're going to need a way to support prototypes that only exist in the XBL scope. I'm going to dig into this for a bit.

I just wanted to let you guys know that I'll be looking this over and getting back to work on bug 825392 here shortly. Last night my net died on me, I have a new ISP and quite a few issues, waiting on a tech but I'm not very hopeful about it. Had this same ISP over 5 years ago and tons of issues then too.

Ok, looking at this more, I think this is something that's going to need to be fixed in cody's patch in bug 825392. I'll comment over there. Andrew, I'd just keep the current behavior here for now and unblock yourself. We should check in on this issue in a month or so to make sure it's sorted out though - can you add that to your sec pokelist?

Sure. I may file a new sec-high bug or something.

This passes in true everywhere except for - nsIDocument::AdoptNode. Eventually, this could become an UncheckedUnwrap or something. - nsXBLProtoImpl::InitTargetObjects. Hopefully this will be fixed by cody's patch. A L64 debug try run for this is green: https://tbpl.mozilla.org/?tree=Try&rev=143a886e539f I'm not sure how exactly to go about landing it. There's some badness this patch is probably fixing, but we've had the public bug sitting around for over a year, and there's nothing the patch reveals in particular beyond that, so I'm tempted to just upload the patch to the public bug, then put it up for review and land on trunk as usual. Then, later in the cycle, I'll nominate for branches, which will expose more that there are probably actual problems.

(In reply to Andrew McCreight [:mccr8] from comment #31) > This passes in true everywhere except for > - nsIDocument::AdoptNode. Eventually, this could become an UncheckedUnwrap > or something. > - nsXBLProtoImpl::InitTargetObjects. Hopefully this will be fixed by cody's > patch. I have definitely taken care of the compartment mismatch, but its a strange situation. Ill be cleaning up some code and trying to post what I have on bug 825392 soon. Somehow with the fix I have right after a comparison like xpc::IsXBLScope(js::GetObjectCompartment(globalObject)) ? js::GetGlobalForObjectCrossCompartment(globalObject) : xpc::GetXBLScope(cx, js::GetGlobalForObjectCrossCompartment(globalObject))); IsXBLScope returns false, but an assertion is hit following the call to GetXBLScope saying that that the scope already is the XBL scope essentially. Ill have a cleaned up version up later today probably, Ive been trying to figure it out and have massive code in place for debugging that has be stripped. My point is we should be good on my end soon.

So, there's basically already a bug on file for this, bug 733636. We probably want to backport it many places, as there's a bunch of gross code in here that this fixes. I was thinking I'd put the patch up for review in the public bug, get it reviewed and landed there, as the patch itself doesn't really reveal anything in particular. Then, later in the cycle, I'd put it up for approval for branches, to reduce the window where it is clear that we think something bad may be going on. Does that sound reasonable, Al?

That works for me.