Last Comment Bug 739796 - Make same-origin cross-compartment location expandos visible in a compartment-per-global world
: Make same-origin cross-compartment location expandos visible in a compartment...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 760118
Blocks: cpg
  Show dependency treegraph
 
Reported: 2012-03-27 14:46 PDT by Bobby Holley (:bholley) (busy with Stylo)
Modified: 2012-06-05 06:12 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1 - Move same-compartment security wrapping into a method on XPCWrappedNative. v1 (6.19 KB, patch)
2012-03-27 15:42 PDT, Bobby Holley (:bholley) (busy with Stylo)
gal: review+
mrbkap: feedback+
Details | Diff | Splinter Review
part 2 - Make same-origin cross-compartment Location object access go through the LW in the host compartment. v2 (4.14 KB, patch)
2012-03-27 15:42 PDT, Bobby Holley (:bholley) (busy with Stylo)
gal: review+
mrbkap: feedback-
Details | Diff | Splinter Review
part 1.5: Make js_TransplantObjectWithWrapper and GetSameCompartmentSecurityWrapper play nicely together. v1 (4.68 KB, patch)
2012-04-03 13:19 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
part 1.5 - Make js_TransplantObjectWithWrapper and GetSameCompartmentSecurityWrapper play nicely together. v2 (8.17 KB, patch)
2012-04-03 17:41 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
part 1.5 - Make js_TransplantObjectWithWrapper and GetSameCompartmentSecurityWrapper play nicely together. v3 (8.32 KB, patch)
2012-04-04 00:02 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review

Description Bobby Holley (:bholley) (busy with Stylo) 2012-03-27 14:46:32 PDT
Split off from bug 733984.
Comment 1 Bobby Holley (:bholley) (busy with Stylo) 2012-03-27 15:42:22 PDT
Created attachment 609924 [details] [diff] [review]
part 1 - Move same-compartment security wrapping into a method on XPCWrappedNative. v1

First, we do some refactoring.
Comment 2 Bobby Holley (:bholley) (busy with Stylo) 2012-03-27 15:42:58 PDT
Created attachment 609925 [details] [diff] [review]
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) (busy with Stylo) 2012-03-27 22:50:04 PDT
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 (⌚ UTC+1/+2) 2012-03-28 02:37:02 PDT
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 (⌚ UTC+1/+2) 2012-03-28 02:39:02 PDT
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) 2012-03-28 07:18:30 PDT
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) 2012-03-28 07:26:59 PDT
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) (busy with Stylo) 2012-03-28 08:47:00 PDT
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) (busy with Stylo) 2012-04-03 13:19:55 PDT
Created attachment 611951 [details] [diff] [review]
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) 2012-04-03 14:13:41 PDT
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) (busy with Stylo) 2012-04-03 17:41:52 PDT
Created attachment 612044 [details] [diff] [review]
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) (busy with Stylo) 2012-04-04 00:01:42 PDT
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) (busy with Stylo) 2012-04-04 00:02:04 PDT
Created attachment 612116 [details] [diff] [review]
part 1.5 - Make js_TransplantObjectWithWrapper and GetSameCompartmentSecurityWrapper play nicely together. v3
Comment 14 Bobby Holley (:bholley) (busy with Stylo) 2012-04-04 08:53:21 PDT
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) (busy with Stylo) 2012-04-05 10:13:46 PDT
Pushed to try (without the other cpg baggage): https://tbpl.mozilla.org/?tree=Try&rev=644dd789143e
Comment 16 Bobby Holley (:bholley) (busy with Stylo) 2012-04-05 12:26:32 PDT
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

Note You need to log in before you can comment on or make changes to this bug.