From f61ec1c3d77f9f021540641c16b24b80dcb736ab Mon Sep 17 00:00:00 2001 From: Sean Feng Date: Thu, 3 Mar 2022 19:13:47 +0000 Subject: [PATCH] Bug 1741671 - Enable BFCache for pages with beforeunload event listeners on Desktop r=smaug The changes only made it works in SHIP(session-history-in-parent) only. Differential Revision: https://phabricator.services.mozilla.com/D131715 --- docshell/shistory/nsSHistory.cpp | 57 ++++++++-------- .../file_ship_beforeunload_fired.html | 37 +++++++++++ docshell/test/navigation/mochitest.ini | 12 ++++ .../test_ship_beforeunload_fired.html | 63 ++++++++++++++++++ .../test_ship_beforeunload_fired_2.html | 65 +++++++++++++++++++ .../test_ship_beforeunload_fired_3.html | 65 +++++++++++++++++++ dom/base/Document.cpp | 13 ++-- dom/base/nsGlobalWindowInner.cpp | 35 +++++++--- dom/base/nsGlobalWindowInner.h | 2 +- dom/ipc/WindowGlobalParent.cpp | 1 + dom/tests/browser/browser_hasbeforeunload.js | 18 +++++ modules/libpref/init/StaticPrefList.yaml | 7 ++ .../back-forward-cache/events.html.ini | 5 +- .../tests/browser/dummy_page.html | 4 +- 14 files changed, 335 insertions(+), 49 deletions(-) create mode 100644 docshell/test/navigation/file_ship_beforeunload_fired.html create mode 100644 docshell/test/navigation/test_ship_beforeunload_fired.html create mode 100644 docshell/test/navigation/test_ship_beforeunload_fired_2.html create mode 100644 docshell/test/navigation/test_ship_beforeunload_fired_3.html diff --git a/docshell/shistory/nsSHistory.cpp b/docshell/shistory/nsSHistory.cpp index e74b6230d485..95361d4c5d20 100644 --- a/docshell/shistory/nsSHistory.cpp +++ b/docshell/shistory/nsSHistory.cpp @@ -1324,37 +1324,34 @@ void nsSHistory::LoadURIOrBFCache(LoadEntryResult& aLoadEntry) { "saving presentation=%i", canSave)); - if (!canSave) { - nsCOMPtr frameLoaderOwner = - do_QueryInterface(canonicalBC->GetEmbedderElement()); - if (frameLoaderOwner) { - RefPtr currentFrameLoader = - frameLoaderOwner->GetFrameLoader(); - if (currentFrameLoader && - currentFrameLoader->GetMaybePendingBrowsingContext()) { - WindowGlobalParent* wgp = - currentFrameLoader->GetMaybePendingBrowsingContext() - ->Canonical() - ->GetCurrentWindowGlobal(); - if (wgp) { - wgp->PermitUnload([canonicalBC, loadState, she, frameLoader, - currentFrameLoader](bool aAllow) { - if (aAllow) { - FinishRestore(canonicalBC, loadState, she, frameLoader, - false); - } else if (currentFrameLoader - ->GetMaybePendingBrowsingContext()) { - nsISHistory* shistory = - currentFrameLoader->GetMaybePendingBrowsingContext() - ->Canonical() - ->GetSessionHistory(); - if (shistory) { - shistory->InternalSetRequestedIndex(-1); - } + nsCOMPtr frameLoaderOwner = + do_QueryInterface(canonicalBC->GetEmbedderElement()); + if (frameLoaderOwner) { + RefPtr currentFrameLoader = + frameLoaderOwner->GetFrameLoader(); + if (currentFrameLoader && + currentFrameLoader->GetMaybePendingBrowsingContext()) { + if (WindowGlobalParent* wgp = + currentFrameLoader->GetMaybePendingBrowsingContext() + ->Canonical() + ->GetCurrentWindowGlobal()) { + wgp->PermitUnload([canonicalBC, loadState, she, frameLoader, + currentFrameLoader, canSave](bool aAllow) { + if (aAllow) { + FinishRestore(canonicalBC, loadState, she, frameLoader, + canSave && canonicalBC->AllowedInBFCache( + Nothing(), nullptr)); + } else if (currentFrameLoader->GetMaybePendingBrowsingContext()) { + nsISHistory* shistory = + currentFrameLoader->GetMaybePendingBrowsingContext() + ->Canonical() + ->GetSessionHistory(); + if (shistory) { + shistory->InternalSetRequestedIndex(-1); } - }); - return; - } + } + }); + return; } } } diff --git a/docshell/test/navigation/file_ship_beforeunload_fired.html b/docshell/test/navigation/file_ship_beforeunload_fired.html new file mode 100644 index 000000000000..a1f416f959a1 --- /dev/null +++ b/docshell/test/navigation/file_ship_beforeunload_fired.html @@ -0,0 +1,37 @@ + + + diff --git a/docshell/test/navigation/mochitest.ini b/docshell/test/navigation/mochitest.ini index e95a1ac0f7db..c18f2d13aac0 100644 --- a/docshell/test/navigation/mochitest.ini +++ b/docshell/test/navigation/mochitest.ini @@ -200,3 +200,15 @@ support-files = [test_recursive_frames.html] [test_bug1699721.html] skip-if = !fission # tests fission-only process switching behaviour +[test_ship_beforeunload_fired.html] +support-files = + file_ship_beforeunload_fired.html +skip-if = !sessionHistoryInParent +[test_ship_beforeunload_fired_2.html] +support-files = + file_ship_beforeunload_fired.html +skip-if = !sessionHistoryInParent +[test_ship_beforeunload_fired_3.html] +support-files = + file_ship_beforeunload_fired.html +skip-if = !sessionHistoryInParent diff --git a/docshell/test/navigation/test_ship_beforeunload_fired.html b/docshell/test/navigation/test_ship_beforeunload_fired.html new file mode 100644 index 000000000000..e43711676b6f --- /dev/null +++ b/docshell/test/navigation/test_ship_beforeunload_fired.html @@ -0,0 +1,63 @@ + + + + Test that ensures beforeunload is fired when session-history-in-parent is enabled + + + + + + + diff --git a/docshell/test/navigation/test_ship_beforeunload_fired_2.html b/docshell/test/navigation/test_ship_beforeunload_fired_2.html new file mode 100644 index 000000000000..93669502a54e --- /dev/null +++ b/docshell/test/navigation/test_ship_beforeunload_fired_2.html @@ -0,0 +1,65 @@ + + + + Test that ensures beforeunload is fired when session-history-in-parent is enabled + + + + + + + diff --git a/docshell/test/navigation/test_ship_beforeunload_fired_3.html b/docshell/test/navigation/test_ship_beforeunload_fired_3.html new file mode 100644 index 000000000000..8951f269c5d6 --- /dev/null +++ b/docshell/test/navigation/test_ship_beforeunload_fired_3.html @@ -0,0 +1,65 @@ + + + + Test that ensures beforeunload is fired when session-history-in-parent is enabled + + + + + + + diff --git a/dom/base/Document.cpp b/dom/base/Document.cpp index c7f0a8966657..6385f57d75e3 100644 --- a/dom/base/Document.cpp +++ b/dom/base/Document.cpp @@ -11173,10 +11173,15 @@ bool Document::CanSavePresentation(nsIRequest* aNewRequest, ret = false; } if (manager->HasBeforeUnloadListeners()) { - MOZ_LOG(gPageCacheLog, mozilla::LogLevel::Verbose, - ("Save of %s blocked due to beforeUnload handlers", uri.get())); - aBFCacheCombo |= BFCacheStatus::BEFOREUNLOAD_LISTENER; - ret = false; + if (!mozilla::SessionHistoryInParent() || + !StaticPrefs:: + docshell_shistory_bfcache_ship_allow_beforeunload_listeners()) { + MOZ_LOG( + gPageCacheLog, mozilla::LogLevel::Verbose, + ("Save of %s blocked due to beforeUnload handlers", uri.get())); + aBFCacheCombo |= BFCacheStatus::BEFOREUNLOAD_LISTENER; + ret = false; + } } } } diff --git a/dom/base/nsGlobalWindowInner.cpp b/dom/base/nsGlobalWindowInner.cpp index 9deca8cf0087..c5700544ed53 100644 --- a/dom/base/nsGlobalWindowInner.cpp +++ b/dom/base/nsGlobalWindowInner.cpp @@ -81,6 +81,7 @@ #include "mozilla/SpinEventLoopUntil.h" #include "mozilla/Sprintf.h" #include "mozilla/StaticPrefs_browser.h" +#include "mozilla/StaticPrefs_docshell.h" #include "mozilla/StaticPrefs_dom.h" #include "mozilla/StaticPrefs_privacy.h" #include "mozilla/StorageAccess.h" @@ -6590,13 +6591,22 @@ void nsGlobalWindowInner::EventListenerAdded(nsAtom* aType) { mHasVRDisplayActivateEvents = true; } - if ((aType == nsGkAtoms::onunload || aType == nsGkAtoms::onbeforeunload) && - mWindowGlobalChild) { + if (aType == nsGkAtoms::onunload && mWindowGlobalChild) { if (++mUnloadOrBeforeUnloadListenerCount == 1) { mWindowGlobalChild->BlockBFCacheFor(BFCacheStatus::UNLOAD_LISTENER); } - if (aType == nsGkAtoms::onbeforeunload && - (!mDoc || !(mDoc->GetSandboxFlags() & SANDBOXED_MODALS))) { + } + + if (aType == nsGkAtoms::onbeforeunload && mWindowGlobalChild) { + if (!mozilla::SessionHistoryInParent() || + !StaticPrefs:: + docshell_shistory_bfcache_ship_allow_beforeunload_listeners()) { + if (++mUnloadOrBeforeUnloadListenerCount == 1) { + mWindowGlobalChild->BlockBFCacheFor( + BFCacheStatus::BEFOREUNLOAD_LISTENER); + } + } + if (!mDoc || !(mDoc->GetSandboxFlags() & SANDBOXED_MODALS)) { mWindowGlobalChild->BeforeUnloadAdded(); MOZ_ASSERT(mWindowGlobalChild->BeforeUnloadListeners() > 0); } @@ -6618,14 +6628,23 @@ void nsGlobalWindowInner::EventListenerAdded(nsAtom* aType) { } void nsGlobalWindowInner::EventListenerRemoved(nsAtom* aType) { - if ((aType == nsGkAtoms::onunload || aType == nsGkAtoms::onbeforeunload) && - mWindowGlobalChild) { + if (aType == nsGkAtoms::onunload && mWindowGlobalChild) { MOZ_ASSERT(mUnloadOrBeforeUnloadListenerCount > 0); if (--mUnloadOrBeforeUnloadListenerCount == 0) { mWindowGlobalChild->UnblockBFCacheFor(BFCacheStatus::UNLOAD_LISTENER); } - if (aType == nsGkAtoms::onbeforeunload && - (!mDoc || !(mDoc->GetSandboxFlags() & SANDBOXED_MODALS))) { + } + + if (aType == nsGkAtoms::onbeforeunload && mWindowGlobalChild) { + if (!mozilla::SessionHistoryInParent() || + !StaticPrefs:: + docshell_shistory_bfcache_ship_allow_beforeunload_listeners()) { + if (--mUnloadOrBeforeUnloadListenerCount == 0) { + mWindowGlobalChild->UnblockBFCacheFor( + BFCacheStatus::BEFOREUNLOAD_LISTENER); + } + } + if (!mDoc || !(mDoc->GetSandboxFlags() & SANDBOXED_MODALS)) { mWindowGlobalChild->BeforeUnloadRemoved(); MOZ_ASSERT(mWindowGlobalChild->BeforeUnloadListeners() >= 0); } diff --git a/dom/base/nsGlobalWindowInner.h b/dom/base/nsGlobalWindowInner.h index c3c29b90696a..e916a553d016 100644 --- a/dom/base/nsGlobalWindowInner.h +++ b/dom/base/nsGlobalWindowInner.h @@ -1520,7 +1520,7 @@ class nsGlobalWindowInner final : public mozilla::dom::EventTarget, RefPtr mVREventObserver; - // The number of unload and beforeunload even listeners registered on this + // The number of unload and beforeunload event listeners registered on this // window. uint64_t mUnloadOrBeforeUnloadListenerCount = 0; diff --git a/dom/ipc/WindowGlobalParent.cpp b/dom/ipc/WindowGlobalParent.cpp index 1cc0787ad633..289759230a74 100644 --- a/dom/ipc/WindowGlobalParent.cpp +++ b/dom/ipc/WindowGlobalParent.cpp @@ -1388,6 +1388,7 @@ nsCString BFCacheStatusToString(uint32_t aFlags) { ADD_BFCACHESTATUS_TO_STRING(HAS_USED_VR); ADD_BFCACHESTATUS_TO_STRING(CONTAINS_REMOTE_SUBFRAMES); ADD_BFCACHESTATUS_TO_STRING(NOT_ONLY_TOPLEVEL_IN_BCG); + ADD_BFCACHESTATUS_TO_STRING(BEFOREUNLOAD_LISTENER); ADD_BFCACHESTATUS_TO_STRING(ACTIVE_LOCK); #undef ADD_BFCACHESTATUS_TO_STRING diff --git a/dom/tests/browser/browser_hasbeforeunload.js b/dom/tests/browser/browser_hasbeforeunload.js index b9ef812d6ddf..97cd96aeaf0d 100644 --- a/dom/tests/browser/browser_hasbeforeunload.js +++ b/dom/tests/browser/browser_hasbeforeunload.js @@ -408,6 +408,12 @@ function assertHasBeforeUnload(browser, expected) { * inner window is removed from the DOM. */ add_task(async function test_inner_window_scenarios() { + // Turn this off because the test expects the page to be not bfcached. + await SpecialPowers.pushPrefEnv({ + set: [ + ["docshell.shistory.bfcache.ship_allow_beforeunload_listeners", false], + ], + }); await BrowserTestUtils.withNewTab( { gBrowser, @@ -592,6 +598,12 @@ add_task(async function test_inner_window_scenarios() { * to the iframe DOM nodes instead of the inner windows. */ add_task(async function test_outer_window_scenarios() { + // Turn this off because the test expects the page to be not bfcached. + await SpecialPowers.pushPrefEnv({ + set: [ + ["docshell.shistory.bfcache.ship_allow_beforeunload_listeners", false], + ], + }); await BrowserTestUtils.withNewTab( { gBrowser, @@ -787,6 +799,12 @@ add_task(async function test_outer_window_scenarios() { * are added on both inner and outer windows. */ add_task(async function test_mixed_inner_and_outer_window_scenarios() { + // Turn this off because the test expects the page to be not bfcached. + await SpecialPowers.pushPrefEnv({ + set: [ + ["docshell.shistory.bfcache.ship_allow_beforeunload_listeners", false], + ], + }); await BrowserTestUtils.withNewTab( { gBrowser, diff --git a/modules/libpref/init/StaticPrefList.yaml b/modules/libpref/init/StaticPrefList.yaml index 63506081fff3..a19cae0a86c2 100644 --- a/modules/libpref/init/StaticPrefList.yaml +++ b/modules/libpref/init/StaticPrefList.yaml @@ -1816,6 +1816,13 @@ value: @IS_ANDROID@ mirror: always +# If true, page with beforeunload event listeners can be bfcached. +# This only works when sessionHistoryInParent is enabled. +- name: docshell.shistory.bfcache.ship_allow_beforeunload_listeners + type: bool + value: @IS_NIGHTLY_BUILD@ + mirror: always + #--------------------------------------------------------------------------- # Prefs starting with "dom." #--------------------------------------------------------------------------- diff --git a/testing/web-platform/meta/html/browsers/browsing-the-web/back-forward-cache/events.html.ini b/testing/web-platform/meta/html/browsers/browsing-the-web/back-forward-cache/events.html.ini index f98bff68a2c6..3b9979f1026c 100644 --- a/testing/web-platform/meta/html/browsers/browsing-the-web/back-forward-cache/events.html.ini +++ b/testing/web-platform/meta/html/browsers/browsing-the-web/back-forward-cache/events.html.ini @@ -1,9 +1,8 @@ [events.html] + prefs: [docshell.shistory.bfcache.ship_allow_beforeunload_listeners:true] [beforeunload] expected: - if os == "android": PASS - PRECONDITION_FAILED - + if not sessionHistoryInParent and (os != "android") : PRECONDITION_FAILED [unload] expected: if os == "android": PASS diff --git a/toolkit/components/remotebrowserutils/tests/browser/dummy_page.html b/toolkit/components/remotebrowserutils/tests/browser/dummy_page.html index 907113cd1d66..8205e90d5dea 100644 --- a/toolkit/components/remotebrowserutils/tests/browser/dummy_page.html +++ b/toolkit/components/remotebrowserutils/tests/browser/dummy_page.html @@ -7,9 +7,7 @@