###!!! ASSERTION: Weird scope returned: 'betterScope == newScope', file /Users/jruderman/trees/mozilla-central/js/xpconnect/src/nsXPConnect.cpp, line 1661 Assertion failure: compartment mismatched, at /Users/jruderman/trees/mozilla-central/js/src/jscntxtinlines.h:268 Guessing this is a regression from CPG (bug 650353).

(In reply to Jesse Ruderman from comment #0) > Guessing this is a regression from CPG (bug 650353). Most likely, yeah. Taking.

So we've got a div with a nontrivial .style. the issue here seems to be that the CSSStyleDeclaration wrapper doesn't get reparented along with the div when we call adoptNode on the div. bz, how is this stuff supposed to work? In general, it looks like our strategy is to recursively call CloneAndAdopt on our children, which (per-spec) moves all the children into the new document: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsNodeUtils.cpp#585 But what about style?

There's no useful spec for what should happen with .style, if nothing else because there's no web-visible way to tell which window the .style is associated with. But yes, we should probably reparent anything that's parented to elements.

Related to bug 752164, bug 752262, bug 752038 and bug 752309? Can this bug be opened up?

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #5) > Related to bug 752164, bug 752262, bug 752038 and bug 752309? Can this bug > be opened up? I thought this was related to bug 752038, but I don't think it is anymore. They might both be exposed by the same issue, but this bug is very much about .style, and there's no .style in bug 752038. These two bugs are my top priority. This bug is a compartment mismatch, so if we care about security on nightly, we should not open it up.

(In reply to Boris Zbarsky (:bz) from comment #4) > But yes, we should probably reparent anything that's parented to elements. There's no way to do this in XPConnect other than walking all the wrappers in the scope and walking the parent chain for each one. So we probably need to do this manually in CloneAndAdopt. I'm assuming we can get the style declaration (if any) for a given node, right? Are there any other non-child objects that can be parented to an element?

Ugh, so we probably mostly got away with this because reparenting an object mostly did't affect anything having that object as a parent or grandparent etc. But now we need to do something to all those other objects :-(. (In reply to Bobby Holley (:bholley) from comment #7) > Are there any other non-child > objects that can be parented to an element? We'll need to check PreCreate hooks and GetParentObject. The various NodeList and HTMLCollection wrappers often have an element as their parent.

Hm, so this could be tricky to fix. Peter says there are objects like NodeList that have an Element as their parent but which the Element doesn't know about. He says we could fix this, but it would be a lot of code. If we can't find all children in CloneAndAdopt, we have the choice of either sweeping the whole compartment (which would be bad of adoptNode/importNode is perf-critical - is it, bz?), or just leaving it in the old scope. If we do the latter, nothing blows up right away, but we get hosed if someone triggers a call to MoveWrappers later on (which is exactly what this bug is about). I'm starting to wonder why we have to move these wrappers at all though, so I filed bug 752446. Once we have an answer over there, maybe the way forward here will be more clear.

Peter, can we just change nodelists to use the element's owner document as the parent? With the possible exception of .childNodes? And perhaps add the owner document to the nodelist cache key? I guess some of the table nodelists would still be a problem. Bobby, a general question here is what should happens when a node moves compartments for various getters on the node that return objects. Seems like they should likewise move compartments....

So I've been thinking about this, and I've now convinced myself that we should stop exposing ReparentWrappedNativeIfFound, and _always_ do a whole-scope sweep (we currently do that already for document.write, but not for adoptNode). And we should change the API so that it's no longer "reparent wrappers from scope A to scope B", but rather "re-query PreCreate for all the wrappers in scope A and fix them up accordingly". That will fix this bug, and possibly some others. The only downside is that it's slower, but jst says we don't really care about the performance of adoptNode, and that a whole-compartment sweep is probably fine.

(In reply to Bobby Holley (:bholley) from comment #11) > but jst says we don't really care about the performance of > adoptNode, and that a whole-compartment sweep is probably fine. 'specially since they are smaller now :)

Can we quantify "slower"? We don't care if it gets somewhat slower; we do care if each adopt operation stats taking seconds (esp. given the implicit-adopt behavior of insertions).

If a page adopts 10 nodes in a row, does that mean 10 sweeps?

(In reply to Boris Zbarsky (:bz) from comment #13) > Can we quantify "slower"? We don't care if it gets somewhat slower; we do > care if each adopt operation stats taking seconds (esp. given the > implicit-adopt behavior of insertions). Yeah, we'll need to measure it. It will be O(n) where there are n wrappers in the scope. Any idea how many that tends to be? (In reply to Jesse Ruderman from comment #14) > If a page adopts 10 nodes in a row, does that mean 10 sweeps? If no node is a descendant of any of the others, yes.

> Any idea how many that tends to be? Order of 1e4 in most cases, with a range of 1e2 to 1e6, I would guess, but worth measuring.

Bobby, do you have any thoughts as to what security rating this should get? It sounds kind of bad to me, but I'm not really sure.

Wow, I'm not sure how I managed to do that. Sorry.

Compartment mismatches are pretty much always sg:crit.

Worked up a solution to this. It's not pretty, but it doesn't kill us performance-wise. Flagging peter for review.

[1:36pm] bholley: CPG means that we now have cross-compartment same-origin stuff [1:36pm] bholley: this was not the norm before, but was possible to create with document.domain [1:37pm] bholley: so most of these CPG bugs are also bugs on all branches, just really hard to find [1:37pm] bholley: I don't think we should bother backporting fixes, but these bugs shouldn't be opened up until the affected branches are unsupported

Editing the status flags conservatively per comment 21.

peter: any chance I could get a review here so that this can land before the branch?

Comment on attachment 626765 [details] [diff] [review] Handle orphaned wrappers. v1 Review of attachment 626765 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine, but I'm a bit confused why we need to do this in ReparentWrappedNativeIfFound. ::: js/xpconnect/src/nsXPConnect.cpp @@ +1511,5 @@ > return UnexpectedFailure(NS_ERROR_FAILURE); > > + // This wrapper could theoretically be an orphan (a wrapper whose parent > + // has moved to another scope), even though it's debatable whether that > + // can ever happen in CloneAndAdopt (the only consumer of this API). To Given that we reparent nodes before their children, won't all the children be orphans here? Why do we rescue them first and then try to reparent them again (in CloneAndAdopt)? @@ +1519,5 @@ > + nsresult rv; > + rv = XPCWrappedNative::GetUsedOnly(ccx, aCOMObj, > + scope, > + XPCNativeInterface::GetISupports(ccx), > + getter_AddRefs(wrapper)); Maybe pass in null for the Interface, so we don't call FindTearoff. @@ +1573,5 @@ > + // that, the wrapper is no longer in the old scope, then we don't need to > + // reparent it. > + MOZ_ASSERT(wrapper->GetScope() == oldScope); > + nsresult rv = wrapper->RescueOrphans(ccx); > + NS_ENSURE_SUCCESS(rv, rv); Can't we do this later on if betterScope != oldScope and betterScope != newScope? Seems like that's the only case where we'd want to rescue?

(In reply to Peter Van der Beken [:peterv] from comment #24) > This looks fine, but I'm a bit confused why we need to do this in > ReparentWrappedNativeIfFound. > Given that we reparent nodes before their children, won't all the children > be orphans here? Why do we rescue them first and then try to reparent them > again (in CloneAndAdopt)? My concern was that we'd run into the same situation as the MoveWrappers case: trying to move |wrapper| from scope A to scope B when |wrapper.parent| long ago moved to scope C. The reason I was dubious about this happening is that this would involve us calling CloneAndAdopt on something whose parent was able to leave it behind (like style). I guessed that CloneAndAdopt was only for nodes (which presumably can't be left behind), but figured we should make the API safe anyway. You're right though that, given the current implementation of CloneAndAdopt, all calls to ReparentWrappedNativeIfFound except for the root node will short-circuit (fixing themselves up with RescueOrphans rather than with the subsequent PreCreate logic). I don't think this hurts anything though, and the orphan logic is probably faster (since it avoids a call to PreCreate). I'm ok to change it though if you want. We just have to make sure that nobody else uses the API. > Maybe pass in null for the Interface, so we don't call FindTearoff. Will do. > Can't we do this later on if betterScope != oldScope and betterScope != > newScope? Seems like that's the only case where we'd want to rescue? Yes, but I wanted to just take care of this definitively before getting into the messy PreCreate/oldnewscope logic. We've had so many security bugs in that code that my eyes start to glaze over when I look at it. Adding yet another "if we didn't get what we expected, try something else" to that stuff seems very unappealing. This check doesn't impose a significant performance penalty (in the common case, it's just a few parent lookups, dwarfed by the brain transplant we do shortly thereafter, and in the uncommon case it saves us the call to precreate), and it's not on the critical path.

(In reply to Bobby Holley (:bholley) from comment #25) > You're right though that, given the current implementation of CloneAndAdopt, > all calls to ReparentWrappedNativeIfFound except for the root node will > short-circuit (fixing themselves up with RescueOrphans rather than with the > subsequent PreCreate logic). I don't think this hurts anything though, and > the orphan logic is probably faster (since it avoids a call to PreCreate). Note that ReparentWrapperIfFound doesn't call PreCreate. I think that's a bug, but means that we could already avoid calling ReparentWrapperIfFound if scope == scope2, but now it's actually worthwhile since it'll happen most of the time?

Actually, we can't avoid ReparentWrapperIfFound in case the current parent is not part of the nodes being adopted (a child node of A doesn't necessarily have A as its parent).

Comment on attachment 626765 [details] [diff] [review] Handle orphaned wrappers. v1 Review of attachment 626765 [details] [diff] [review]: ----------------------------------------------------------------- I do think we should not add the rescuing in ReparentWrappedNativeIfFound, in that case we want to force the wrapper into the new scope (and give it a new parent). We don't care where its old parent is. So moving it with its old parent and then moving it to where the new parent is seems useless. I think it is a bug that we don't call PreCreate from ReparentWrappedNativeIfFound, but if we did we should enforce that the new parent is in the new scope imo.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=026a530f5904 Looks green enough.

Pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/19cfb43d77d3

philor says: Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/40270cf81989 for Windows build failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=12292550&tree=Mozilla-Inbound

I think you have to JS_FRIEND_API IsCrossCompartmentWrapper in jswrapper.h.

Backed out: https://hg.mozilla.org/mozilla-central/rev/40270cf81989

pushed to try with my suggested fix from comment 32: https://tbpl.mozilla.org/?tree=Try&rev=6fa39fa7d645

Updated and pushed to try again: https://tbpl.mozilla.org/?tree=Try&rev=277566781f6a Definitely green this time. Repushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/a6cee80d4fde

Whoops, looks like mccr8 also did a try push. Thanks for the help Andrew! :-)

Verified crash in nightly debug before fix and fix in 6/5 nightly debug.

Bobby, should this get landed on beta? It is being tracked for 14.

I assume the problem is that each patch will have to be heavily changed for each branch? If it is just a matter of landing I can do that. ;)

Heavily changed, generally not. But possibly some rebasing, testing, and try pushing. :-)

This looks like it landed just before the uplift, so the fix is in Fx15? This is primarily CPG bustage so it's good to have it fixed in that same release.