Bug 1616353 - Part 13: Don't create an extra BrowsingContext when opening a new tab from content, r=kmag

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

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

dom/base/nsFrameLoader.cpp

@@ -259,6 +259,14 @@ static bool IsTopContent(BrowsingContext* aParent, Element* aOwner) {
 
 static already_AddRefed<BrowsingContext> CreateBrowsingContext(
     Element* aOwner, nsIOpenWindowInfo* aOpenWindowInfo) {
+  // If we've got a pending BrowserParent from the content process, use the
+  // BrowsingContext which was created for it.
+  if (aOpenWindowInfo && aOpenWindowInfo->GetNextRemoteBrowser()) {
+    MOZ_ASSERT(XRE_IsParentProcess());
+    return do_AddRef(
+        aOpenWindowInfo->GetNextRemoteBrowser()->GetBrowsingContext());
+  }
+
   RefPtr<BrowsingContext> opener;
   if (aOpenWindowInfo && !aOpenWindowInfo->GetForceNoOpener()) {
     opener = aOpenWindowInfo->GetParent();
@@ -2486,9 +2494,6 @@ bool nsFrameLoader::TryRemoteBrowserInternal() {
     return false;
   }
 
-  uint64_t nextRemoteBrowserId =
-      mOpenWindowInfo ? mOpenWindowInfo->GetNextRemoteBrowserId() : 0;
-
   if (!EnsureBrowsingContextAttached()) {
     return false;
   }
@@ -2562,22 +2567,26 @@ bool nsFrameLoader::TryRemoteBrowserInternal() {
 
   nsCOMPtr<Element> ownerElement = mOwnerContent;
 
-  mRemoteBrowser = ContentParent::CreateBrowser(
-      context, ownerElement, mRemoteType, mPendingBrowsingContext,
-      openerContentParent, nextRemoteBrowserId);
+  RefPtr<BrowserParent> nextRemoteBrowser =
+      mOpenWindowInfo ? mOpenWindowInfo->GetNextRemoteBrowser() : nullptr;
+  if (nextRemoteBrowser) {
+    mRemoteBrowser = new BrowserHost(nextRemoteBrowser);
+    if (nextRemoteBrowser->GetOwnerElement()) {
+      MOZ_ASSERT_UNREACHABLE("Shouldn't have an owner element before");
+      return false;
+    }
+    nextRemoteBrowser->SetOwnerElement(ownerElement);
+  } else {
+    mRemoteBrowser = ContentParent::CreateBrowser(
+        context, ownerElement, mRemoteType, mPendingBrowsingContext,
+        openerContentParent);
+  }
   if (!mRemoteBrowser) {
     return false;
   }
 
-  // If we were given a remote tab ID, we may be attaching to an existing remote
-  // browser, which already has its own BrowsingContext. If so, we need to
-  // detach our original BC and take ownership of the one from the remote
-  // browser.
-  if (mPendingBrowsingContext != mRemoteBrowser->GetBrowsingContext()) {
-    MOZ_DIAGNOSTIC_ASSERT(nextRemoteBrowserId);
-    mPendingBrowsingContext->Detach();
-    mPendingBrowsingContext = mRemoteBrowser->GetBrowsingContext();
-  }
+  MOZ_DIAGNOSTIC_ASSERT(mPendingBrowsingContext ==
+                        mRemoteBrowser->GetBrowsingContext());
 
   mRemoteBrowser->GetBrowsingContext()->Embed();
 

dom/ipc/ContentParent.cpp

@@ -586,9 +586,6 @@ StaticAutoPtr<LinkedList<ContentParent>> ContentParent::sContentParents;
 UniquePtr<SandboxBrokerPolicyFactory>
     ContentParent::sSandboxBrokerPolicyFactory;
 #endif
-uint64_t ContentParent::sNextRemoteTabId = 0;
-nsDataHashtable<nsUint64HashKey, BrowserParent*>
-    ContentParent::sNextBrowserParents;
 StaticAutoPtr<nsTArray<RefPtr<BrowsingContextGroup>>>
     ContentParent::sBrowsingContextGroupHolder;
 #if defined(XP_MACOSX) && defined(MOZ_SANDBOX)
@@ -1193,7 +1190,7 @@ mozilla::ipc::IPCResult ContentParent::RecvLaunchRDDProcess(
 already_AddRefed<RemoteBrowser> ContentParent::CreateBrowser(
     const TabContext& aContext, Element* aFrameElement,
     const nsAString& aRemoteType, BrowsingContext* aBrowsingContext,
-    ContentParent* aOpenerContentParent, uint64_t aNextRemoteTabId) {
+    ContentParent* aOpenerContentParent) {
   AUTO_PROFILER_LABEL("ContentParent::CreateBrowser", OTHER);
 
   if (!sCanLaunchSubprocesses) {
@@ -1205,18 +1202,6 @@ already_AddRefed<RemoteBrowser> ContentParent::CreateBrowser(
     remoteType.AssignLiteral(DEFAULT_REMOTE_TYPE);
   }
 
-  if (aNextRemoteTabId) {
-    if (BrowserParent* parent =
-            sNextBrowserParents.GetAndRemove(aNextRemoteTabId)
-                .valueOr(nullptr)) {
-      RefPtr<BrowserHost> browserHost = new BrowserHost(parent);
-      MOZ_ASSERT(!parent->GetOwnerElement(),
-                 "Shouldn't have an owner elemnt before");
-      parent->SetOwnerElement(aFrameElement);
-      return browserHost.forget();
-    }
-  }
-
   ProcessPriority initialPriority = GetInitialProcessPriority(aFrameElement);
   TabId tabId(nsContentUtils::GenerateTabId());
 
@@ -4610,12 +4595,12 @@ mozilla::ipc::IPCResult ContentParent::CommonCreateWindow(
     PBrowserParent* aThisTab, BrowsingContext* aParent, bool aSetOpener,
     const uint32_t& aChromeFlags, const bool& aCalledFromJS,
     const bool& aWidthSpecified, nsIURI* aURIToLoad, const nsCString& aFeatures,
-    const float& aFullZoom, uint64_t aNextRemoteTabId, const nsString& aName,
-    nsresult& aResult, nsCOMPtr<nsIRemoteTab>& aNewRemoteTab,
-    bool* aWindowIsNew, int32_t& aOpenLocation,
-    nsIPrincipal* aTriggeringPrincipal, nsIReferrerInfo* aReferrerInfo,
-    bool aLoadURI, nsIContentSecurityPolicy* aCsp,
-    const OriginAttributes& aOriginAttributes) {
+    const float& aFullZoom, BrowserParent* aNextRemoteBrowser,
+    const nsString& aName, nsresult& aResult,
+    nsCOMPtr<nsIRemoteTab>& aNewRemoteTab, bool* aWindowIsNew,
+    int32_t& aOpenLocation, nsIPrincipal* aTriggeringPrincipal,
+    nsIReferrerInfo* aReferrerInfo, bool aLoadURI,
+    nsIContentSecurityPolicy* aCsp, const OriginAttributes& aOriginAttributes) {
   // The content process should never be in charge of computing whether or
   // not a window should be private - the parent will do that.
   const uint32_t badFlags = nsIWebBrowserChrome::CHROME_PRIVATE_WINDOW |
@@ -4629,7 +4614,7 @@ mozilla::ipc::IPCResult ContentParent::CommonCreateWindow(
   openInfo->mForceNoOpener = !aSetOpener;
   openInfo->mParent = aParent;
   openInfo->mIsRemote = true;
-  openInfo->mNextRemoteBrowserId = aNextRemoteTabId;
+  openInfo->mNextRemoteBrowser = aNextRemoteBrowser;
   openInfo->mOriginAttributes = aOriginAttributes;
 
   RefPtr<BrowserParent> topParent = BrowserParent::GetFrom(aThisTab);
@@ -4898,17 +4883,14 @@ mozilla::ipc::IPCResult ContentParent::RecvCreateWindow(
   }
 
   BrowserParent::AutoUseNewTab aunt(newTab, &cwi.urlToLoad());
-  const uint64_t nextRemoteTabId = ++sNextRemoteTabId;
-  sNextBrowserParents.Put(nextRemoteTabId, newTab);
 
   nsCOMPtr<nsIRemoteTab> newRemoteTab;
   int32_t openLocation = nsIBrowserDOMWindow::OPEN_NEWWINDOW;
   mozilla::ipc::IPCResult ipcResult = CommonCreateWindow(
       aThisTab, aParent.get(), !!newBCOpener, aChromeFlags, aCalledFromJS,
-      aWidthSpecified, aURIToLoad, aFeatures, aFullZoom, nextRemoteTabId,
-      VoidString(), rv, newRemoteTab, &cwi.windowOpened(), openLocation,
-      aTriggeringPrincipal, aReferrerInfo,
-      /* aLoadUri = */ false, aCsp, aOriginAttributes);
+      aWidthSpecified, aURIToLoad, aFeatures, aFullZoom, newTab, VoidString(),
+      rv, newRemoteTab, &cwi.windowOpened(), openLocation, aTriggeringPrincipal,
+      aReferrerInfo, /* aLoadUri = */ false, aCsp, aOriginAttributes);
   if (!ipcResult) {
     return ipcResult;
   }
@@ -4917,9 +4899,6 @@ mozilla::ipc::IPCResult ContentParent::RecvCreateWindow(
     return IPC_OK();
   }
 
-  if (sNextBrowserParents.GetAndRemove(nextRemoteTabId).valueOr(nullptr)) {
-    cwi.windowOpened() = false;
-  }
   MOZ_ASSERT(BrowserHost::GetFrom(newRemoteTab.get()) ==
              newTab->GetBrowserHost());
 
@@ -4984,7 +4963,7 @@ mozilla::ipc::IPCResult ContentParent::RecvCreateWindowInDifferentProcess(
   mozilla::ipc::IPCResult ipcResult = CommonCreateWindow(
       aThisTab, aParent.get(), /* aSetOpener = */ false, aChromeFlags,
       aCalledFromJS, aWidthSpecified, aURIToLoad, aFeatures, aFullZoom,
-      /* aNextRemoteTabId = */ 0, aName, rv, newRemoteTab, &windowIsNew,
+      /* aNextRemoteBrowser = */ nullptr, aName, rv, newRemoteTab, &windowIsNew,
       openLocation, aTriggeringPrincipal, aReferrerInfo,
       /* aLoadUri = */ true, aCsp, aOriginAttributes);
   if (!ipcResult) {

dom/ipc/ContentParent.h

@@ -230,7 +230,7 @@ class ContentParent final
   static already_AddRefed<RemoteBrowser> CreateBrowser(
       const TabContext& aContext, Element* aFrameElement,
       const nsAString& aRemoteType, BrowsingContext* aBrowsingContext,
-      ContentParent* aOpenerContentParent, uint64_t aNextRemoteTabId);
+      ContentParent* aOpenerContentParent);
 
   static void GetAll(nsTArray<ContentParent*>& aArray);
 
@@ -731,11 +731,11 @@ class ContentParent final
       const uint32_t& aChromeFlags, const bool& aCalledFromJS,
       const bool& aWidthSpecified, nsIURI* aURIToLoad,
       const nsCString& aFeatures, const float& aFullZoom,
-      uint64_t aNextRemoteTabId, const nsString& aName, nsresult& aResult,
-      nsCOMPtr<nsIRemoteTab>& aNewRemoteTab, bool* aWindowIsNew,
-      int32_t& aOpenLocation, nsIPrincipal* aTriggeringPrincipal,
-      nsIReferrerInfo* aReferrerInfo, bool aLoadUri,
-      nsIContentSecurityPolicy* aCsp,
+      BrowserParent* aNextRemoteBrowser, const nsString& aName,
+      nsresult& aResult, nsCOMPtr<nsIRemoteTab>& aNewRemoteTab,
+      bool* aWindowIsNew, int32_t& aOpenLocation,
+      nsIPrincipal* aTriggeringPrincipal, nsIReferrerInfo* aReferrerInfo,
+      bool aLoadUri, nsIContentSecurityPolicy* aCsp,
       const OriginAttributes& aOriginAttributes);
 
   explicit ContentParent(int32_t aPluginID)
@@ -1461,9 +1461,6 @@ class ContentParent final
 
   RefPtr<mozilla::dom::ProcessMessageManager> mMessageManager;
 
-  static uint64_t sNextRemoteTabId;
-  static nsDataHashtable<nsUint64HashKey, BrowserParent*> sNextBrowserParents;
-
   static StaticAutoPtr<nsTArray<RefPtr<BrowsingContextGroup>>>
       sBrowsingContextGroupHolder;
 

toolkit/components/windowwatcher/nsIOpenWindowInfo.idl

@@ -11,9 +11,13 @@ webidl BrowsingContext;
 %{ C++
 namespace mozilla {
 class OriginAttributes;
+namespace dom {
+class BrowserParent;
+} // namespace dom
 } // namespace mozilla
 %}
 [ref] native const_OriginAttributes(const mozilla::OriginAttributes);
+[ptr] native BrowserParent(mozilla::dom::BrowserParent);
 
 /**
  * nsIOpenWindowInfo is a helper type which contains details used when opening
@@ -34,8 +38,9 @@ interface nsIOpenWindowInfo : nsISupports {
   [infallible]
   readonly attribute boolean forceNoOpener;
 
-  [infallible]
-  readonly attribute unsigned long long nextRemoteBrowserId;
+  /** BrowserParent instance to use in the new window */
+  [notxpcom, nostdcall]
+  BrowserParent getNextRemoteBrowser();
 
   /** Origin Attributes for the to-be-created toplevel BrowsingContext */
   [implicit_jscontext, binaryname(ScriptableOriginAttributes)]

toolkit/components/windowwatcher/nsOpenWindowInfo.cpp

@@ -7,6 +7,7 @@
 #include "nsOpenWindowInfo.h"
 #include "mozilla/OriginAttributes.h"
 #include "mozilla/dom/ToJSValue.h"
+#include "mozilla/dom/BrowserParent.h"
 
 NS_IMPL_ISUPPORTS(nsOpenWindowInfo, nsIOpenWindowInfo)
 
@@ -37,8 +38,6 @@ const OriginAttributes& nsOpenWindowInfo::GetOriginAttributes() {
   return mOriginAttributes;
 }
 
-NS_IMETHODIMP nsOpenWindowInfo::GetNextRemoteBrowserId(
-    uint64_t* aNextRemoteBrowserId) {
-  *aNextRemoteBrowserId = mNextRemoteBrowserId;
-  return NS_OK;
+BrowserParent* nsOpenWindowInfo::GetNextRemoteBrowser() {
+  return mNextRemoteBrowser;
 }

toolkit/components/windowwatcher/nsOpenWindowInfo.h

@@ -19,7 +19,7 @@ class nsOpenWindowInfo : public nsIOpenWindowInfo {
 
   bool mForceNoOpener = false;
   bool mIsRemote = false;
-  uint64_t mNextRemoteBrowserId = 0;
+  RefPtr<BrowserParent> mNextRemoteBrowser;
   OriginAttributes mOriginAttributes;
   RefPtr<BrowsingContext> mParent;
 

xpfe/appshell/AppWindow.cpp

@@ -2175,7 +2175,7 @@ NS_IMETHODIMP AppWindow::CreateNewContentWindow(
   NS_ENSURE_STATE(appWin->mPrimaryContentShell ||
                   appWin->mPrimaryBrowserParent);
   MOZ_ASSERT_IF(appWin->mPrimaryContentShell,
-                aOpenWindowInfo->GetNextRemoteBrowserId() == 0);
+                !aOpenWindowInfo->GetNextRemoteBrowser());
 
   newWindow.forget(_retval);