731471 - Don't reparent wrappers that don't want to move

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

I thought this was a bug when reading this code long ago, and now I've finally hit it with compartment-per-global.

In the wrapper reparenting code, we call PreCreate on the object to see what parent it wants. If it directly wants the old global, we leave it be. But if it just wants something in the old scope, we reparent _that_ thing first, and then continue.

Unfortunately, we don't check whether the parent actually moved scopes, but just assume that it did. So we don't handle the case where parent wants to stay in the old scope. Instead, we proceed along our merry way, and, if the two scopes are in different compartments, hit a compartment mismatch assertion.

This stuff is kind of a mess, but I don't think it's worth the effort to refactor, since it'll probably go away once the new bindings settle.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Don't reparent wrappers that don't want to move. v1

Attaching a patch. Flagging peterv for review.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

MOZ_ASSERT, perhaps?

Comment 3

[Bobby Holley (:bholley)]

(In reply to Ms2ger from comment #2)
> MOZ_ASSERT, perhaps?

I want this one to be nonfatal. I can imagine it firing during automation or something, and I'd rather not have to drop everything to fix it.

Comment 4

[Peter Van der Beken [:peterv]]

Comment on attachment 601477 [details] [diff] [review]
Don't reparent wrappers that don't want to move. v1

Please add the crashtest from bug 716383.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Peter Van der Beken [:peterv] from comment #4)
> Please add the crashtest from bug 716383.

So, the test relies on window.open, which doesn't work in crashtests (reftests can't open new windows). It's also a non-fatal assertion (and not one that we clearly want to make fatal), so we can't use a mochitest either. I tried rewriting the testcase to use an iframe, but I couldn't get the assertion to fire.

So, barring any further directives from peter, I'm just going to check this in without the test. I've manually verified that it does indeed fix it, though.

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

Comment 7

[Bobby Holley (:bholley)]

Looks green. Pushed to m-i:
http://hg.mozilla.org/integration/mozilla-inbound/rev/7609e085d456

Comment 8

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/7609e085d456

Comment 9

[Jesse Ruderman]

> It's also a non-fatal assertion (and not one that we clearly want to make fatal), 
> so we can't use a mochitest either.

Please check in a mochitest and add your +1 to bug 404077.  It's worth adding the test even if it won't catch an exact regression right away.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Jesse Ruderman from comment #9)
> > It's also a non-fatal assertion (and not one that we clearly want to make fatal), 
> > so we can't use a mochitest either.
> 
> Please check in a mochitest and add your +1 to bug 404077.  It's worth
> adding the test even if it won't catch an exact regression right away.

https://hg.mozilla.org/integration/mozilla-inbound/rev/aca558fd4bfb

Comment 11

[Phil Ringnalda (:philor)]

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