From 56b9a33c060ad6e9ed568cb610d717b9ce5818c6 Mon Sep 17 00:00:00 2001 From: Andreas Farre Date: Mon, 5 Jul 2021 09:30:53 +0000 Subject: [PATCH] Bug 1701303 - Move counting of private browsing contexts to parent process. r=smaug,johannh Move the counting of private browsing contexts to the parent process. Also change to only consider non-chrome browsing contexts when counting private contexts. The latter is possible due to bug 1528115, because we no longer need to support hidden private windows. With counting in the parent process we can make sure that when we're changing remoteness on a private browsing context the private browsing context count never drops to zero. This fixes an issue with Fission, where we remoteness changes could transiently have a zero private browsing context count, that would be mistaken for the last private browsing context going away. Changing to only count non-chrome browsing contexts makes us only fire 'last-pb-context-exited' once, and since we count them in the parent there is no missing information about contexts that makes us wait for a content process about telling us about insertion or removal of browsing contexts. Differential Revision: https://phabricator.services.mozilla.com/D118182 --- .../privatebrowsing/test/browser/browser.ini | 1 + ...ng_last_private_browsing_context_exited.js | 66 +++++++++++++++ docshell/base/BrowsingContext.cpp | 8 ++ docshell/base/CanonicalBrowsingContext.cpp | 84 +++++++++++++++++++ docshell/base/CanonicalBrowsingContext.h | 7 ++ docshell/base/nsDocShell.cpp | 71 ---------------- docshell/base/nsDocShell.h | 1 - docshell/base/nsIDocShell.idl | 2 - dom/ipc/BrowserChild.cpp | 4 - dom/ipc/ContentParent.cpp | 32 ------- dom/ipc/ContentParent.h | 2 - dom/ipc/PContent.ipdl | 3 - dom/xul/ChromeObserver.cpp | 3 - dom/xul/nsXULElement.cpp | 23 ----- dom/xul/nsXULElement.h | 2 - 15 files changed, 166 insertions(+), 143 deletions(-) create mode 100644 browser/components/privatebrowsing/test/browser/browser_privatebrowsing_last_private_browsing_context_exited.js diff --git a/browser/components/privatebrowsing/test/browser/browser.ini b/browser/components/privatebrowsing/test/browser/browser.ini index 37f8e8f9304c..8d423be009db 100644 --- a/browser/components/privatebrowsing/test/browser/browser.ini +++ b/browser/components/privatebrowsing/test/browser/browser.ini @@ -42,6 +42,7 @@ skip-if = verify [browser_privatebrowsing_downloadLastDir_toggle.js] [browser_privatebrowsing_favicon.js] [browser_privatebrowsing_history_shift_click.js] +[browser_privatebrowsing_last_private_browsing_context_exited.js] [browser_privatebrowsing_lastpbcontextexited.js] [browser_privatebrowsing_localStorage.js] [browser_privatebrowsing_localStorage_before_after.js] diff --git a/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_last_private_browsing_context_exited.js b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_last_private_browsing_context_exited.js new file mode 100644 index 000000000000..1fd28d4ca63a --- /dev/null +++ b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_last_private_browsing_context_exited.js @@ -0,0 +1,66 @@ +/* Any copyright is dedicated to the Public Domain. + http://creativecommons.org/publicdomain/zero/1.0/ */ + +"use strict"; + +add_task(async function test_no_notification_when_pb_autostart() { + let observedLastPBContext = false; + let observerExited = { + observe(aSubject, aTopic, aData) { + observedLastPBContext = true; + }, + }; + Services.obs.addObserver(observerExited, "last-pb-context-exited"); + + await SpecialPowers.pushPrefEnv({ + set: [["browser.privatebrowsing.autostart", true]], + }); + + let win = await BrowserTestUtils.openNewBrowserWindow(); + + let browser = win.gBrowser.selectedTab.linkedBrowser; + ok(browser.browsingContext.usePrivateBrowsing, "should use private browsing"); + + await BrowserTestUtils.closeWindow(win); + + await SpecialPowers.popPrefEnv(); + Services.obs.removeObserver(observerExited, "last-pb-context-exited"); + ok(!observedLastPBContext, "No last-pb-context-exited notification seen"); +}); + +add_task(async function test_notification_when_about_preferences() { + let observedLastPBContext = false; + let observerExited = { + observe(aSubject, aTopic, aData) { + observedLastPBContext = true; + }, + }; + Services.obs.addObserver(observerExited, "last-pb-context-exited"); + + let win = await BrowserTestUtils.openNewBrowserWindow({ private: true }); + + let browser = win.gBrowser.selectedTab.linkedBrowser; + ok(browser.browsingContext.usePrivateBrowsing, "should use private browsing"); + ok(browser.browsingContext.isContent, "should be content browsing context"); + + let tab = await BrowserTestUtils.addTab(win.gBrowser, "about:preferences"); + ok( + tab.linkedBrowser.browsingContext.usePrivateBrowsing, + "should use private browsing" + ); + ok( + tab.linkedBrowser.browsingContext.isContent, + "should be content browsing context" + ); + + let tabClose = BrowserTestUtils.waitForTabClosing(win.gBrowser.selectedTab); + BrowserTestUtils.removeTab(win.gBrowser.selectedTab); + await tabClose; + + ok(!observedLastPBContext, "No last-pb-context-exited notification seen"); + + await BrowserTestUtils.closeWindow(win); + + Services.obs.removeObserver(observerExited, "last-pb-context-exited"); + ok(observedLastPBContext, "No last-pb-context-exited notification seen"); +}); diff --git a/docshell/base/BrowsingContext.cpp b/docshell/base/BrowsingContext.cpp index e0ec4ed9cc65..466c28d578ec 100644 --- a/docshell/base/BrowsingContext.cpp +++ b/docshell/base/BrowsingContext.cpp @@ -799,6 +799,10 @@ void BrowsingContext::Attach(bool aFromIPC, ContentParent* aOriginProcess) { obs->NotifyWhenScriptSafe(ToSupports(this), "browsing-context-attached", nullptr); } + + if (XRE_IsParentProcess()) { + Canonical()->CanonicalAttach(); + } } void BrowsingContext::Detach(bool aFromIPC) { @@ -1588,6 +1592,10 @@ NS_IMETHODIMP BrowsingContext::SetPrivateBrowsing(bool aPrivateBrowsing) { if (IsContent()) { mOriginAttributes.SyncAttributesWithPrivateBrowsing(aPrivateBrowsing); } + + if (XRE_IsParentProcess()) { + Canonical()->AdjustPrivateBrowsingCount(aPrivateBrowsing); + } } AssertOriginAttributesMatchPrivateBrowsing(); diff --git a/docshell/base/CanonicalBrowsingContext.cpp b/docshell/base/CanonicalBrowsingContext.cpp index 177d3135ba4c..808d1ef86e64 100644 --- a/docshell/base/CanonicalBrowsingContext.cpp +++ b/docshell/base/CanonicalBrowsingContext.cpp @@ -27,6 +27,7 @@ #include "mozilla/NullPrincipal.h" #include "mozilla/StaticPrefs_docshell.h" #include "mozilla/StaticPrefs_fission.h" +#include "mozilla/Telemetry.h" #include "nsIWebNavigation.h" #include "mozilla/MozPromiseInlines.h" #include "nsDocShell.h" @@ -60,6 +61,49 @@ extern mozilla::LazyLogModule gSHIPBFCacheLog; #define AUTOPLAY_LOG(msg, ...) \ MOZ_LOG(gAutoplayPermissionLog, LogLevel::Debug, (msg, ##__VA_ARGS__)) +static mozilla::LazyLogModule sPBContext("PBContext"); + +// Global count of canonical browsing contexts with the private attribute set +static uint32_t gNumberOfPrivateContexts = 0; + +static void IncreasePrivateCount() { + gNumberOfPrivateContexts++; + MOZ_LOG(sPBContext, mozilla::LogLevel::Debug, + ("%s: Private browsing context count %d -> %d", __func__, + gNumberOfPrivateContexts - 1, gNumberOfPrivateContexts)); + if (gNumberOfPrivateContexts > 1) { + return; + } + + static bool sHasSeenPrivateContext = false; + if (!sHasSeenPrivateContext) { + sHasSeenPrivateContext = true; + mozilla::Telemetry::ScalarSet( + mozilla::Telemetry::ScalarID::DOM_PARENTPROCESS_PRIVATE_WINDOW_USED, + true); + } +} + +static void DecreasePrivateCount() { + MOZ_ASSERT(gNumberOfPrivateContexts > 0); + gNumberOfPrivateContexts--; + + MOZ_LOG(sPBContext, mozilla::LogLevel::Debug, + ("%s: Private browsing context count %d -> %d", __func__, + gNumberOfPrivateContexts + 1, gNumberOfPrivateContexts)); + if (!gNumberOfPrivateContexts && + !mozilla::Preferences::GetBool("browser.privatebrowsing.autostart")) { + nsCOMPtr observerService = + mozilla::services::GetObserverService(); + if (observerService) { + MOZ_LOG(sPBContext, mozilla::LogLevel::Debug, + ("%s: last-pb-context-exited fired", __func__)); + observerService->NotifyObservers(nullptr, "last-pb-context-exited", + nullptr); + } + } +} + namespace mozilla { namespace dom { @@ -1089,6 +1133,30 @@ void CanonicalBrowsingContext::CanonicalDiscard() { } CancelSessionStoreUpdate(); + + if (UsePrivateBrowsing() && EverAttached() && IsContent()) { + DecreasePrivateCount(); + } +} + +void CanonicalBrowsingContext::CanonicalAttach() { + if (UsePrivateBrowsing() && IsContent()) { + IncreasePrivateCount(); + } +} + +void CanonicalBrowsingContext::AdjustPrivateBrowsingCount( + bool aPrivateBrowsing) { + if (IsDiscarded() || !EverAttached() || IsChrome()) { + return; + } + + MOZ_DIAGNOSTIC_ASSERT(aPrivateBrowsing == UsePrivateBrowsing()); + if (aPrivateBrowsing) { + IncreasePrivateCount(); + } else { + DecreasePrivateCount(); + } } void CanonicalBrowsingContext::NotifyStartDelayedAutoplayMedia() { @@ -1372,6 +1440,22 @@ nsresult CanonicalBrowsingContext::PendingRemotenessChange::FinishTopContent() { MOZ_RELEASE_ASSERT(frameLoaderOwner, "embedder browser must be nsFrameLoaderOwner"); + // If we're process switching a browsing context in private browsing + // mode we might decrease the private browsing count to '0', which + // would make us fire "last-pb-context-exited" and drop the private + // session. To prevent that we artificially increment the number of + // private browsing contexts with '1' until the process switch is done. + bool usePrivateBrowsing = mTarget->UsePrivateBrowsing(); + if (usePrivateBrowsing) { + IncreasePrivateCount(); + } + + auto restorePrivateCount = MakeScopeExit([usePrivateBrowsing]() { + if (usePrivateBrowsing) { + DecreasePrivateCount(); + } + }); + // Tell frontend code that this browser element is about to change process. nsresult rv = browser->BeforeChangeRemoteness(); if (NS_FAILED(rv)) { diff --git a/docshell/base/CanonicalBrowsingContext.h b/docshell/base/CanonicalBrowsingContext.h index a277c4ddedd6..c0ddd5722405 100644 --- a/docshell/base/CanonicalBrowsingContext.h +++ b/docshell/base/CanonicalBrowsingContext.h @@ -335,6 +335,13 @@ class CanonicalBrowsingContext final : public BrowsingContext { // Called when the browsing context is being discarded. void CanonicalDiscard(); + // Called when the browsing context is being attached. + void CanonicalAttach(); + + // Called when the browsing context private mode is changed after + // being attached, but before being discarded. + void AdjustPrivateBrowsingCount(bool aPrivateBrowsing); + using Type = BrowsingContext::Type; CanonicalBrowsingContext(WindowContext* aParentWindow, BrowsingContextGroup* aGroup, diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp index 5f74b97bde24..b93d4f64b693 100644 --- a/docshell/base/nsDocShell.cpp +++ b/docshell/base/nsDocShell.cpp @@ -307,33 +307,6 @@ static void FavorPerformanceHint(bool aPerfOverStarvation) { } } -static void IncreasePrivateDocShellCount() { - gNumberOfPrivateDocShells++; - if (gNumberOfPrivateDocShells > 1 || !XRE_IsContentProcess()) { - return; - } - - mozilla::dom::ContentChild* cc = mozilla::dom::ContentChild::GetSingleton(); - cc->SendPrivateDocShellsExist(true); -} - -static void DecreasePrivateDocShellCount() { - MOZ_ASSERT(gNumberOfPrivateDocShells > 0); - gNumberOfPrivateDocShells--; - if (!gNumberOfPrivateDocShells) { - if (XRE_IsContentProcess()) { - dom::ContentChild* cc = dom::ContentChild::GetSingleton(); - cc->SendPrivateDocShellsExist(false); - return; - } - - nsCOMPtr obsvc = services::GetObserverService(); - if (obsvc) { - obsvc->NotifyObservers(nullptr, "last-pb-context-exited", nullptr); - } - } -} - static bool IsTopLevelDoc(BrowsingContext* aBrowsingContext, nsILoadInfo* aLoadInfo) { MOZ_ASSERT(aBrowsingContext); @@ -411,7 +384,6 @@ nsDocShell::nsDocShell(BrowsingContext* aBrowsingContext, mIsBeingDestroyed(false), mIsExecutingOnLoadHandler(false), mSavingOldViewer(false), - mAffectPrivateSessionLifetime(true), mInvisible(false), mHasLoadedNonBlankURI(false), mBlankTiming(false), @@ -1717,14 +1689,6 @@ nsDocShell::GetUsePrivateBrowsing(bool* aUsePrivateBrowsing) { void nsDocShell::NotifyPrivateBrowsingChanged() { MOZ_ASSERT(!mIsBeingDestroyed); - if (mAffectPrivateSessionLifetime) { - if (UsePrivateBrowsing()) { - IncreasePrivateDocShellCount(); - } else { - DecreasePrivateDocShellCount(); - } - } - nsTObserverArray::ForwardIterator iter(mPrivacyObservers); while (iter.HasMore()) { nsWeakPtr ref = iter.GetNext(); @@ -1777,35 +1741,6 @@ nsDocShell::SetRemoteSubframes(bool aUseRemoteSubframes) { return mBrowsingContext->SetRemoteSubframes(aUseRemoteSubframes); } -NS_IMETHODIMP -nsDocShell::SetAffectPrivateSessionLifetime(bool aAffectLifetime) { - MOZ_ASSERT(!mIsBeingDestroyed); - - bool change = aAffectLifetime != mAffectPrivateSessionLifetime; - if (change && UsePrivateBrowsing()) { - if (aAffectLifetime) { - IncreasePrivateDocShellCount(); - } else { - DecreasePrivateDocShellCount(); - } - } - mAffectPrivateSessionLifetime = aAffectLifetime; - - for (auto* child : mChildList.ForwardRange()) { - nsCOMPtr shell = do_QueryObject(child); - if (shell) { - shell->SetAffectPrivateSessionLifetime(aAffectLifetime); - } - } - return NS_OK; -} - -NS_IMETHODIMP -nsDocShell::GetAffectPrivateSessionLifetime(bool* aAffectLifetime) { - *aAffectLifetime = mAffectPrivateSessionLifetime; - return NS_OK; -} - NS_IMETHODIMP nsDocShell::AddWeakPrivacyTransitionObserver( nsIPrivacyTransitionObserver* aObserver) { @@ -2601,8 +2536,6 @@ nsresult nsDocShell::SetDocLoaderParent(nsDocLoader* aParent) { value = false; } SetAllowDNSPrefetch(mAllowDNSPrefetch && value); - SetAffectPrivateSessionLifetime( - parentAsDocShell->GetAffectPrivateSessionLifetime()); // We don't need to inherit metaViewportOverride, because the viewport // is only relevant for the outermost nsDocShell, not for any iframes @@ -4527,10 +4460,6 @@ nsDocShell::Destroy() { // to break the cycle between us and the timers. CancelRefreshURITimers(); - if (UsePrivateBrowsing() && mAffectPrivateSessionLifetime) { - DecreasePrivateDocShellCount(); - } - return NS_OK; } diff --git a/docshell/base/nsDocShell.h b/docshell/base/nsDocShell.h index ee3d3b69cf8d..0be1c873204e 100644 --- a/docshell/base/nsDocShell.h +++ b/docshell/base/nsDocShell.h @@ -1282,7 +1282,6 @@ class nsDocShell final : public nsDocLoader, // should be passed a SHEntry to save itself into. bool mSavingOldViewer : 1; - bool mAffectPrivateSessionLifetime : 1; bool mInvisible : 1; bool mHasLoadedNonBlankURI : 1; diff --git a/docshell/base/nsIDocShell.idl b/docshell/base/nsIDocShell.idl index 6ae176e22a80..352b70d12030 100644 --- a/docshell/base/nsIDocShell.idl +++ b/docshell/base/nsIDocShell.idl @@ -605,8 +605,6 @@ interface nsIDocShell : nsIDocShellTreeItem */ [noscript, notxpcom] bool pluginsAllowedInCurrentDoc(); - [noscript, infallible] attribute boolean affectPrivateSessionLifetime; - /** * Indicates whether the UI may enable the character encoding menu. The UI * must disable the menu when this property is false. diff --git a/dom/ipc/BrowserChild.cpp b/dom/ipc/BrowserChild.cpp index 00ba87d396a7..4441cde15347 100644 --- a/dom/ipc/BrowserChild.cpp +++ b/dom/ipc/BrowserChild.cpp @@ -495,10 +495,6 @@ nsresult BrowserChild::Init(mozIDOMWindowProxy* aParent, NS_ENSURE_SUCCESS(rv, rv); } - docShell->SetAffectPrivateSessionLifetime( - mBrowsingContext->UsePrivateBrowsing() || - mChromeFlags & nsIWebBrowserChrome::CHROME_PRIVATE_LIFETIME); - #ifdef DEBUG nsCOMPtr loadContext = do_GetInterface(WebNavigation()); MOZ_ASSERT(loadContext); diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp index 2290824be23c..d9cd6248866f 100644 --- a/dom/ipc/ContentParent.cpp +++ b/dom/ipc/ContentParent.cpp @@ -1539,9 +1539,6 @@ already_AddRefed ContentParent::CreateBrowser( if (loadContext && loadContext->UseRemoteSubframes()) { chromeFlags |= nsIWebBrowserChrome::CHROME_FISSION_WINDOW; } - if (docShell->GetAffectPrivateSessionLifetime()) { - chromeFlags |= nsIWebBrowserChrome::CHROME_PRIVATE_LIFETIME; - } if (tabId == 0) { return nullptr; @@ -4766,35 +4763,6 @@ mozilla::ipc::IPCResult ContentParent::RecvScriptErrorInternal( return IPC_OK(); } -mozilla::ipc::IPCResult ContentParent::RecvPrivateDocShellsExist( - const bool& aExist) { - if (!sPrivateContent) { - sPrivateContent = MakeUnique>(); - if (!sHasSeenPrivateDocShell) { - sHasSeenPrivateDocShell = true; - Telemetry::ScalarSet( - Telemetry::ScalarID::DOM_PARENTPROCESS_PRIVATE_WINDOW_USED, true); - } - } - if (aExist) { - sPrivateContent->AppendElement(this); - } else { - sPrivateContent->RemoveElement(this); - - // Only fire the notification if we have private and non-private - // windows: if privatebrowsing.autostart is true, all windows are - // private. - if (!sPrivateContent->Length() && - !Preferences::GetBool("browser.privatebrowsing.autostart")) { - nsCOMPtr obs = - mozilla::services::GetObserverService(); - obs->NotifyObservers(nullptr, "last-pb-context-exited", nullptr); - sPrivateContent = nullptr; - } - } - return IPC_OK(); -} - bool ContentParent::DoLoadMessageManagerScript(const nsAString& aURL, bool aRunInGlobalScope) { MOZ_ASSERT(!aRunInGlobalScope); diff --git a/dom/ipc/ContentParent.h b/dom/ipc/ContentParent.h index 6d62062decd0..c59adbb02801 100644 --- a/dom/ipc/ContentParent.h +++ b/dom/ipc/ContentParent.h @@ -1118,8 +1118,6 @@ class ContentParent final const ClonedMessageData* aStack = nullptr); public: - mozilla::ipc::IPCResult RecvPrivateDocShellsExist(const bool& aExist); - mozilla::ipc::IPCResult RecvCommitBrowsingContextTransaction( const MaybeDiscarded& aContext, BrowsingContext::BaseTransaction&& aTransaction, uint64_t aEpoch); diff --git a/dom/ipc/PContent.ipdl b/dom/ipc/PContent.ipdl index 763b37bd1934..4fc8c003e60c 100644 --- a/dom/ipc/PContent.ipdl +++ b/dom/ipc/PContent.ipdl @@ -1154,9 +1154,6 @@ parent: sync GetIconForExtension(nsCString aFileExt, uint32_t aIconSize) returns (uint8_t[] bits); - // Notify the parent of the presence or absence of private docshells - async PrivateDocShellsExist(bool aExist); - // Tell the parent that the child has gone idle for the first time. async FirstIdle(); diff --git a/dom/xul/ChromeObserver.cpp b/dom/xul/ChromeObserver.cpp index afeb553df540..b0ded0486feb 100644 --- a/dom/xul/ChromeObserver.cpp +++ b/dom/xul/ChromeObserver.cpp @@ -162,9 +162,6 @@ void ChromeObserver::AttributeChanged(dom::Element* aElement, HideWindowChrome(value->Equals(u"true"_ns, eCaseMatters)); } else if (aName == nsGkAtoms::chromemargin) { SetChromeMargins(value); - } else if (aName == nsGkAtoms::windowtype && aElement->IsXULElement()) { - RefPtr xulElement = nsXULElement::FromNodeOrNull(aElement); - xulElement->MaybeUpdatePrivateLifetime(); } // title and drawintitlebar are settable on // any root node (windows, dialogs, etc) diff --git a/dom/xul/nsXULElement.cpp b/dom/xul/nsXULElement.cpp index 2bd8b738866d..db6707beca45 100644 --- a/dom/xul/nsXULElement.cpp +++ b/dom/xul/nsXULElement.cpp @@ -134,21 +134,6 @@ nsXULElement::nsXULElement(already_AddRefed&& aNodeInfo) nsXULElement::~nsXULElement() = default; -void nsXULElement::MaybeUpdatePrivateLifetime() { - if (AttrValueIs(kNameSpaceID_None, nsGkAtoms::windowtype, - u"navigator:browser"_ns, eCaseMatters) || - AttrValueIs(kNameSpaceID_None, nsGkAtoms::windowtype, - u"navigator:geckoview"_ns, eCaseMatters)) { - return; - } - - nsPIDOMWindowOuter* win = OwnerDoc()->GetWindow(); - nsCOMPtr docShell = win ? win->GetDocShell() : nullptr; - if (docShell) { - docShell->SetAffectPrivateSessionLifetime(false); - } -} - /* static */ nsXULElement* NS_NewBasicXULElement( already_AddRefed&& aNodeInfo) { @@ -234,14 +219,6 @@ already_AddRefed nsXULElement::CreateFromPrototype( } } - if (aIsRoot && aPrototype->mNodeInfo->Equals(nsGkAtoms::window)) { - for (size_t i = 0; i < aPrototype->mAttributes.Length(); ++i) { - if (aPrototype->mAttributes[i].mName.Equals(nsGkAtoms::windowtype)) { - element->MaybeUpdatePrivateLifetime(); - } - } - } - return baseElement.forget().downcast(); } diff --git a/dom/xul/nsXULElement.h b/dom/xul/nsXULElement.h index 40985432e05e..affe1d7f7906 100644 --- a/dom/xul/nsXULElement.h +++ b/dom/xul/nsXULElement.h @@ -504,8 +504,6 @@ class nsXULElement : public nsStyledElement { bool IsInteractiveHTMLContent() const override; - void MaybeUpdatePrivateLifetime(); - protected: ~nsXULElement();