If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 15

Status

()

Core
XPConnect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: bholley)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla15
x86_64
Mac OS X
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox15 fixed, firefox-esr10 unaffected)

Details

(Whiteboard: [advisory-tracking+])

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 621165 [details]
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.
(Reporter)

Comment 1

6 years ago
Created attachment 621166 [details]
testcase (outer)
(Reporter)

Comment 2

6 years ago
Created attachment 621168 [details]
stack trace
Probably the same issue as bug 751995. Taking.
Assignee: nobody → bobbyholley+bmo

Updated

5 years ago
Blocks: 752309
(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

5 years ago
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.
(In reply to Robert Kaiser (:kairo@mozilla.com) 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.
Created attachment 621987 [details]
reduced testcase

Attaching a simpler, reduced testcase.
Attachment #621165 - Attachment is obsolete: true
Attachment #621166 - Attachment is obsolete: true
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?
CCing jst, since he might be a good candidate to advise about wrapper-caching nsLocation.
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.
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.
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.
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.
Blocks: 752150
Created attachment 624504 [details] [diff] [review]
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. ;-)
Attachment #624504 - Flags: review?(mrbkap)
Created attachment 624506 [details] [diff] [review]
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.
Attachment #624506 - Flags: review?(mrbkap)
Attachment #624504 - Attachment is obsolete: true
Attachment #624504 - Flags: review?(mrbkap)
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!
Attachment #624506 - Flags: review?(mrbkap) → review+
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. :-(
Flags: in-testsuite+
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/3bfee91d5f09
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox15: --- → fixed
Resolution: --- → FIXED
No longer blocks: 752150

Updated

5 years ago
status-firefox-esr10: --- → unaffected
Whiteboard: [advisory-tracking+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.