752038 - "ASSERTION: wrapper already in new scope!" with location.hash, document.open

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Jesse Ruderman]

Attachment: testcase (inner)

To reproduce, save both testcase files, then open outer.html.

###!!! ASSERTION: wrapper already in new scope!: '!newMap->Find(wrapper->GetIdentityObject())', file js/xpconnect/src/XPCWrappedNative.cpp, line 1636

Might be a regression from cpg.

Comment 1

[Jesse Ruderman]

Attachment: testcase (outer)

Comment 2

[Jesse Ruderman]

Attachment: stack trace

Comment 3

[Bobby Holley (:bholley)]

Probably the same issue as bug 751995. Taking.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #3)
> Probably the same issue as bug 751995. Taking.

They appear to be different issues. I'm looking into this one now.

Comment 5

[Robert Kaiser]

Bug 752164 comment #1 says those two might be dupes, should the crash signature be copied in here as well?
In that case, bug 752309 has another testcase and the same signature.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Robert Kaiser (:[e-mail]) from comment #5)
> Bug 752164 comment #1 says those two might be dupes, should the crash
> signature be copied in here as well?
> In that case, bug 752309 has another testcase and the same signature.

Let's keep using the other bug to track the crashes until we're sure that this is the same issue.

Comment 7

[Bobby Holley (:bholley)]

Attachment: reduced testcase

Attaching a simpler, reduced testcase.

Comment 8

[Bobby Holley (:bholley)]

So here's what's going on. This is the #1 topcrasher on Nightly, so there's a moderate amount of urgency here.

We do a dummy hash navigation, which causes a Location wrapper to be created for the underlying DOM object. We then do a document.write, which gives us a new scope and forces everything, including the Location object, to be reparented to the new scope.

During this reparenting, we iterate over all the wrappers. We happen to hit the |Document| first. Post-CPG this operation crosses compartment boundaries, so we do a brain transplant, and copy all of the properties from the JS object across the compartment (including document.location). This causes us to do a call wrap() on document.location, which takes us into PrepareForWrapping. This calls PreCreate on native nsLocation object in order to figure out if it needs to make a cross-compartment wrapper.

nsLocationSH::PreCreate then examines the docshell in order to find the outer window (a particular oddity of Location objects is that they're parented to the outer window for security reasons). But the outer window has already been brain transplanted into the new scope. So PreCreate informs PrepareForWrapping that the object ought to live in the new scope, and PrepareForUnwrapping dutifully creates a new XPCWN for it in the new scope.

But now we're screwed, because we have two wrappers for the same object (one in each scope). We continue along our merry way in MoveWrappers, until we hit the XPCWN for the Location object in the old scope. But by this point, we've already got an XPCWN for the underlying native in the new scope. So we assert, pooch the hash table, and (sometimes) crash a bit later on.

There are a couple of things we could do to fix this. We could make the JS_CopyPropertiesFrom stuff be more careful about this, but I can't think of a great way to do that. My ideal approach, I think, is to make nsLocation implement nsWrapperCache, and change PrepareForWrapping to check the wrapper cache (and defer to it) before calling PreCreate.

So the only big question here is if there are any gotchas surrounding wrapper-caching Location. Anybody have any?

Comment 9

[Bobby Holley (:bholley)]

CCing jst, since he might be a good candidate to advise about wrapper-caching nsLocation.

Comment 10

[Bobby Holley (:bholley)]

So, it seems that I was slightly wrong about how things fall apart at the end. I just investigated the testcase over in bug 752309, which doesn't involve nsLocation, but rather nsDOMStringMap.

What happens is that, after transplanting wrappers, we null out their private (since the XPCWN is gone, and all that's left is the shell JS object). So in this case, the DOMStringMap gets transplanted first (before the location), at which point the expando on |document| is bogus.

nsDOMStringMapSH::PreCreate just tries to parent the object to the window associated with the document's scope. I guess the document has already changed windows by this point, so this probably isn't unique to location. Maybe another solution is in order.

Comment 11

[Bobby Holley (:bholley)]

So, in a sense, there are two bugs here that occur during MoveWrappers, depending on whether we first encounter an object as a wrapper-to-move or as a property on an object being transplanted.

If the first is true, then we end up nulling out the private of the JS object once we move the XPCWN over to the new scope, which causes us to crash if we subsequently encounter a reference to the same object defined on the object upon which we call JS_CopyPropertiesFrom. I think we can probably fix this by replacing the neutered object with a cross-compartment wrapper to the new object (Hopefully mrbkap writes that API comment for JS_TransplantObject soon ;-) ).

If the second is true, things are a little trickier. When JS_CopyPropertiesFrom calls wrap, it ends up calling PreCreate in PrepareForWrapping (as mentioned in comment 8), and PreCreate jumps the gun a little bit and returns the new scope. The only way I can think of to fix this in our current architecture is, as suggested before, to have PrepareForUnwrapping defer to the wrapper cache.

But now we're not just talking about Location anymore. We're talking about inheriting nsWrapperCache for every object that implements PreCreate. This might not be so much work, and we can probably assert this invariant in XPConnect whenever we find WANT_PRECREATE on a set of scriptable flags. But there might also be snakes in the grass.

So, I'd appreciate input from jst, mrbkap, or peterv about the prospect of requiring nsWrapperCache for anything that implements PreCreate. It seems like it would make sense, but I may not have the whole picture here.

Comment 12

[Bobby Holley (:bholley)]

So, I realized that it might not make sense for all PreCreate objects to implement nsWrapperCache. Specifically, there are some objects that _sometimes_ specify a parent when calling PreCreate, but default to the global in certain cases. It's not totally clear to me whether this is just an exceptional/error case or whether it's a valid code path, but if the latter is true then certain objects with PreCreate hooks might sometimes have multiple wrappers. Ugh.

So I think we might just want to stick global state on XPCJSRuntime that says "are we reparenting wrappers right now?" If we are, we can just unconditionally return from the PreWrap callback for everything that has a PreCreate hook. This isn't totally correct, per se, but it's probably good enough. I'll look into doing that once I get all this same-compartment-security-wrapper stuff sorted out.

Comment 13

[Bobby Holley (:bholley)]

So, js_TransplantObjectWithWrapper is supposed to handle the first case in comment 11. But unfortunately we crash if anyone gets around SCSWs. Which happens any time you call wrap() on an SCSW, which we do during wrapper reparenting. This is also bug 754044, which I just filed.

Comment 14

[Bobby Holley (:bholley)]

Attachment: Avoid getting confused by PreCreate giving a different answer when we wrap objects cross-compartment during reparenting. v1

Blake and I discussed this on IRC, and ended up deciding to go with an ugly member variable on XPCJSRuntime. But then I had a very clever idea that I'm quite proud of. Hope you approve, Blake. ;-)

Comment 15

[Bobby Holley (:bholley)]

Attachment: Avoid getting confused by PreCreate giving a different answer when we wrap objects cross-compartment during reparenting. v2

Fixed a few typos in the comment.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 624506 [details] [diff] [review]
Avoid getting confused by PreCreate giving a different answer when we wrap objects cross-compartment during reparenting. v2

Phew!

Comment 17

[Bobby Holley (:bholley)]

pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/3bfee91d5f09

I thought I'd pushed this to try, but I can't find the link anywhere. It's unlikely that this would break anything, but if it does I apologize. :-(

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3bfee91d5f09