739796 - Make same-origin cross-compartment location expandos visible in a compartment-per-global world

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Split off from bug 733984.

Comment 1

[Bobby Holley (:bholley)]

Attachment: part 1 - Move same-compartment security wrapping into a method on XPCWrappedNative. v1

First, we do some refactoring.

Comment 2

[Bobby Holley (:bholley)]

Attachment: part 2 - Make same-origin cross-compartment Location object access go through the LW in the host compartment. v2

Then, we fix the bug.

Comment 3

[Bobby Holley (:bholley)]

Darn, this turned the whole tree orange:

https://tbpl.mozilla.org/?tree=Try&rev=f886d566a87a

Looks like I didn't properly handle the interaction with TransplantObjectWithWrapper:

#0  0x0000000100030fed in MOZ_Crash () at Assertions.cpp:76
#1  0x000000010003104f in MOZ_Assert "cxCompartment == js::GetObjectCompartment(flat)" Assertions.cpp:88
#2  0x0000000103c941d4 in XPCWrappedNative::GetSameCompartmentSecurityWrapper XPCWrappedNative.cpp:2160
#3  0x0000000103d8d4e1 in xpc::WrapperFactory::Rewrap WrapperFactory.cpp:395
#4  0x0000000104adaeb3 in JSCompartment::wrap jscompartment.cpp:314
#5  0x0000000104adb2fc in JSCompartment::wrap jscompartment.cpp:357
#6  0x0000000104a94c03 in JS_WrapObject jsapi.cpp:1493
#7  0x0000000104a956ba in js_TransplantObjectWithWrapper jsapi.cpp:1691
#8  0x0000000103c987ba in XPCWrappedNative::ReparentWrapperIfFound XPCWrappedNative.cpp:1669

I'll look at it more tomorrow.

Comment 4

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

Comment on attachment 609924 [details] [diff] [review]
part 1 - Move same-compartment security wrapping into a method on XPCWrappedNative. v1

Review of attachment 609924 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCConvert.cpp
@@ +1088,4 @@
>  
> +        // Outerize any inner windows.
> +        flat = JS_ObjectToOuterObject(cx, flat);
> +        NS_ASSERTION(flat, "bad outer object hook!");

MOZ_ASSERT?

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +2174,5 @@
> +        wrapper = xpc::WrapperFactory::WrapLocationObject(cx, flat);
> +        if (!wrapper)
> +            return NULL;
> +    }
> +    else if (NeedsSOW()) {

'} else if'?

::: js/xpconnect/src/xpcprivate.h
@@ +2707,5 @@
> +    // mFlatJSObject otherwise.
> +    //
> +    // This takes care of checking mWrapperWord to see if we already have such
> +    // a wrapper.
> +    JSObject* GetSameCompartmentSecurityWrapper(JSContext *cx);

JSObject *GetSameCompartmentSecurityWrapper

Comment 5

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

Comment on attachment 609925 [details] [diff] [review]
part 2 - Make same-origin cross-compartment Location object access go through the LW in the host compartment. v2

Review of attachment 609925 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +387,5 @@
> +            // Do the double-wrapping if need be.
> +            if (IsLocationObject(obj)) {
> +                JSAutoEnterCompartment ac;
> +                if (!ac.enter(cx, obj))
> +                    return nsnull;

Null here

@@ +390,5 @@
> +                if (!ac.enter(cx, obj))
> +                    return nsnull;
> +                XPCWrappedNative *wn = GetWrappedNative(cx, obj);
> +                if (!wn)
> +                    return false;

And false here?

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 609924 [details] [diff] [review]
part 1 - Move same-compartment security wrapping into a method on XPCWrappedNative. v1

f=mrbkap with the same nits as Ms2ger.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 609925 [details] [diff] [review]
part 2 - Make same-origin cross-compartment Location object access go through the LW in the host compartment. v2

With the benefit of hindsight, f- because this doesn't handle the ReparentWrapperIfFound case ;-)

I'm not sure how to fix the broken mochitests without loosening the assertion somehow, though.

Comment 8

[Bobby Holley (:bholley)]

Ugh, why do we even need to brain transplant location objects anyway? Couldn't we just null out any references to it, or replace it with the location object in the new scope? I guess maybe that wouldn't be much simpler...

Do brain transplants preserve expandos?

Comment 9

[Bobby Holley (:bholley)]

Attachment: part 1.5: Make js_TransplantObjectWithWrapper and GetSameCompartmentSecurityWrapper play nicely together. v1

Worked up a fix for the brain transplant stuff. Flagging mrbkap for review.

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

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 611951 [details] [diff] [review]
part 1.5: Make js_TransplantObjectWithWrapper and GetSameCompartmentSecurityWrapper play nicely together. v1

Holding my breath. :-)

Comment 11

[Bobby Holley (:bholley)]

Attachment: part 1.5 - Make js_TransplantObjectWithWrapper and GetSameCompartmentSecurityWrapper play nicely together. v2

Sigh, this is getting comical.

First of all, I hadn't noticed that we had test coverage that required SCSWs for
chrome. This seemed wrong to me, so I fixed it. Test changes included in this patch.

Additionally, the last patch caused failures when we hadn't ever done the lazy SCSW
generation before reparenting this wrapper. this meant that the SCSW really
did need to be created during the transplant, which failed for the same reasons.

This whole situation is a bit of a footgun for anyone else who ever tries to transplant
objects with SCSWs (though really, that's not exactly an easy thing regardless).
I'm sort of tempted just to get rid of the lazy generation entirely and _always_
generate SCSWs on XPCWN instantiation. That would get rid of this whole mess entirely,
and simplify the code. It would be a bit wasteful though, since we'd be generating
possibly-unnecessary SCSWs during cross-compartment cross-origin access. I'm not sure
if that's a huge deal, though.

Your call, blake. We can go the route of this patch, or I can rejigger and simplify all
this stuff so that we just create the SCSWs in XPCWN::GetNewOrUsed, and then we can just
grab them during wrapping, and not worry about when we can and can't create SCSWs during
cross-compartment wrapping.

Comment 12

[Bobby Holley (:bholley)]

Oh for f's sake...

So in my last try push ( https://tbpl.mozilla.org/?tree=Try&rev=bf79403e10bf ), it turned out that we aren't guaranteed to be in a given compartment (either the new or the old) during XPCWrappedNative::ReparentWrapperIfFound. Fixing that.

I pushed to try again here: https://tbpl.mozilla.org/?tree=Try&rev=ff772c14adc0

Given the track record, I'll avoid flagging for review again until we can get all the way through the unit tests without crashing during wrapper reparenting :\

Sorry for the churn, blake.

Comment 13

[Bobby Holley (:bholley)]

Attachment: part 1.5 - Make js_TransplantObjectWithWrapper and GetSameCompartmentSecurityWrapper play nicely together. v3

Comment 14

[Bobby Holley (:bholley)]

Comment on attachment 612116 [details] [diff] [review]
part 1.5 - Make js_TransplantObjectWithWrapper and GetSameCompartmentSecurityWrapper play nicely together. v3

_finally_.

Comment 15

[Bobby Holley (:bholley)]

Pushed to try (without the other cpg baggage): https://tbpl.mozilla.org/?tree=Try&rev=644dd789143e

Comment 16

[Bobby Holley (:bholley)]

Looks green modulo some pre-existing orange from inbound. Pushed to m-i:

http://hg.mozilla.org/integration/mozilla-inbound/rev/fe8ffd7166eb
http://hg.mozilla.org/integration/mozilla-inbound/rev/850547d0b635
http://hg.mozilla.org/integration/mozilla-inbound/rev/6a4c590bf1d2

Comment 17

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/6a4c590bf1d2
https://hg.mozilla.org/mozilla-central/rev/850547d0b635
https://hg.mozilla.org/mozilla-central/rev/fe8ffd7166eb