Bug 1530146 part 1. Switch XrayWaiver to always being same-realm with its target. r=bholley
authorBoris Zbarsky <bzbarsky@mit.edu>
Fri, 01 Mar 2019 02:54:41 +0000
changeset 519800 fe2cba661d5eae557c0d7611bb5442fc0bfc02a2
parent 519736 c1075c1f1605435c69a6964895600db8a711fbb5
child 519801 dadc02e71d59e18fb6829fa83509b38ea0acc58c
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbholley
bugs1530146
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1530146 part 1. Switch XrayWaiver to always being same-realm with its target. r=bholley Differential Revision: https://phabricator.services.mozilla.com/D21481
js/xpconnect/src/XPCMaps.h
js/xpconnect/tests/chrome/chrome.ini
js/xpconnect/tests/chrome/file_bug1530146.html
js/xpconnect/tests/chrome/file_bug1530146_inner.html
js/xpconnect/tests/chrome/test_bug1530146.html
js/xpconnect/wrappers/WrapperFactory.cpp
js/xpconnect/wrappers/WrapperFactory.h
--- a/js/xpconnect/src/XPCMaps.h
+++ b/js/xpconnect/src/XPCMaps.h
@@ -493,17 +493,19 @@ class JSObject2JSObjectMap {
     return nullptr;
   }
 
   /* Note: If the entry already exists, return the old value. */
   inline JSObject* Add(JSContext* cx, JSObject* key, JSObject* value) {
     MOZ_ASSERT(key, "bad param");
     Map::AddPtr p = mTable.lookupForAdd(key);
     if (p) {
-      return p->value();
+      JSObject* oldValue = p->value();
+      p->value() = value;
+      return oldValue;
     }
     if (!mTable.add(p, key, value)) {
       return nullptr;
     }
     MOZ_ASSERT(xpc::CompartmentPrivate::Get(key)->scope->mWaiverWrapperMap ==
                this);
     return value;
   }
--- a/js/xpconnect/tests/chrome/chrome.ini
+++ b/js/xpconnect/tests/chrome/chrome.ini
@@ -113,8 +113,10 @@ skip-if = os == 'win' || os == 'mac' || 
 [test_sharedChromeCompartment.html]
 [test_weakmap_keys_preserved.xul]
 [test_weakmap_keys_preserved2.xul]
 [test_weakref.xul]
 [test_windowProxyDeadWrapper.html]
 [test_wrappers.xul]
 [test_xrayic.xul]
 [test_xrayToJS.xul]
+[test_bug1530146.html]
+support-files = file_bug1530146.html file_bug1530146_inner.html
new file mode 100644
--- /dev/null
+++ b/js/xpconnect/tests/chrome/file_bug1530146.html
@@ -0,0 +1,5 @@
+<!DOCTYPE html>
+<script>
+  document.domain = document.domain;
+</script>
+<iframe></iframe>
new file mode 100644
--- /dev/null
+++ b/js/xpconnect/tests/chrome/file_bug1530146_inner.html
@@ -0,0 +1,4 @@
+<!DOCTYPE html>
+<script>
+  var obj = { a: "hello" }
+</script>
new file mode 100644
--- /dev/null
+++ b/js/xpconnect/tests/chrome/test_bug1530146.html
@@ -0,0 +1,58 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1530146
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 1530146</title>
+  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="chrome://global/skin"/>
+  <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css"/>
+  <script type="application/javascript">
+
+  /** Test for Bug 1530146 **/
+  SimpleTest.waitForExplicitFinish();
+
+  addLoadEvent(setupTest);
+
+  var sb;
+
+  function setupTest() {
+    // Create a sandbox with an expanded principal for our iframe.
+    sb = new Cu.Sandbox([frames[0].document.nodePrincipal],
+                        {sandboxPrototype: frames[0]});
+    // Grab a waiver for the subframe in the sandbox to make sure the waiver
+    // stays alive.  It would be nice if we could just use waiveXray in the
+    // sandbox: https://bugzilla.mozilla.org/show_bug.cgi?id=1531614
+    Cu.evalInSandbox('this.waiver = document.querySelector("iframe").contentWindow.wrappedJSObject',
+                     sb);
+    var ifr = frames[0].document.querySelector("iframe");
+    ifr.onload = doTest;
+    ifr.src = "file_bug1530146_inner.html";
+  }
+
+function doTest() {
+    // Create a new sandbox for the iframe's subframe
+    var sb2 = new Cu.Sandbox([frames[0][0].document.nodePrincipal],
+                             {sandboxPrototype: frames[0][0]});
+    // Reget the waiver; this is where things can go awry.
+    Cu.evalInSandbox('this.waiver = window.wrappedJSObject', sb2);
+    is(Cu.evalInSandbox("this.waiver.obj.a", sb2), "hello",
+       "Should get the right value and not crash");
+    is(Cu.evalInSandbox("(new this.waiver.Image()).localName", sb2), "img",
+       "Should create an image and not crash");
+    SimpleTest.finish();
+  }
+  </script>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1530146">Mozilla Bug 1530146</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+<iframe src="http://mochi.test:8888/chrome/js/xpconnect/tests/chrome/file_bug1530146.html"></iframe>
+</div>
+<pre id="test">
+</pre>
+</body>
+</html>
--- a/js/xpconnect/wrappers/WrapperFactory.cpp
+++ b/js/xpconnect/wrappers/WrapperFactory.cpp
@@ -201,20 +201,22 @@ JSObject* WrapperFactory::GetXrayWaiver(
 
   if (!scope->mWaiverWrapperMap) {
     return nullptr;
   }
 
   return scope->mWaiverWrapperMap->Find(obj);
 }
 
-JSObject* WrapperFactory::CreateXrayWaiver(JSContext* cx, HandleObject obj) {
-  // The caller is required to have already done a lookup.
+JSObject* WrapperFactory::CreateXrayWaiver(JSContext* cx, HandleObject obj,
+                                           bool allowExisting) {
+  // The caller is required to have already done a lookup, unless it's
+  // trying to replace an existing waiver.
   // NB: This implictly performs the assertions of GetXrayWaiver.
-  MOZ_ASSERT(!GetXrayWaiver(obj));
+  MOZ_ASSERT(bool(GetXrayWaiver(obj)) == allowExisting);
   XPCWrappedNativeScope* scope = ObjectScope(obj);
 
   JSAutoRealm ar(cx, obj);
   JSObject* waiver = Wrapper::New(cx, obj, &XrayWaiver::singleton);
   if (!waiver) {
     return nullptr;
   }
 
@@ -825,75 +827,102 @@ bool WrapperFactory::WaiveXrayAndWrap(JS
 }
 
 /*
  * Calls to JS_TransplantObject* should go through these helpers here so that
  * waivers get fixed up properly.
  */
 
 static bool FixWaiverAfterTransplant(JSContext* cx, HandleObject oldWaiver,
-                                     HandleObject newobj) {
+                                     HandleObject newobj,
+                                     bool crossCompartmentTransplant) {
   MOZ_ASSERT(Wrapper::wrapperHandler(oldWaiver) == &XrayWaiver::singleton);
   MOZ_ASSERT(!js::IsCrossCompartmentWrapper(newobj));
 
-  // If the new compartment has a CCW for oldWaiver, nuke this CCW. This
-  // prevents confusing RemapAllWrappersForObject: it would call RemapWrapper
-  // with two same-compartment objects (the CCW and the new waiver).
-  //
-  // This can happen when loading a chrome page in a content frame and there
-  // exists a CCW from the chrome compartment to oldWaiver wrapping the window
-  // we just transplanted:
-  //
-  // Compartment 1  |  Compartment 2
-  // ----------------------------------------
-  // CCW1 -----------> oldWaiver --> CCW2 --+
-  // newWaiver                              |
-  // WindowProxy <--------------------------+
-  js::NukeCrossCompartmentWrapperIfExists(cx, js::GetObjectCompartment(newobj),
-                                          oldWaiver);
+  if (crossCompartmentTransplant) {
+    // If the new compartment has a CCW for oldWaiver, nuke this CCW. This
+    // prevents confusing RemapAllWrappersForObject: it would call RemapWrapper
+    // with two same-compartment objects (the CCW and the new waiver).
+    //
+    // This can happen when loading a chrome page in a content frame and there
+    // exists a CCW from the chrome compartment to oldWaiver wrapping the window
+    // we just transplanted:
+    //
+    // Compartment 1  |  Compartment 2
+    // ----------------------------------------
+    // CCW1 -----------> oldWaiver --> CCW2 --+
+    // newWaiver                              |
+    // WindowProxy <--------------------------+
+    js::NukeCrossCompartmentWrapperIfExists(
+        cx, js::GetObjectCompartment(newobj), oldWaiver);
+  } else {
+    // We kept the same object identity, so the waiver should be a
+    // waiver for our object, just in the wrong Realm.
+    MOZ_ASSERT(newobj == Wrapper::wrappedObject(oldWaiver));
+  }
 
-  // Create a waiver in the new compartment. We know there's not one already
-  // because we _just_ transplanted, which means that |newobj| was either
-  // created from scratch, or was previously cross-compartment wrapper (which
-  // should have no waiver). CreateXrayWaiver asserts this.
-  JSObject* newWaiver = WrapperFactory::CreateXrayWaiver(cx, newobj);
+  // Create a waiver in the new compartment. We know there's not one already in
+  // the crossCompartmentTransplant case because we _just_ transplanted, which
+  // means that |newobj| was either created from scratch, or was previously
+  // cross-compartment wrapper (which should have no waiver). On the other hand,
+  // in the !crossCompartmentTransplant case we know one already exists.
+  // CreateXrayWaiver asserts all this.
+  JSObject* newWaiver = WrapperFactory::CreateXrayWaiver(
+      cx, newobj, /* allowExisting = */ !crossCompartmentTransplant);
   if (!newWaiver) {
     return false;
   }
 
+  if (!crossCompartmentTransplant) {
+    // CreateXrayWaiver should have updated the map to point to the new waiver.
+    MOZ_ASSERT(WrapperFactory::GetXrayWaiver(newobj) == newWaiver);
+  }
+
   // Update all the cross-compartment references to oldWaiver to point to
   // newWaiver.
   if (!js::RemapAllWrappersForObject(cx, oldWaiver, newWaiver)) {
     return false;
   }
 
-  // There should be no same-compartment references to oldWaiver, and we
-  // just remapped all cross-compartment references. It's dead, so we can
-  // remove it from the map.
-  XPCWrappedNativeScope* scope = ObjectScope(oldWaiver);
-  JSObject* key = Wrapper::wrappedObject(oldWaiver);
-  MOZ_ASSERT(scope->mWaiverWrapperMap->Find(key));
-  scope->mWaiverWrapperMap->Remove(key);
+  if (crossCompartmentTransplant) {
+    // There should be no same-compartment references to oldWaiver, and we
+    // just remapped all cross-compartment references. It's dead, so we can
+    // remove it from the map.
+    XPCWrappedNativeScope* scope = ObjectScope(oldWaiver);
+    JSObject* key = Wrapper::wrappedObject(oldWaiver);
+    MOZ_ASSERT(scope->mWaiverWrapperMap->Find(key));
+    scope->mWaiverWrapperMap->Remove(key);
+  }
+
   return true;
 }
 
 JSObject* TransplantObject(JSContext* cx, JS::HandleObject origobj,
                            JS::HandleObject target) {
   RootedObject oldWaiver(cx, WrapperFactory::GetXrayWaiver(origobj));
+  MOZ_ASSERT_IF(oldWaiver, GetNonCCWObjectRealm(oldWaiver) ==
+                               GetNonCCWObjectRealm(origobj));
   RootedObject newIdentity(cx, JS_TransplantObject(cx, origobj, target));
   if (!newIdentity || !oldWaiver) {
     return newIdentity;
   }
 
-  // If we transplanted within a compartment, oldWaiver is still valid.
-  if (newIdentity == origobj) {
-    return newIdentity;
+  bool crossCompartmentTransplant = (newIdentity != origobj);
+  if (!crossCompartmentTransplant) {
+    // We might still have been transplanted across realms within a single
+    // compartment.
+    if (GetNonCCWObjectRealm(oldWaiver) == GetNonCCWObjectRealm(newIdentity)) {
+      // The old waiver is same-realm with the new object; nothing else to do
+      // here.
+      return newIdentity;
+    }
   }
 
-  if (!FixWaiverAfterTransplant(cx, oldWaiver, newIdentity)) {
+  if (!FixWaiverAfterTransplant(cx, oldWaiver, newIdentity,
+                                crossCompartmentTransplant)) {
     return nullptr;
   }
   return newIdentity;
 }
 
 JSObject* TransplantObjectRetainingXrayExpandos(JSContext* cx,
                                                 JS::HandleObject origobj,
                                                 JS::HandleObject target) {
--- a/js/xpconnect/wrappers/WrapperFactory.h
+++ b/js/xpconnect/wrappers/WrapperFactory.h
@@ -69,17 +69,20 @@ class WrapperFactory {
 
   static bool HasWaiveXrayFlag(JSObject* wrapper) {
     return HasWrapperFlag(wrapper, WAIVE_XRAY_WRAPPER_FLAG);
   }
 
   static bool IsCOW(JSObject* wrapper);
 
   static JSObject* GetXrayWaiver(JS::HandleObject obj);
-  static JSObject* CreateXrayWaiver(JSContext* cx, JS::HandleObject obj);
+  // If allowExisting is true, there is an existing waiver for obj in
+  // its scope, but we want to replace it with the new one.
+  static JSObject* CreateXrayWaiver(JSContext* cx, JS::HandleObject obj,
+                                    bool allowExisting = false);
   static JSObject* WaiveXray(JSContext* cx, JSObject* obj);
 
   // Computes whether we should allow the creation of an Xray waiver from
   // |target| to |origin|.
   static bool AllowWaiver(JS::Compartment* target, JS::Compartment* origin);
 
   // Convenience method for the above, operating on a wrapper.
   static bool AllowWaiver(JSObject* wrapper);