795532 - Inconsistency in cross-compartment map

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

I think I found a problem at the end of JS_TransplantObject:

    if (origobj->compartment() != destination) {
        RootedObject newIdentityWrapper(cx, newIdentity);
        AutoCompartment ac(cx, origobj);
        if (!JS_WrapObject(cx, newIdentityWrapper.address()))
            return NULL;
        if (!origobj->swap(cx, newIdentityWrapper))
            return NULL;
        origobj->compartment()->crossCompartmentWrappers.put(ObjectValue(*newIdentity), origv);
    }

We have some object O, we create a wrapper W for it, then we swap the contents of W with some other object W'. Finally, we need to update the CCW map to say that W' is the wrapper for O, rather than W.

However, when we're creating the wrapper W, it may be that W doesn't directly wrap O. That's because JSCompartment::wrap first unwraps O before wrapping it. So it may be that W is really a wrapper for some other object O'.

The problem is that we still update the wrapper map to say that W' is a wrapper for O. It seems like we should really say that it's a wrapper for O'. That's what JSCompartment::wrap does, at least.

Comment 1

[Bobby Holley (:bholley)]

Comment on attachment 666137 [details] [diff] [review]
patch

Hm, so the two places in Gecko where we use a same-compartment wrapper as an identity object are (1) Xray waivers and (2) outer windows. For (1), we never call transplant. For (2), we're saved by the fact that JSCompartment::wrap passes stopAtOuter=true to Unwrap(), meaning that we don't actually unwrap the outer window proxy.

So is the concern in:
> +        // JS_WrapObject isn't guaranteed to return a wrapper for
> +        // newIdentity. It might end up wrapping a slightly different object.

Purely theoretical?

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

OK, I wasn't sure if this could actually happen. I found the issue by mucking with a test case, and it asserted unexpectedly.

I still think it would be better to change the behavior here. It doesn't cost us anything, and what we're doing right now seems like it might cause problems in the future. I can change the comment or something to better reflect reality.

Comment 3

[Bobby Holley (:bholley)]

So, after clarifying this in my head a little bit:

The contract we have is that the Prewrap callback returns the identity object as far as the cross-compartment wrapper map is concerned. For outer windows, we handle this by just never unwrapping the outer window in the first place (stopAtOuter = false). For Xray Waivers, we explicitly rewrap the object in a waiver during the prewrap callback (giving us the appropriate separate identity).

I'm pretty sure stuff will break if these invariants aren't true. So I think we should actually be asserting that the before and after versions of this patch give the same results. That is to say, that key == ObjectValue(*newIdentity). Does that make sense?

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: assertion patch

Sounds good to me.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/67728fe694e2

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/67728fe694e2