Bug 1510760, part 4 - Add transplant support to GetRemoteOuterWindowProxy(). r=peterv

When we call GetRemoteOuterWindowProxy in the middle of a transplant,
the remote proxy that the function returns will be almost immediately
swapped with some other object. Rather than trying to fix up the
remote proxy map when that happens, this patch adds a new argument
that is a pointer to the object, if any, that the remote proxy is
going to be swapped to. This will be used in the remote proxy map.

Having a value in the remote proxy map that is not a remote proxy
could cause issues if somebody ends up calling
GetRemoteOuterWindowProxy() a second time before the transplant has
finished. To avoid that, my patch asserts that we are returning an
object with the appropriate class.

Differential Revision: https://phabricator.services.mozilla.com/D37598

--HG--
extra : moz-landing-system : lando

docshell/base/BrowsingContext.cpp

@@ -673,7 +673,8 @@ void BrowsingContext::Location(JSContext* aCx,
                                JS::MutableHandle<JSObject*> aLocation,
                                ErrorResult& aError) {
   aError.MightThrowJSException();
-  sSingleton.GetProxyObject(aCx, &mLocation, aLocation);
+  sSingleton.GetProxyObject(aCx, &mLocation, /* aTransplantTo = */ nullptr,
+                            aLocation);
   if (!aLocation) {
     aError.StealExceptionFromJSContext(aCx);
   }

docshell/base/BrowsingContext.h

@@ -499,8 +499,13 @@ class BrowsingContext : public nsWrapperCache, public BrowsingContextBase {
  * lives in this process, and a same-process WindowProxy should be used (see
  * nsGlobalWindowOuter). This should only be called by bindings code, ToJSValue
  * is the right API to get a WindowProxy for a BrowsingContext.
+ *
+ * If aTransplantTo is non-null, then the WindowProxy object will eventually be
+ * transplanted onto it. Therefore it should be used as the value in the remote
+ * proxy map.
  */
 extern bool GetRemoteOuterWindowProxy(JSContext* aCx, BrowsingContext* aContext,
+                                      JS::Handle<JSObject*> aTransplantTo,
                                       JS::MutableHandle<JSObject*> aRetVal);
 
 typedef BrowsingContext::Transaction BrowsingContextTransaction;

dom/base/RemoteOuterWindowProxy.cpp

@@ -62,11 +62,12 @@ const js::Class RemoteOuterWindowProxy::Base::sClass =
     PROXY_CLASS_DEF("Proxy", JSCLASS_HAS_RESERVED_SLOTS(2));
 
 bool GetRemoteOuterWindowProxy(JSContext* aCx, BrowsingContext* aContext,
+                               JS::Handle<JSObject*> aTransplantTo,
                                JS::MutableHandle<JSObject*> aRetVal) {
   MOZ_ASSERT(!aContext->GetDocShell(),
              "Why are we creating a RemoteOuterWindowProxy?");
 
-  sSingleton.GetProxyObject(aCx, aContext, aRetVal);
+  sSingleton.GetProxyObject(aCx, aContext, aTransplantTo, aRetVal);
   return !!aRetVal;
 }
 

dom/base/WindowProxyHolder.h

@@ -70,6 +70,7 @@ inline void ImplCycleCollectionUnlink(WindowProxyHolder& aProxy) {
 }
 
 extern bool GetRemoteOuterWindowProxy(JSContext* aCx, BrowsingContext* aContext,
+                                      JS::Handle<JSObject*> aTransplantTo,
                                       JS::MutableHandle<JSObject*> aValue);
 
 }  // namespace dom

dom/bindings/RemoteObjectProxy.cpp

@@ -172,13 +172,23 @@ const char* RemoteObjectProxyBase::className(
 
 void RemoteObjectProxyBase::GetOrCreateProxyObject(
     JSContext* aCx, void* aNative, const js::Class* aClasp,
-    JS::MutableHandle<JSObject*> aProxy, bool& aNewObjectCreated) const {
+    JS::Handle<JSObject*> aTransplantTo, JS::MutableHandle<JSObject*> aProxy,
+    bool& aNewObjectCreated) const {
   xpc::CompartmentPrivate* priv =
       xpc::CompartmentPrivate::Get(JS::CurrentGlobalOrNull(aCx));
   xpc::CompartmentPrivate::RemoteProxyMap& map = priv->GetRemoteProxyMap();
   auto result = map.lookupForAdd(aNative);
   if (result) {
+    MOZ_ASSERT(!aTransplantTo,
+               "No existing value allowed if we're doing a transplant");
+
     aProxy.set(result->value());
+
+    // During a transplant, we put an object that is temporarily not a
+    // proxy object into the map. Make sure that we don't return one of
+    // these objects in the middle of a transplant.
+    MOZ_RELEASE_ASSERT(js::GetObjectClass(aProxy) == aClasp);
+
     return;
   }
 
@@ -193,7 +203,13 @@ void RemoteObjectProxyBase::GetOrCreateProxyObject(
 
   aNewObjectCreated = true;
 
-  if (!map.add(result, aNative, obj)) {
+  // If we're transplanting onto an object, we want to make sure that it does
+  // not have the same class as aClasp to ensure that the release assert earlier
+  // in this function will actually fire if we try to return a proxy object in
+  // the middle of a transplant.
+  MOZ_ASSERT_IF(aTransplantTo, js::GetObjectClass(aTransplantTo) != aClasp);
+
+  if (!map.add(result, aNative, aTransplantTo ? aTransplantTo : obj)) {
     return;
   }
 

dom/bindings/RemoteObjectProxy.h

@@ -117,6 +117,7 @@ class RemoteObjectProxyBase : public js::BaseProxyHandler,
    */
   void GetOrCreateProxyObject(JSContext* aCx, void* aNative,
                               const js::Class* aClasp,
+                              JS::Handle<JSObject*> aTransplantTo,
                               JS::MutableHandle<JSObject*> aProxy,
                               bool& aNewObjectCreated) const;
 
@@ -149,9 +150,11 @@ class RemoteObjectProxy : public RemoteObjectProxyBase {
   }
 
   void GetProxyObject(JSContext* aCx, Native* aNative,
+                      JS::Handle<JSObject*> aTransplantTo,
                       JS::MutableHandle<JSObject*> aProxy) const {
     bool objectCreated = false;
-    GetOrCreateProxyObject(aCx, aNative, &sClass, aProxy, objectCreated);
+    GetOrCreateProxyObject(aCx, aNative, &sClass, aTransplantTo, aProxy,
+                           objectCreated);
     if (objectCreated) {
       NS_ADDREF(aNative);
     }

dom/bindings/ToJSValue.cpp

@@ -86,7 +86,8 @@ bool ToJSValue(JSContext* aCx, const WindowProxyHolder& aArgument,
     return ToJSValue(aCx, windowProxy, aValue);
   }
 
-  if (!GetRemoteOuterWindowProxy(aCx, bc, &windowProxy)) {
+  if (!GetRemoteOuterWindowProxy(aCx, bc, /* aTransplantTo = */ nullptr,
+                                 &windowProxy)) {
     return false;
   }
   aValue.setObjectOrNull(windowProxy);