751995 - Compartment mismatch with adoptNode, style

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: testcase (asserts fatally when loaded)

###!!! 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).

Comment 1

[Jesse Ruderman]

Attachment: stack traces

Comment 2

[Bobby Holley (:bholley)]

(In reply to Jesse Ruderman from comment #0)

> Guessing this is a regression from CPG (bug 650353).

Most likely, yeah. Taking.

Comment 3

[Bobby Holley (:bholley)]

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?

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Martijn Wargers (dead)]

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

Comment 6

[Bobby Holley (:bholley)]

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

Comment 7

[Bobby Holley (:bholley)]

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

Comment 8

[Peter Van der Beken [:peterv]]

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.

Comment 9

[Bobby Holley (:bholley)]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[Bobby Holley (:bholley)]

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.

Comment 12

[Luke Wagner [:luke]]

(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 :)

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Jesse Ruderman]

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

Comment 15

[Bobby Holley (:bholley)]

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

Comment 16

[Boris Zbarsky [:bzbarsky]]

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

Comment 17

[Andrew McCreight [:mccr8]]

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.

Comment 18

[Andrew McCreight [:mccr8]]

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

Comment 19

[Bobby Holley (:bholley)]

Compartment mismatches are pretty much always sg:crit.

Comment 20

[Bobby Holley (:bholley)]

Attachment: Handle orphaned wrappers. v1

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

Comment 21

[Andrew McCreight [:mccr8]]

[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

Comment 22

[Bobby Holley (:bholley)]

Editing the status flags conservatively per comment 21.

Comment 23

[Bobby Holley (:bholley)]

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

Comment 24

[Peter Van der Beken [:peterv]]

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?

Comment 25

[Bobby Holley (:bholley)]

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

Comment 26

[Peter Van der Beken [:peterv]]

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

Comment 27

[Peter Van der Beken [:peterv]]

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 28

[Peter Van der Beken [:peterv]]

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.

Comment 29

[Bobby Holley (:bholley)]

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

Looks green enough.

Comment 30

[Bobby Holley (:bholley)]

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

Comment 31

[Mats Palmgren (inactive)]

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

Comment 32

[Andrew McCreight [:mccr8]]

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

Comment 33

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/19cfb43d77d3

Comment 34

[(no longer active)]

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

Comment 35

[Andrew McCreight [:mccr8]]

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

Comment 36

[Bobby Holley (:bholley)]

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

Comment 37

[Bobby Holley (:bholley)]

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

Comment 38

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/a6cee80d4fde

Comment 39

[Al Billings [:abillings - ex-MoCo]]

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

Comment 40

[Andrew McCreight [:mccr8]]

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

Comment 42

[Andrew McCreight [:mccr8]]

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. ;)

Comment 43

[Bobby Holley (:bholley)]

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

Comment 45

[Daniel Veditz [:dveditz]]

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.