From b68275373efa548226a314e91bd02f1b9dba453c Mon Sep 17 00:00:00 2001 From: Kris Maglione Date: Mon, 14 Dec 2020 21:25:46 +0000 Subject: [PATCH] Bug 1672479: Fix beforeunload handling in location.refresh under SHiP. r=peterv Per spec, whenever a "beforeunload" event handler which would affect a `location.reload()` call exists, it must be called before the `reload()` call returns. If a handler requests to block the navigation and we choose to display a confirmation prompt, that must also be displayed before the call returns. With session history in parent, though, that currently does not happen, because `location.reload()` triggers an async IPC call to the parent process, and only attempts the actual reload (and thus beforeunload dispatch and prompting) once it returns, which is too late to affect the caller. This patch changes the handling in this case to manually perform permit unload checks before taking an async code path. This still leaves the opportunity for session history handlers in the parent to cancel the load asynchronously, but that doesn't violate any spec-defined behavior. Differential Revision: https://phabricator.services.mozilla.com/D94354 --- docshell/base/nsDocShell.cpp | 22 +++++++++++++++---- docshell/base/nsDocShell.h | 6 ++++- docshell/base/nsDocShellLoadState.cpp | 14 +++++++++++- docshell/base/nsDocShellLoadState.h | 10 +++++++++ docshell/test/browser/browser.ini | 1 - .../test/browser/browser_onbeforeunload.js | 4 ++-- 6 files changed, 48 insertions(+), 9 deletions(-) diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp index 8289fc74a66f..2a207f8a4e92 100644 --- a/docshell/base/nsDocShell.cpp +++ b/docshell/base/nsDocShell.cpp @@ -3993,10 +3993,19 @@ nsDocShell::Reload(uint32_t aReloadFlags) { bool forceReload = IsForceReloadType(loadType); if (!XRE_IsParentProcess()) { RefPtr docShell(this); + nsCOMPtr cv(mContentViewer); + + bool okToUnload = true; + MOZ_TRY(cv->PermitUnload(&okToUnload)); + if (!okToUnload) { + return NS_OK; + } + RefPtr doc(GetDocument()); RefPtr browsingContext(mBrowsingContext); nsCOMPtr currentURI(mCurrentURI); nsCOMPtr referrerInfo(mReferrerInfo); + ContentChild::GetSingleton()->SendNotifyOnHistoryReload( mBrowsingContext, forceReload, [docShell, doc, loadType, browsingContext, currentURI, referrerInfo]( @@ -4016,16 +4025,18 @@ nsDocShell::Reload(uint32_t aReloadFlags) { MOZ_LOG( gSHLog, LogLevel::Debug, ("nsDocShell %p Reload - LoadHistoryEntry", docShell.get())); + loadState.ref()->SetNotifiedBeforeUnloadListeners(true); docShell->LoadHistoryEntry(loadState.ref(), loadType, reloadingActiveEntry.ref()); } else { MOZ_LOG(gSHLog, LogLevel::Debug, ("nsDocShell %p ReloadDocument", docShell.get())); ReloadDocument(docShell, doc, loadType, browsingContext, - currentURI, referrerInfo); + currentURI, referrerInfo, + /* aNotifiedBeforeUnloadListeners */ true); } }, - [](ResponseRejectReason) {}); + [](mozilla::ipc::ResponseRejectReason) {}); } else { // Parent process bool canReload = false; @@ -4079,7 +4090,8 @@ nsresult nsDocShell::ReloadDocument(nsDocShell* aDocShell, Document* aDocument, uint32_t aLoadType, BrowsingContext* aBrowsingContext, nsIURI* aCurrentURI, - nsIReferrerInfo* aReferrerInfo) { + nsIReferrerInfo* aReferrerInfo, + bool aNotifiedBeforeUnloadListeners) { if (!aDocument) { return NS_OK; } @@ -4152,6 +4164,7 @@ nsresult nsDocShell::ReloadDocument(nsDocShell* aDocShell, Document* aDocument, loadState->SetBaseURI(baseURI); loadState->SetHasValidUserGestureActivation( context && context->HasValidTransientUserGestureActivation()); + loadState->SetNotifiedBeforeUnloadListeners(aNotifiedBeforeUnloadListeners); return aDocShell->InternalLoad(loadState); } @@ -9194,7 +9207,8 @@ nsresult nsDocShell::InternalLoad(nsDocShellLoadState* aLoadState, } // Check if the page doesn't want to be unloaded. The javascript: // protocol handler deals with this for javascript: URLs. - if (!isJavaScript && isNotDownload && mContentViewer) { + if (!isJavaScript && isNotDownload && + !aLoadState->NotifiedBeforeUnloadListeners() && mContentViewer) { bool okToUnload; rv = mContentViewer->PermitUnload(&okToUnload); diff --git a/docshell/base/nsDocShell.h b/docshell/base/nsDocShell.h index e00f41b5bd46..957100612b75 100644 --- a/docshell/base/nsDocShell.h +++ b/docshell/base/nsDocShell.h @@ -645,10 +645,14 @@ class nsDocShell final : public nsDocLoader, void SetHistoryEntryAndUpdateBC(const mozilla::Maybe& aLSHE, const mozilla::Maybe& aOSHE); + // If aNotifiedBeforeUnloadListeners is true, "beforeunload" event listeners + // were notified by the caller and given the chance to abort the navigation, + // and should not be notified again. static nsresult ReloadDocument( nsDocShell* aDocShell, mozilla::dom::Document* aDocument, uint32_t aLoadType, mozilla::dom::BrowsingContext* aBrowsingContext, - nsIURI* aCurrentURI, nsIReferrerInfo* aReferrerInfo); + nsIURI* aCurrentURI, nsIReferrerInfo* aReferrerInfo, + bool aNotifiedBeforeUnloadListeners = false); // // URI Load diff --git a/docshell/base/nsDocShellLoadState.cpp b/docshell/base/nsDocShellLoadState.cpp index 8d9a329eeedf..7d098e180243 100644 --- a/docshell/base/nsDocShellLoadState.cpp +++ b/docshell/base/nsDocShellLoadState.cpp @@ -38,7 +38,8 @@ nsDocShellLoadState::nsDocShellLoadState(nsIURI* aURI) nsDocShellLoadState::nsDocShellLoadState( const DocShellLoadStateInit& aLoadState) - : mLoadIdentifier(aLoadState.LoadIdentifier()) { + : mNotifiedBeforeUnloadListeners(false), + mLoadIdentifier(aLoadState.LoadIdentifier()) { MOZ_ASSERT(aLoadState.URI(), "Cannot create a LoadState with a null URI!"); mResultPrincipalURI = aLoadState.ResultPrincipalURI(); mResultPrincipalURIIsSome = aLoadState.ResultPrincipalURIIsSome(); @@ -94,6 +95,7 @@ nsDocShellLoadState::nsDocShellLoadState(const nsDocShellLoadState& aOther) mLoadReplace(aOther.mLoadReplace), mInheritPrincipal(aOther.mInheritPrincipal), mPrincipalIsExplicit(aOther.mPrincipalIsExplicit), + mNotifiedBeforeUnloadListeners(aOther.mNotifiedBeforeUnloadListeners), mPrincipalToInherit(aOther.mPrincipalToInherit), mPartitionedPrincipalToInherit(aOther.mPartitionedPrincipalToInherit), mForceAllowDataURI(aOther.mForceAllowDataURI), @@ -133,6 +135,7 @@ nsDocShellLoadState::nsDocShellLoadState(nsIURI* aURI, uint64_t aLoadIdentifier) mLoadReplace(false), mInheritPrincipal(false), mPrincipalIsExplicit(false), + mNotifiedBeforeUnloadListeners(false), mForceAllowDataURI(false), mOriginalFrameSrc(false), mIsFormSubmission(false), @@ -465,6 +468,15 @@ void nsDocShellLoadState::SetPrincipalIsExplicit(bool aPrincipalIsExplicit) { mPrincipalIsExplicit = aPrincipalIsExplicit; } +bool nsDocShellLoadState::NotifiedBeforeUnloadListeners() const { + return mNotifiedBeforeUnloadListeners; +} + +void nsDocShellLoadState::SetNotifiedBeforeUnloadListeners( + bool aNotifiedBeforeUnloadListeners) { + mNotifiedBeforeUnloadListeners = aNotifiedBeforeUnloadListeners; +} + bool nsDocShellLoadState::ForceAllowDataURI() const { return mForceAllowDataURI; } diff --git a/docshell/base/nsDocShellLoadState.h b/docshell/base/nsDocShellLoadState.h index 3c75d25461ad..e86d985b6a9b 100644 --- a/docshell/base/nsDocShellLoadState.h +++ b/docshell/base/nsDocShellLoadState.h @@ -120,6 +120,14 @@ class nsDocShellLoadState final { void SetPrincipalIsExplicit(bool aPrincipalIsExplicit); + + // If true, "beforeunload" event listeners were notified by the creater of the + // LoadState and given the chance to abort the navigation, and should not be + // notified again. + bool NotifiedBeforeUnloadListeners() const; + + void SetNotifiedBeforeUnloadListeners(bool aNotifiedBeforeUnloadListeners); + bool ForceAllowDataURI() const; void SetForceAllowDataURI(bool aForceAllowDataURI); @@ -360,6 +368,8 @@ class nsDocShellLoadState final { // for a content docshell the load fails. bool mPrincipalIsExplicit; + bool mNotifiedBeforeUnloadListeners; + // Principal we're inheriting. If null, this means the principal should be // inherited from the current document. If set to NullPrincipal, the channel // will fill in principal information later in the load. See internal comments diff --git a/docshell/test/browser/browser.ini b/docshell/test/browser/browser.ini index c47648e7eb75..9b4b22699bd4 100644 --- a/docshell/test/browser/browser.ini +++ b/docshell/test/browser/browser.ini @@ -133,7 +133,6 @@ support-files = [browser_loadURI_postdata.js] [browser_multiple_pushState.js] [browser_onbeforeunload.js] -skip-if = fission [browser_onbeforeunload_navigation.js] skip-if = (os == 'win' && !debug) # bug 1300351 [browser_onunload_stop.js] diff --git a/docshell/test/browser/browser_onbeforeunload.js b/docshell/test/browser/browser_onbeforeunload.js index 454fad1fdb53..3f92a58e5a2b 100644 --- a/docshell/test/browser/browser_onbeforeunload.js +++ b/docshell/test/browser/browser_onbeforeunload.js @@ -2,7 +2,7 @@ // We need to test a lot of permutations here, and there isn't any sensible way // to split them up or run them faster. -requestLongerTimeout(2); +requestLongerTimeout(4); const BASE_URL = "http://mochi.test:8888/browser/docshell/test/browser/"; @@ -123,7 +123,7 @@ function* generatePermutations(depth) { } } -const PERMUTATIONS = Array.from(generatePermutations(3)); +const PERMUTATIONS = Array.from(generatePermutations(4)); const FRAMES = [ { process: 0 },