From 09988936b70ceaecb80cf29890dcb9ec4724ea45 Mon Sep 17 00:00:00 2001 From: Nika Layzell Date: Tue, 7 Apr 2020 21:39:59 +0000 Subject: [PATCH] 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 | 39 +++++++++------- dom/ipc/ContentParent.cpp | 45 +++++-------------- dom/ipc/ContentParent.h | 15 +++---- .../windowwatcher/nsIOpenWindowInfo.idl | 9 +++- .../windowwatcher/nsOpenWindowInfo.cpp | 7 ++- .../windowwatcher/nsOpenWindowInfo.h | 2 +- xpfe/appshell/AppWindow.cpp | 2 +- 7 files changed, 54 insertions(+), 65 deletions(-) diff --git a/dom/base/nsFrameLoader.cpp b/dom/base/nsFrameLoader.cpp index b2f007096b54..8382d3a6ed44 100644 --- a/dom/base/nsFrameLoader.cpp +++ b/dom/base/nsFrameLoader.cpp @@ -259,6 +259,14 @@ static bool IsTopContent(BrowsingContext* aParent, Element* aOwner) { static already_AddRefed 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 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 ownerElement = mOwnerContent; - mRemoteBrowser = ContentParent::CreateBrowser( - context, ownerElement, mRemoteType, mPendingBrowsingContext, - openerContentParent, nextRemoteBrowserId); + RefPtr 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(); diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp index 65c6cbc63bb5..1dea4674bc76 100644 --- a/dom/ipc/ContentParent.cpp +++ b/dom/ipc/ContentParent.cpp @@ -586,9 +586,6 @@ StaticAutoPtr> ContentParent::sContentParents; UniquePtr ContentParent::sSandboxBrokerPolicyFactory; #endif -uint64_t ContentParent::sNextRemoteTabId = 0; -nsDataHashtable - ContentParent::sNextBrowserParents; StaticAutoPtr>> ContentParent::sBrowsingContextGroupHolder; #if defined(XP_MACOSX) && defined(MOZ_SANDBOX) @@ -1193,7 +1190,7 @@ mozilla::ipc::IPCResult ContentParent::RecvLaunchRDDProcess( already_AddRefed 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 ContentParent::CreateBrowser( remoteType.AssignLiteral(DEFAULT_REMOTE_TYPE); } - if (aNextRemoteTabId) { - if (BrowserParent* parent = - sNextBrowserParents.GetAndRemove(aNextRemoteTabId) - .valueOr(nullptr)) { - RefPtr 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& 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& 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 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 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) { diff --git a/dom/ipc/ContentParent.h b/dom/ipc/ContentParent.h index 3298528769ab..7f3f92c770aa 100644 --- a/dom/ipc/ContentParent.h +++ b/dom/ipc/ContentParent.h @@ -230,7 +230,7 @@ class ContentParent final static already_AddRefed CreateBrowser( const TabContext& aContext, Element* aFrameElement, const nsAString& aRemoteType, BrowsingContext* aBrowsingContext, - ContentParent* aOpenerContentParent, uint64_t aNextRemoteTabId); + ContentParent* aOpenerContentParent); static void GetAll(nsTArray& 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& aNewRemoteTab, bool* aWindowIsNew, - int32_t& aOpenLocation, nsIPrincipal* aTriggeringPrincipal, - nsIReferrerInfo* aReferrerInfo, bool aLoadUri, - nsIContentSecurityPolicy* aCsp, + BrowserParent* aNextRemoteBrowser, const nsString& aName, + nsresult& aResult, nsCOMPtr& 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 mMessageManager; - static uint64_t sNextRemoteTabId; - static nsDataHashtable sNextBrowserParents; - static StaticAutoPtr>> sBrowsingContextGroupHolder; diff --git a/toolkit/components/windowwatcher/nsIOpenWindowInfo.idl b/toolkit/components/windowwatcher/nsIOpenWindowInfo.idl index 2dd62dfe1ffc..cc911326562b 100644 --- a/toolkit/components/windowwatcher/nsIOpenWindowInfo.idl +++ b/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)] diff --git a/toolkit/components/windowwatcher/nsOpenWindowInfo.cpp b/toolkit/components/windowwatcher/nsOpenWindowInfo.cpp index 363b7731346a..090a3ec414c5 100644 --- a/toolkit/components/windowwatcher/nsOpenWindowInfo.cpp +++ b/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; } diff --git a/toolkit/components/windowwatcher/nsOpenWindowInfo.h b/toolkit/components/windowwatcher/nsOpenWindowInfo.h index b0801b588541..c5743ff544fe 100644 --- a/toolkit/components/windowwatcher/nsOpenWindowInfo.h +++ b/toolkit/components/windowwatcher/nsOpenWindowInfo.h @@ -19,7 +19,7 @@ class nsOpenWindowInfo : public nsIOpenWindowInfo { bool mForceNoOpener = false; bool mIsRemote = false; - uint64_t mNextRemoteBrowserId = 0; + RefPtr mNextRemoteBrowser; OriginAttributes mOriginAttributes; RefPtr mParent; diff --git a/xpfe/appshell/AppWindow.cpp b/xpfe/appshell/AppWindow.cpp index 9304b9c1e57b..ef9144876cfb 100644 --- a/xpfe/appshell/AppWindow.cpp +++ b/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);