Bug 1589054 - Part 1: Cleanly kill BrowserBridgeChild if process switch fails, r=farre
☠☠ backed out by 7537be6fe6bf ☠ ☠
authorNika Layzell <nika@thelayzells.com>
Mon, 21 Oct 2019 14:03:36 +0000
changeset 498370 03bc24aa3a2c434e579cb44ba840b691d77cf841
parent 498369 562a3883426b733283d6d2094b10c0d0c2981a64
child 498371 91e4d5c6422a0f6d47bd16618938b22a100da7fd
push id36717
push usernbeleuzu@mozilla.com
push dateMon, 21 Oct 2019 21:51:55 +0000
treeherdermozilla-central@563f437f24b9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfarre
bugs1589054
milestone71.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 1589054 - Part 1: Cleanly kill BrowserBridgeChild if process switch fails, r=farre Differential Revision: https://phabricator.services.mozilla.com/D49464
dom/base/nsFrameLoaderOwner.cpp
dom/base/nsFrameLoaderOwner.h
dom/ipc/BrowserBridgeChild.cpp
dom/ipc/BrowserBridgeChild.h
dom/ipc/ContentChild.cpp
dom/ipc/WindowGlobalChild.cpp
--- a/dom/base/nsFrameLoaderOwner.cpp
+++ b/dom/base/nsFrameLoaderOwner.cpp
@@ -177,44 +177,28 @@ void nsFrameLoaderOwner::ChangeRemotenes
       mFrameLoader->LoadFrame(false);
     }
   };
 
   ChangeRemotenessCommon(ShouldPreserveBrowsingContext(aOptions),
                          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);
     return;
   }
 
   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;
   };
 
   // NOTE: We always use the DEFAULT_REMOTE_TYPE here, because we don't actually
   // know the real remote type, and don't need to, as we're a content process.
   ChangeRemotenessCommon(
       /* preserve */ true, NS_LITERAL_STRING(DEFAULT_REMOTE_TYPE),
       frameLoaderInit, rv);
--- a/dom/base/nsFrameLoaderOwner.h
+++ b/dom/base/nsFrameLoaderOwner.h
@@ -9,23 +9,19 @@
 
 #include "nsISupports.h"
 
 class nsFrameLoader;
 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
 #define NS_FRAMELOADEROWNER_IID                      \
   {                                                  \
     0x1b4fd25c, 0x2e57, 0x11e9, {                    \
       0x9e, 0x5a, 0x5b, 0x86, 0xe9, 0x89, 0xa5, 0xc0 \
     }                                                \
@@ -52,20 +48,18 @@ class nsFrameLoaderOwner : public nsISup
   // remoteness requirements. This should follow the same path as
   // tabbrowser.js's updateBrowserRemoteness, including running the same logic
   // and firing the same events as unbinding a XULBrowserElement from the tree.
   // However, this method is available from backend and does not manipulate the
   // DOM.
   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();
   bool ShouldPreserveBrowsingContext(
       const mozilla::dom::RemotenessOptions& aOptions);
   void ChangeRemotenessCommon(bool aPreserveContext,
                               const nsAString& aRemoteType,
                               std::function<void()>& aFrameLoaderInit,
--- a/dom/ipc/BrowserBridgeChild.cpp
+++ b/dom/ipc/BrowserBridgeChild.cpp
@@ -25,33 +25,33 @@
 #include "nsSubDocumentFrame.h"
 #include "nsView.h"
 
 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)
   if (mEmbeddedDocAccessible) {
     mEmbeddedDocAccessible->Shutdown();
   }
 #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);
 
   nsDocShell::Cast(docShell)->OOPChildLoadStarted(this);
 
 #if defined(ACCESSIBILITY)
   if (a11y::DocAccessible* docAcc =
--- a/dom/ipc/BrowserBridgeChild.h
+++ b/dom/ipc/BrowserBridgeChild.h
@@ -51,30 +51,29 @@ class BrowserBridgeChild : public PBrows
   void NavigateByKey(bool aForward, bool aForDocumentNavigation);
 
   void Activate();
 
   void Deactivate(bool aWindowLowering);
 
   void SetIsUnderHiddenEmbedderElement(bool aIsUnderHiddenEmbedderElement);
 
-  already_AddRefed<BrowserBridgeHost> FinishInit();
+  already_AddRefed<BrowserBridgeHost> FinishInit(nsFrameLoader* aFrameLoader);
 
 #if defined(ACCESSIBILITY) && defined(XP_WIN)
   a11y::RemoteIframeDocProxyAccessibleWrap* GetEmbeddedDocAccessible() {
     return mEmbeddedDocAccessible;
   }
 #endif
 
   static BrowserBridgeChild* GetFrom(nsFrameLoader* aFrameLoader);
 
   static BrowserBridgeChild* GetFrom(nsIContent* aContent);
 
-  BrowserBridgeChild(nsFrameLoader* aFrameLoader,
-                     BrowsingContext* aBrowsingContext, TabId aId);
+  BrowserBridgeChild(BrowsingContext* aBrowsingContext, TabId aId);
 
  protected:
   friend class ContentChild;
   friend class PBrowserBridgeChild;
 
   mozilla::ipc::IPCResult RecvSetLayersId(
       const mozilla::layers::LayersId& aLayersId);
 
--- a/dom/ipc/ContentChild.cpp
+++ b/dom/ipc/ContentChild.cpp
@@ -2122,23 +2122,23 @@ already_AddRefed<RemoteBrowser> ContentC
     chromeFlags |= nsIWebBrowserChrome::CHROME_FISSION_WINDOW;
   }
   if (docShell->GetAffectPrivateSessionLifetime()) {
     chromeFlags |= nsIWebBrowserChrome::CHROME_PRIVATE_LIFETIME;
   }
 
   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(
     const FileDescOrError& cacheFile, const bool& wantCacheData) {
   return new loader::ScriptCacheChild();
 }
 
 bool ContentChild::DeallocPScriptCacheChild(PScriptCacheChild* cache) {
--- a/dom/ipc/WindowGlobalChild.cpp
+++ b/dom/ipc/WindowGlobalChild.cpp
@@ -276,21 +276,21 @@ mozilla::ipc::IPCResult WindowGlobalChil
     dom::BrowsingContext* aFrameContext, uint64_t aPendingSwitchId) {
   MOZ_DIAGNOSTIC_ASSERT(XRE_IsContentProcess());
 
   MOZ_LOG(aFrameContext->GetLog(), LogLevel::Debug,
           ("RecvMakeFrameLocal ID=%" PRIx64, aFrameContext->Id()));
 
   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);
   MOZ_DIAGNOSTIC_ASSERT(flo, "Embedder must be a nsFrameLoaderOwner");
 
   // Trigger a process switch into the current process.
   RemotenessOptions options;
   options.mRemoteType.Assign(VoidString());
@@ -306,30 +306,48 @@ mozilla::ipc::IPCResult WindowGlobalChil
   MOZ_DIAGNOSTIC_ASSERT(XRE_IsContentProcess());
 
   MOZ_LOG(aFrameContext->GetLog(), LogLevel::Debug,
           ("RecvMakeFrameRemote ID=%" PRIx64, aFrameContext->Id()));
 
   // 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();
 }
 
 mozilla::ipc::IPCResult WindowGlobalChild::RecvDrawSnapshot(
     const Maybe<IntRect>& aRect, const float& aScale,
     const nscolor& aBackgroundColor, const uint32_t& aFlags,
     DrawSnapshotResolver&& aResolve) {
   nsCOMPtr<nsIDocShell> docShell = BrowsingContext()->GetDocShell();