Bug 1589054 - Part 1: Cleanly kill BrowserBridgeChild if process switch fails, r=farre

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

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

dom/base/nsFrameLoaderOwner.cpp

@@ -182,9 +182,8 @@ void nsFrameLoaderOwner::ChangeRemoteness(
                          aOptions.mRemoteType, frameLoaderInit, rv);
 }
 
-void nsFrameLoaderOwner::ChangeRemotenessWithBridge(
-    mozilla::ipc::ManagedEndpoint<mozilla::dom::PBrowserBridgeChild> aEndpoint,
-    uint64_t aTabId, mozilla::ErrorResult& rv) {
+void nsFrameLoaderOwner::ChangeRemotenessWithBridge(BrowserBridgeChild* aBridge,
+                                                    mozilla::ErrorResult& rv) {
   MOZ_ASSERT(XRE_IsContentProcess());
   if (NS_WARN_IF(!mFrameLoader)) {
     rv.Throw(NS_ERROR_UNEXPECTED);
@@ -192,24 +191,9 @@ void nsFrameLoaderOwner::ChangeRemotenessWithBridge(
   }
 
   std::function<void()> frameLoaderInit = [&] {
-    RefPtr<BrowsingContext> browsingContext = mFrameLoader->mBrowsingContext;
-    RefPtr<BrowserBridgeChild> bridge =
-        new BrowserBridgeChild(mFrameLoader, browsingContext, TabId(aTabId));
-    Document* ownerDoc = mFrameLoader->GetOwnerDoc();
-    if (NS_WARN_IF(!ownerDoc)) {
-      rv.Throw(NS_ERROR_UNEXPECTED);
-      return;
-    }
-
-    RefPtr<BrowserChild> browser =
-        BrowserChild::GetFrom(ownerDoc->GetDocShell());
-    if (!browser->BindPBrowserBridgeEndpoint(std::move(aEndpoint), bridge)) {
-      rv.Throw(NS_ERROR_UNEXPECTED);
-      return;
-    }
-
-    RefPtr<BrowserBridgeHost> host = bridge->FinishInit();
-    browsingContext->SetEmbedderElement(mFrameLoader->GetOwnerContent());
+    RefPtr<BrowserBridgeHost> host = aBridge->FinishInit(mFrameLoader);
+    mFrameLoader->mBrowsingContext->SetEmbedderElement(
+        mFrameLoader->GetOwnerContent());
     mFrameLoader->mRemoteBrowser = host;
   };
 

dom/base/nsFrameLoaderOwner.h

@@ -14,13 +14,9 @@ namespace mozilla {
 class ErrorResult;
 namespace dom {
 class BrowsingContext;
-class PBrowserBridgeChild;
+class BrowserBridgeChild;
 struct RemotenessOptions;
 }  // namespace dom
-namespace ipc {
-template <typename T>
-class ManagedEndpoint;
-}  // namespace ipc
 }  // namespace mozilla
 
 // IID for the FrameLoaderOwner interface
@@ -57,10 +53,8 @@ class nsFrameLoaderOwner : public nsISupports {
   void ChangeRemoteness(const mozilla::dom::RemotenessOptions& aOptions,
                         mozilla::ErrorResult& rv);
 
-  void ChangeRemotenessWithBridge(
-      mozilla::ipc::ManagedEndpoint<mozilla::dom::PBrowserBridgeChild>
-          aEndpoint,
-      uint64_t aTabId, mozilla::ErrorResult& rv);
+  void ChangeRemotenessWithBridge(mozilla::dom::BrowserBridgeChild* aBridge,
+                                  mozilla::ErrorResult& rv);
 
  private:
   bool UseRemoteSubframes();

dom/ipc/BrowserBridgeChild.cpp

@@ -30,13 +30,9 @@ using namespace mozilla::ipc;
 namespace mozilla {
 namespace dom {
 
-BrowserBridgeChild::BrowserBridgeChild(nsFrameLoader* aFrameLoader,
-                                       BrowsingContext* aBrowsingContext,
+BrowserBridgeChild::BrowserBridgeChild(BrowsingContext* aBrowsingContext,
                                        TabId aId)
-    : mId{aId},
-      mLayersId{0},
-      mFrameLoader(aFrameLoader),
-      mBrowsingContext(aBrowsingContext) {}
+    : mId{aId}, mLayersId{0}, mBrowsingContext(aBrowsingContext) {}
 
 BrowserBridgeChild::~BrowserBridgeChild() {
 #if defined(ACCESSIBILITY) && defined(XP_WIN)
@@ -46,7 +42,11 @@ BrowserBridgeChild::~BrowserBridgeChild() {
 #endif
 }
 
-already_AddRefed<BrowserBridgeHost> BrowserBridgeChild::FinishInit() {
+already_AddRefed<BrowserBridgeHost> BrowserBridgeChild::FinishInit(
+    nsFrameLoader* aFrameLoader) {
+  MOZ_DIAGNOSTIC_ASSERT(!mFrameLoader);
+  mFrameLoader = aFrameLoader;
+
   RefPtr<Element> owner = mFrameLoader->GetOwnerContent();
   nsCOMPtr<nsIDocShell> docShell = do_GetInterface(owner->GetOwnerGlobal());
   MOZ_DIAGNOSTIC_ASSERT(docShell);

dom/ipc/BrowserBridgeChild.h

@@ -56,7 +56,7 @@ class BrowserBridgeChild : public PBrowserBridgeChild {
 
   void SetIsUnderHiddenEmbedderElement(bool aIsUnderHiddenEmbedderElement);
 
-  already_AddRefed<BrowserBridgeHost> FinishInit();
+  already_AddRefed<BrowserBridgeHost> FinishInit(nsFrameLoader* aFrameLoader);
 
 #if defined(ACCESSIBILITY) && defined(XP_WIN)
   a11y::RemoteIframeDocProxyAccessibleWrap* GetEmbeddedDocAccessible() {
@@ -68,8 +68,7 @@ class BrowserBridgeChild : public PBrowserBridgeChild {
 
   static BrowserBridgeChild* GetFrom(nsIContent* aContent);
 
-  BrowserBridgeChild(nsFrameLoader* aFrameLoader,
-                     BrowsingContext* aBrowsingContext, TabId aId);
+  BrowserBridgeChild(BrowsingContext* aBrowsingContext, TabId aId);
 
  protected:
   friend class ContentChild;

dom/ipc/ContentChild.cpp

@@ -2127,13 +2127,13 @@ already_AddRefed<RemoteBrowser> ContentChild::CreateBrowser(
 
   TabId tabId(nsContentUtils::GenerateTabId());
   RefPtr<BrowserBridgeChild> browserBridge =
-      new BrowserBridgeChild(aFrameLoader, aBrowsingContext, tabId);
+      new BrowserBridgeChild(aBrowsingContext, tabId);
 
   browserChild->SendPBrowserBridgeConstructor(
       browserBridge, PromiseFlatString(aContext.PresentationURL()), aRemoteType,
       aBrowsingContext, chromeFlags, tabId);
 
-  return browserBridge->FinishInit();
+  return browserBridge->FinishInit(aFrameLoader);
 }
 
 PScriptCacheChild* ContentChild::AllocPScriptCacheChild(

dom/ipc/WindowGlobalChild.cpp

@@ -281,11 +281,11 @@ mozilla::ipc::IPCResult WindowGlobalChild::RecvMakeFrameLocal(
 
   RefPtr<Element> embedderElt = aFrameContext->GetEmbedderElement();
   if (NS_WARN_IF(!embedderElt)) {
-    return IPC_FAIL(this, "No embedder element in this process");
+    return IPC_OK();
   }
 
   if (NS_WARN_IF(embedderElt->GetOwnerGlobal() != WindowGlobal())) {
-    return IPC_FAIL(this, "Wrong actor");
+    return IPC_OK();
   }
 
   RefPtr<nsFrameLoaderOwner> flo = do_QueryObject(embedderElt);
@@ -311,20 +311,38 @@ mozilla::ipc::IPCResult WindowGlobalChild::RecvMakeFrameRemote(
   // Immediately resolve the promise, acknowledging the request.
   aResolve(true);
 
+  // Immediately construct the BrowserBridgeChild so we can destroy it cleanly
+  // if the process switch fails.
+  RefPtr<BrowserBridgeChild> bridge =
+      new BrowserBridgeChild(aFrameContext, aTabId);
+  RefPtr<BrowserChild> manager = GetBrowserChild();
+  if (NS_WARN_IF(
+          !manager->BindPBrowserBridgeEndpoint(std::move(aEndpoint), bridge))) {
+    return IPC_OK();
+  }
+
   RefPtr<Element> embedderElt = aFrameContext->GetEmbedderElement();
   if (NS_WARN_IF(!embedderElt)) {
-    return IPC_FAIL(this, "No embedder element in this process");
+    BrowserBridgeChild::Send__delete__(bridge);
+    return IPC_OK();
   }
 
   if (NS_WARN_IF(embedderElt->GetOwnerGlobal() != WindowGlobal())) {
-    return IPC_FAIL(this, "Wrong actor");
+    BrowserBridgeChild::Send__delete__(bridge);
+    return IPC_OK();
   }
 
   RefPtr<nsFrameLoaderOwner> flo = do_QueryObject(embedderElt);
   MOZ_DIAGNOSTIC_ASSERT(flo, "Embedder must be a nsFrameLoaderOwner");
 
   // Trgger a process switch into the specified process.
-  flo->ChangeRemotenessWithBridge(std::move(aEndpoint), aTabId, IgnoreErrors());
+  IgnoredErrorResult rv;
+  flo->ChangeRemotenessWithBridge(bridge, rv);
+  if (NS_WARN_IF(rv.Failed())) {
+    BrowserBridgeChild::Send__delete__(bridge);
+    return IPC_OK();
+  }
+
   return IPC_OK();
 }