From dc39236b977faa6ca317562a23d754b7b1f56bab Mon Sep 17 00:00:00 2001 From: Olli Pettay Date: Sun, 21 Mar 2021 21:46:26 +0000 Subject: [PATCH] Bug 1699484 - Support Timer based bfcache eviction, r=peterv The patch makes HistoryTracker rely on SHEntrySharedParentState instead of nsSHEntryShared. nsSHEntryShared already extends SHEntrySharedParentState. The test was modified a tiny bit to make it easier to see the results. The test does pass with SHIP+BFCache. Depends on D108851 Differential Revision: https://phabricator.services.mozilla.com/D108984 --- docshell/shistory/SessionHistoryEntry.cpp | 7 +++--- docshell/shistory/nsISHEntry.idl | 4 +++- docshell/shistory/nsISHistory.idl | 13 ++++++++--- docshell/shistory/nsSHEntry.cpp | 5 +++-- docshell/shistory/nsSHEntryShared.cpp | 16 ++++++++++++- docshell/shistory/nsSHEntryShared.h | 7 +++--- docshell/shistory/nsSHistory.cpp | 25 ++++++++++----------- docshell/shistory/nsSHistory.h | 10 +++++---- docshell/test/browser/browser_bug1347823.js | 10 ++++----- 9 files changed, 61 insertions(+), 36 deletions(-) diff --git a/docshell/shistory/SessionHistoryEntry.cpp b/docshell/shistory/SessionHistoryEntry.cpp index 37fe43a43f22..4450a68c0220 100644 --- a/docshell/shistory/SessionHistoryEntry.cpp +++ b/docshell/shistory/SessionHistoryEntry.cpp @@ -1041,9 +1041,8 @@ SessionHistoryEntry::HasDynamicallyAddedChild(bool* aHasDynamicallyAddedChild) { } NS_IMETHODIMP_(bool) -SessionHistoryEntry::HasBFCacheEntry(nsIBFCacheEntry* aEntry) { - MOZ_CRASH("This lives in the child process"); - return false; +SessionHistoryEntry::HasBFCacheEntry(SHEntrySharedParentState* aEntry) { + return SharedInfo() == aEntry; } NS_IMETHODIMP @@ -1356,7 +1355,7 @@ void SessionHistoryEntry::SetFrameLoader(nsFrameLoader* aFrameLoader) { MOZ_ASSERT_IF(aFrameLoader, !SharedInfo()->mFrameLoader); // If the pref is disabled, we still allow evicting the existing entries. MOZ_RELEASE_ASSERT(!aFrameLoader || mozilla::BFCacheInParent()); - SharedInfo()->mFrameLoader = aFrameLoader; + SharedInfo()->SetFrameLoader(aFrameLoader); if (aFrameLoader) { if (BrowserParent* bp = aFrameLoader->GetBrowserParent()) { bp->Deactivated(); diff --git a/docshell/shistory/nsISHEntry.idl b/docshell/shistory/nsISHEntry.idl index d77cc3ce00a2..4b28726788a3 100644 --- a/docshell/shistory/nsISHEntry.idl +++ b/docshell/shistory/nsISHEntry.idl @@ -42,6 +42,7 @@ struct EntriesAndBrowsingContextData; [ref] native nsIntRect(nsIntRect); [ptr] native nsDocShellEditorDataPtr(nsDocShellEditorData); [ptr] native nsDocShellLoadStatePtr(nsDocShellLoadState); +[ptr] native SHEntrySharedParentStatePtr(mozilla::dom::SHEntrySharedParentState); webidl BrowsingContext; [builtinclass, scriptable, uuid(0dad26b8-a259-42c7-93f1-2fa7fc076e45)] @@ -358,7 +359,8 @@ interface nsISHEntry : nsISupports * the BFCache entry will evict the SHEntry, since the two entries * correspond to the same document. */ - [noscript, notxpcom] boolean hasBFCacheEntry(in nsIBFCacheEntry aEntry); + [noscript, notxpcom] + boolean hasBFCacheEntry(in SHEntrySharedParentStatePtr aEntry); /** * Adopt aEntry's BFCacheEntry, so now both this and aEntry point to diff --git a/docshell/shistory/nsISHistory.idl b/docshell/shistory/nsISHistory.idl index 1f5b9c5477e9..c08f28b5a08a 100644 --- a/docshell/shistory/nsISHistory.idl +++ b/docshell/shistory/nsISHistory.idl @@ -15,10 +15,16 @@ webidl BrowsingContext; #include "nsTArrayForwardDeclare.h" #include "mozilla/Maybe.h" struct EntriesAndBrowsingContextData; +namespace mozilla { +namespace dom { +class SHEntrySharedParentState; +} // namespace dom +} // namespace mozilla %} [ref] native nsDocshellIDArray(nsTArray); native MaybeInt32(mozilla::Maybe); +[ptr] native SHEntrySharedParentStatePtr(mozilla::dom::SHEntrySharedParentState); /** * An interface to the primary properties of the Session History * component. In an embedded browser environment, the nsIWebBrowser @@ -197,7 +203,8 @@ interface nsISHistory: nsISupports * Evict the content viewer associated with a bfcache entry that has timed * out. */ - void evictExpiredContentViewerForEntry(in nsIBFCacheEntry aEntry); + [noscript, notxpcom] + void evictExpiredContentViewerForEntry(in SHEntrySharedParentStatePtr aEntry); /** * Evict all the content viewers in this session history @@ -209,13 +216,13 @@ interface nsISHistory: nsISupports * expiration. */ [noscript, notxpcom] - void addToExpirationTracker(in nsIBFCacheEntry aEntry); + void addToExpirationTracker(in SHEntrySharedParentStatePtr aEntry); /** * Remove a BFCache entry from expiration tracker. */ [noscript, notxpcom] - void removeFromExpirationTracker(in nsIBFCacheEntry aEntry); + void removeFromExpirationTracker(in SHEntrySharedParentStatePtr aEntry); /** * Remove dynamic entries found at given index. diff --git a/docshell/shistory/nsSHEntry.cpp b/docshell/shistory/nsSHEntry.cpp index 03de6b957134..45f33549fa58 100644 --- a/docshell/shistory/nsSHEntry.cpp +++ b/docshell/shistory/nsSHEntry.cpp @@ -1059,8 +1059,9 @@ bool nsSHEntry::HasDetachedEditor() { return GetState()->mEditorData != nullptr; } -bool nsSHEntry::HasBFCacheEntry(nsIBFCacheEntry* aEntry) { - return static_cast(GetState()) == aEntry; +bool nsSHEntry::HasBFCacheEntry( + mozilla::dom::SHEntrySharedParentState* aEntry) { + return GetState() == aEntry; } NS_IMETHODIMP diff --git a/docshell/shistory/nsSHEntryShared.cpp b/docshell/shistory/nsSHEntryShared.cpp index 17d8bfc6e329..06c7ab337333 100644 --- a/docshell/shistory/nsSHEntryShared.cpp +++ b/docshell/shistory/nsSHEntryShared.cpp @@ -70,7 +70,8 @@ SHEntrySharedParentState::SHEntrySharedParentState( SHEntrySharedParentState::~SHEntrySharedParentState() { MOZ_ASSERT(mId != 0); - RefPtr loader = mFrameLoader.forget(); + RefPtr loader = mFrameLoader; + SetFrameLoader(nullptr); if (loader) { loader->Destroy(); } @@ -117,7 +118,20 @@ void SHEntrySharedChildState::CopyFrom(SHEntrySharedChildState* aEntry) { } void SHEntrySharedParentState::SetFrameLoader(nsFrameLoader* aFrameLoader) { + // If expiration tracker is removing this object, IsTracked() returns false. + if (GetExpirationState()->IsTracked() && mFrameLoader) { + if (nsCOMPtr shistory = do_QueryReferent(mSHistory)) { + shistory->RemoveFromExpirationTracker(this); + } + } + mFrameLoader = aFrameLoader; + + if (mFrameLoader) { + if (nsCOMPtr shistory = do_QueryReferent(mSHistory)) { + shistory->AddToExpirationTracker(this); + } + } } nsFrameLoader* SHEntrySharedParentState::GetFrameLoader() { diff --git a/docshell/shistory/nsSHEntryShared.h b/docshell/shistory/nsSHEntryShared.h index 663c47c07961..bff60c310369 100644 --- a/docshell/shistory/nsSHEntryShared.h +++ b/docshell/shistory/nsSHEntryShared.h @@ -103,6 +103,8 @@ class SHEntrySharedParentState : public SHEntrySharedState { void NotifyListenersContentViewerEvicted(); + nsExpirationState* GetExpirationState() { return &mExpirationState; } + SHEntrySharedParentState(); SHEntrySharedParentState(nsIPrincipal* aTriggeringPrincipal, nsIPrincipal* aPrincipalToInherit, @@ -137,6 +139,8 @@ class SHEntrySharedParentState : public SHEntrySharedState { RefPtr mFrameLoader; + nsExpirationState mExpirationState; + bool mSticky = true; bool mDynamicallyCreated = false; @@ -166,7 +170,6 @@ class SHEntrySharedChildState { nsCOMPtr mWindowState; // FIXME Move to parent? nsCOMPtr mRefreshURIList; - nsExpirationState mExpirationState; UniquePtr mEditorData; }; @@ -202,8 +205,6 @@ class nsSHEntryShared final : public nsIBFCacheEntry, NS_DECL_NSIMUTATIONOBSERVER_CONTENTINSERTED NS_DECL_NSIMUTATIONOBSERVER_CONTENTREMOVED - nsExpirationState* GetExpirationState() { return &mExpirationState; } - private: ~nsSHEntryShared(); diff --git a/docshell/shistory/nsSHistory.cpp b/docshell/shistory/nsSHistory.cpp index 6513ab098c9f..8742f09b694e 100644 --- a/docshell/shistory/nsSHistory.cpp +++ b/docshell/shistory/nsSHistory.cpp @@ -1595,7 +1595,7 @@ void nsSHistory::GloballyEvictContentViewers() { } } -nsresult nsSHistory::FindEntryForBFCache(nsIBFCacheEntry* aBFEntry, +nsresult nsSHistory::FindEntryForBFCache(SHEntrySharedParentState* aEntry, nsISHEntry** aResult, int32_t* aResultIndex) { *aResult = nullptr; @@ -1608,7 +1608,7 @@ nsresult nsSHistory::FindEntryForBFCache(nsIBFCacheEntry* aBFEntry, nsCOMPtr shEntry = mEntries[i]; // Does shEntry have the same BFCacheEntry as the argument to this method? - if (shEntry->HasBFCacheEntry(aBFEntry)) { + if (shEntry->HasBFCacheEntry(aEntry)) { shEntry.forget(aResult); *aResultIndex = i; return NS_OK; @@ -1617,27 +1617,25 @@ nsresult nsSHistory::FindEntryForBFCache(nsIBFCacheEntry* aBFEntry, return NS_ERROR_FAILURE; } -nsresult nsSHistory::EvictExpiredContentViewerForEntry( - nsIBFCacheEntry* aBFEntry) { +NS_IMETHODIMP_(void) +nsSHistory::EvictExpiredContentViewerForEntry( + SHEntrySharedParentState* aEntry) { int32_t index; nsCOMPtr shEntry; - FindEntryForBFCache(aBFEntry, getter_AddRefs(shEntry), &index); + FindEntryForBFCache(aEntry, getter_AddRefs(shEntry), &index); if (index == mIndex) { NS_WARNING("How did the current SHEntry expire?"); - return NS_OK; } if (shEntry) { EvictContentViewerForEntry(shEntry); } - - return NS_OK; } NS_IMETHODIMP_(void) -nsSHistory::AddToExpirationTracker(nsIBFCacheEntry* aBFEntry) { - RefPtr entry = static_cast(aBFEntry); +nsSHistory::AddToExpirationTracker(SHEntrySharedParentState* aEntry) { + RefPtr entry = aEntry; if (!mHistoryTracker || !entry) { return; } @@ -1647,8 +1645,8 @@ nsSHistory::AddToExpirationTracker(nsIBFCacheEntry* aBFEntry) { } NS_IMETHODIMP_(void) -nsSHistory::RemoveFromExpirationTracker(nsIBFCacheEntry* aBFEntry) { - RefPtr entry = static_cast(aBFEntry); +nsSHistory::RemoveFromExpirationTracker(SHEntrySharedParentState* aEntry) { + RefPtr entry = aEntry; MOZ_ASSERT(mHistoryTracker && !mHistoryTracker->IsEmpty()); if (!mHistoryTracker || !entry) { return; @@ -1874,7 +1872,8 @@ void nsSHistory::RemoveDynEntries(int32_t aIndex, nsISHEntry* aEntry) { void nsSHistory::RemoveDynEntriesForBFCacheEntry(nsIBFCacheEntry* aBFEntry) { int32_t index; nsCOMPtr shEntry; - FindEntryForBFCache(aBFEntry, getter_AddRefs(shEntry), &index); + FindEntryForBFCache(static_cast(aBFEntry), + getter_AddRefs(shEntry), &index); if (shEntry) { RemoveDynEntries(index, shEntry); } diff --git a/docshell/shistory/nsSHistory.h b/docshell/shistory/nsSHistory.h index 0f6d2afe38e8..e572fa2318f2 100644 --- a/docshell/shistory/nsSHistory.h +++ b/docshell/shistory/nsSHistory.h @@ -36,7 +36,8 @@ class nsSHistory : public mozilla::LinkedListElement, public nsSupportsWeakReference { public: // The timer based history tracker is used to evict bfcache on expiration. - class HistoryTracker final : public nsExpirationTracker { + class HistoryTracker final + : public nsExpirationTracker { public: explicit HistoryTracker(nsSHistory* aSHistory, uint32_t aTimeout, nsIEventTarget* aEventTarget) @@ -47,7 +48,8 @@ class nsSHistory : public mozilla::LinkedListElement, } protected: - virtual void NotifyExpired(nsSHEntryShared* aObj) override { + virtual void NotifyExpired( + mozilla::dom::SHEntrySharedParentState* aObj) override { RemoveObject(aObj); mSHistory->EvictExpiredContentViewerForEntry(aObj); } @@ -226,8 +228,8 @@ class nsSHistory : public mozilla::LinkedListElement, // Find the history entry for a given bfcache entry. It only looks up between // the range where alive viewers may exist (i.e nsSHistory::VIEWER_WINDOW). - nsresult FindEntryForBFCache(nsIBFCacheEntry* aBFEntry, nsISHEntry** aResult, - int32_t* aResultIndex); + nsresult FindEntryForBFCache(mozilla::dom::SHEntrySharedParentState* aEntry, + nsISHEntry** aResult, int32_t* aResultIndex); // Evict content viewers in this window which don't lie in the "safe" range // around aIndex. diff --git a/docshell/test/browser/browser_bug1347823.js b/docshell/test/browser/browser_bug1347823.js index 551ddca8ab15..58eac7d0ac1f 100644 --- a/docshell/test/browser/browser_bug1347823.js +++ b/docshell/test/browser/browser_bug1347823.js @@ -11,7 +11,7 @@ add_task(async function testValidCache() { }); await BrowserTestUtils.withNewTab( - { gBrowser, url: "data:text/html;charset=utf-8,page1" }, + { gBrowser, url: "data:text/html;charset=utf-8,pageA1" }, async function(browser) { // Make a simple modification for bfcache testing. await SpecialPowers.spawn(browser, [], () => { @@ -19,7 +19,7 @@ add_task(async function testValidCache() { }); // Load a random page. - BrowserTestUtils.loadURI(browser, "data:text/html;charset=utf-8,page2"); + BrowserTestUtils.loadURI(browser, "data:text/html;charset=utf-8,pageA2"); await BrowserTestUtils.browserLoaded(browser); // Go back and verify text content. @@ -44,7 +44,7 @@ add_task(async function testExpiredCache() { }); await BrowserTestUtils.withNewTab( - { gBrowser, url: "data:text/html;charset=utf-8,page1" }, + { gBrowser, url: "data:text/html;charset=utf-8,pageB1" }, async function(browser) { // Make a simple modification for bfcache testing. await SpecialPowers.spawn(browser, [], () => { @@ -52,7 +52,7 @@ add_task(async function testExpiredCache() { }); // Load a random page. - BrowserTestUtils.loadURI(browser, "data:text/html;charset=utf-8,page2"); + BrowserTestUtils.loadURI(browser, "data:text/html;charset=utf-8,pageB2"); await BrowserTestUtils.browserLoaded(browser); // Wait for 3 times of expiration timeout, hopefully it's evicted... @@ -70,7 +70,7 @@ add_task(async function testExpiredCache() { browser.goBack(); await awaitPageShow; await SpecialPowers.spawn(browser, [], () => { - is(content.document.body.textContent, "page1"); + is(content.document.body.textContent, "pageB1"); }); } );