Bug 739796 - Make js_TransplantObjectWithWrapper and GetSameCompartmentSecurityWrapper play nicely together. r=mrbkap
authorBobby Holley <bobbyholley@gmail.com>
Thu, 05 Apr 2012 12:21:12 -0700
changeset 91100 850547d0b635
parent 91099 6a4c590bf1d2
child 91101 fe8ffd7166eb
push id22414
push usermbrubeck@mozilla.com
push date2012-04-06 18:15 +0000
treeherdermozilla-central@a402c2064466 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmrbkap
bugs739796
milestone14.0a1
Bug 739796 - Make js_TransplantObjectWithWrapper and GetSameCompartmentSecurityWrapper play nicely together. r=mrbkap
js/xpconnect/src/XPCWrappedNative.cpp
js/xpconnect/tests/chrome/test_wrappers.xul
js/xpconnect/tests/mochitest/chrome_wrappers_helper.html
--- a/js/xpconnect/src/XPCWrappedNative.cpp
+++ b/js/xpconnect/src/XPCWrappedNative.cpp
@@ -1664,36 +1664,75 @@ XPCWrappedNative::ReparentWrapperIfFound
 
                 JS_SetPrivate(flat, nsnull);
 
                 JSObject *propertyHolder =
                     JS_NewObjectWithGivenProto(ccx, NULL, NULL, aNewParent);
                 if (!propertyHolder || !JS_CopyPropertiesFrom(ccx, propertyHolder, flat))
                     return NS_ERROR_OUT_OF_MEMORY;
 
+                // Before proceeding, eagerly create any same-compartment security wrappers
+                // that the object might have. This forces us to take the 'WithWrapper' path
+                // while transplanting that handles this stuff correctly.
+                {
+                    JSAutoEnterCompartment innerAC;
+                    if (!innerAC.enter(ccx, aOldScope->GetGlobalJSObject()) ||
+                        !wrapper->GetSameCompartmentSecurityWrapper(ccx))
+                        return NS_ERROR_FAILURE;
+                }
+
                 JSObject *ww = wrapper->GetWrapper();
                 if (ww) {
                     JSObject *newwrapper;
                     if (xpc::WrapperFactory::IsLocationObject(flat)) {
                         newwrapper = xpc::WrapperFactory::WrapLocationObject(ccx, newobj);
                         if (!newwrapper)
                             return NS_ERROR_FAILURE;
                     } else {
                         NS_ASSERTION(wrapper->NeedsSOW(), "weird wrapper wrapper");
                         newwrapper = xpc::WrapperFactory::WrapSOWObject(ccx, newobj);
                         if (!newwrapper)
                             return NS_ERROR_FAILURE;
                     }
 
+                    // Ok, now we do the special object-plus-wrapper transplant.
+                    //
+                    // This is some pretty serious brain surgery.
+                    //
+                    // In the case where we wrap a Location object from a same-
+                    // origin compartment, we actually want our cross-compartment
+                    // wrapper to point to the same-compartment wrapper in the
+                    // other compartment. This double-wrapping allows expandos to
+                    // be shared. So our wrapping callback (in WrapperFactory.cpp)
+                    // calls XPCWrappedNative::GetSameCompartmentSecurityWrapper
+                    // before wrapping same-origin Location objects.
+                    //
+                    // This normally works fine, but gets tricky here.
+                    // js_TransplantObjectWithWrapper needs to update the old
+                    // same-compartment security wrapper to be a cross-compartment
+                    // wrapper to the newly transplanted object. So it needs to go
+                    // through the aforementioned double-wrapping mechanism.
+                    // But during the call, things aren't really in a consistent
+                    // state, because mFlatJSObject hasn't yet been updated to
+                    // point to the object in the new compartment.
+                    //
+                    // So we need to cache the new same-compartment security
+                    // wrapper on the XPCWN before the call, so that
+                    // GetSameCompartmentSecurityWrapper can return early before
+                    // getting confused. Hold your breath.
+                    JSObject *wwsaved = ww;
+                    wrapper->SetWrapper(newwrapper);
                     ww = js_TransplantObjectWithWrapper(ccx, flat, ww, newobj,
                                                         newwrapper);
-                    if (!ww)
+                    if (!ww) {
+                        wrapper->SetWrapper(wwsaved);
                         return NS_ERROR_FAILURE;
+                    }
+
                     flat = newobj;
-                    wrapper->SetWrapper(ww);
                 } else {
                     flat = JS_TransplantObject(ccx, flat, newobj);
                     if (!flat)
                         return NS_ERROR_FAILURE;
                 }
 
                 wrapper->mFlatJSObject = flat;
                 if (cache)
@@ -2168,28 +2207,32 @@ XPCWrappedNative::InitTearOffJSObject(XP
 
 JSObject*
 XPCWrappedNative::GetSameCompartmentSecurityWrapper(JSContext *cx)
 {
     // Grab the current state of affairs.
     JSObject *flat = GetFlatJSObject();
     JSObject *wrapper = GetWrapper();
 
+    // If we already have a wrapper, it must be what we want.
+    //
+    // NB: This must come before anything below because of some trickiness
+    // with brain transplants. See the "pretty serious brain surgery" comment
+    // in ReparentWrapperIfFound.
+    if (wrapper)
+        return wrapper;
+
     // Chrome callers don't need same-compartment security wrappers.
     JSCompartment *cxCompartment = js::GetContextCompartment(cx);
     MOZ_ASSERT(cxCompartment == js::GetObjectCompartment(flat));
     if (xpc::AccessCheck::isChrome(cxCompartment)) {
         MOZ_ASSERT(wrapper == NULL);
         return flat;
     }
 
-    // If we already have a wrapper, it must be what we want.
-    if (wrapper)
-        return wrapper;
-
     // Check the possibilities. Note that we need to check for null in each
     // case in order to distinguish between the 'no need for wrapper' and
     // 'wrapping failed' cases.
     if (xpc::WrapperFactory::IsLocationObject(flat)) {
         wrapper = xpc::WrapperFactory::WrapLocationObject(cx, flat);
         if (!wrapper)
             return NULL;
     } else if (NeedsSOW()) {
--- a/js/xpconnect/tests/chrome/test_wrappers.xul
+++ b/js/xpconnect/tests/chrome/test_wrappers.xul
@@ -21,17 +21,17 @@ https://bugzilla.mozilla.org/show_bug.cg
 
   /** Test for Bug 533596 **/
 
   function go() {
     var win = $('ifr').contentWindow;
     var utils = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
                       .getInterface(Components.interfaces.nsIDOMWindowUtils);
     is(utils.getClassName(window), "Proxy", "our window is wrapped correctly")
-    is(utils.getClassName(location), "Proxy", "our location is wrapped correctly")
+    is(utils.getClassName(location), "Location", "chrome doesn't have location wrappers")
     is(utils.getClassName(win), "Proxy", "win is an Proxy");
     is(utils.getClassName(win.location), "Proxy", "deep wrapping works");
     is(win.location.href, "http://example.org/tests/js/xpconnect/tests/mochitest/chrome_wrappers_helper.html",
        "can still get strings out");
 
     var unsafeWin = win.wrappedJSObject;
     is(utils.getClassName(unsafeWin), "Proxy", "can get a Proxy");
     is(utils.getClassName(unsafeWin.location), "Proxy", "deep wrapping works");
--- a/js/xpconnect/tests/mochitest/chrome_wrappers_helper.html
+++ b/js/xpconnect/tests/mochitest/chrome_wrappers_helper.html
@@ -15,16 +15,19 @@
             }
             function run_test(ok, xpcnw, sjow) {
                 // both wrappers should point to our window: XOW
                 check_wrapper(ok, ok, "Proxy", "functions are wrapped properly")
                 check_parent(ok, ok, window, "ok is parented correctly");
                 check_wrapper(ok, xpcnw, "Proxy", "XPCNWs are transformed correctly");
                 check_wrapper(ok, sjow, "Proxy", "SJOWs are transformed correctly");
 
+                check_wrapper(ok, window.location, "Proxy",
+                              "Content needs a same-compartment security wrappers around location");
+
                 ok(defprop1 === 1, "defprop1 exists");
                 window.defprop1 = 2;
                 ok(defprop1 === 2, "defprop1 is properly writable");
 
                 // defprop2 = {}; disabled because the test doesn't work
             }
         </script>
     </head>