Bug 1789296 - Intermittent /html/browsers/browsing-the-web/navigating-across-documents/initial-empty-document/window-open-aboutblank.html. r=smaug

Differential Revision: https://phabricator.services.mozilla.com/D172249

docshell/base/BrowsingContext.cpp

@@ -3635,8 +3635,9 @@ bool BrowsingContext::ShouldAddEntryForRefresh(nsIURI* aPreviousURI,
 
 void BrowsingContext::SessionHistoryCommit(
     const LoadingSessionHistoryInfo& aInfo, uint32_t aLoadType,
-    nsIURI* aPreviousURI, bool aHadActiveEntry, bool aPersist,
-    bool aCloneEntryChildren, bool aChannelExpired, uint32_t aCacheKey) {
+    nsIURI* aPreviousURI, SessionHistoryInfo* aPreviousActiveEntry,
+    bool aPersist, bool aCloneEntryChildren, bool aChannelExpired,
+    uint32_t aCacheKey) {
   nsID changeID = {};
   if (XRE_IsContentProcess()) {
     RefPtr<ChildSHistory> rootSH = Top()->GetChildSessionHistory();
@@ -3650,9 +3651,17 @@ void BrowsingContext::SessionHistoryCommit(
         // refresh for which ShouldAddEntryForRefresh returns false.
         // It is possible that this leads to wrong length temporarily, but
         // so would not having the check for replace.
+        // Note that nsSHistory::AddEntry does a replace load if the current
+        // entry is not marked as a persisted entry. The child process does
+        // not have access to the current entry, so we use the previous active
+        // entry as the best approximation. When that's not the current entry
+        // then the length might be wrong briefly, until the parent process
+        // commits the actual length.
         if (!LOAD_TYPE_HAS_FLAGS(
                 aLoadType, nsIWebNavigation::LOAD_FLAGS_REPLACE_HISTORY) &&
-            (IsTop() || aHadActiveEntry) &&
+            (IsTop()
+                 ? (!aPreviousActiveEntry || aPreviousActiveEntry->GetPersist())
+                 : !!aPreviousActiveEntry) &&
             ShouldUpdateSessionHistory(aLoadType) &&
             (!LOAD_TYPE_HAS_FLAGS(aLoadType,
                                   nsIWebNavigation::LOAD_FLAGS_IS_REFRESH) ||

docshell/base/BrowsingContext.h

@@ -852,9 +852,9 @@ class BrowsingContext : public nsILoadContext, public nsWrapperCache {
   // the current URI.
   void SessionHistoryCommit(const LoadingSessionHistoryInfo& aInfo,
                             uint32_t aLoadType, nsIURI* aCurrentURI,
-                            bool aHadActiveEntry, bool aPersist,
-                            bool aCloneEntryChildren, bool aChannelExpired,
-                            uint32_t aCacheKey);
+                            SessionHistoryInfo* aPreviousActiveEntry,
+                            bool aPersist, bool aCloneEntryChildren,
+                            bool aChannelExpired, uint32_t aCacheKey);
 
   // Set a new active entry on this browsing context. This is used for
   // implementing history.pushState/replaceState and same document navigations.

docshell/base/nsDocShell.cpp

@@ -9054,12 +9054,13 @@ nsresult nsDocShell::HandleSameDocumentNavigation(
           ("Moving the loading entry to the active entry on nsDocShell %p to "
            "%s",
            this, mLoadingEntry->mInfo.GetURI()->GetSpecOrDefault().get()));
-      bool hadActiveEntry = !!mActiveEntry;
 
       nsCOMPtr<nsILayoutHistoryState> currentLayoutHistoryState;
       if (mActiveEntry) {
         currentLayoutHistoryState = mActiveEntry->GetLayoutHistoryState();
       }
+
+      UniquePtr<SessionHistoryInfo> previousActiveEntry(mActiveEntry.release());
       mActiveEntry = MakeUnique<SessionHistoryInfo>(mLoadingEntry->mInfo);
       if (currentLayoutHistoryState) {
         // Restore the existing nsILayoutHistoryState object, since it is
@@ -9076,7 +9077,8 @@ nsresult nsDocShell::HandleSameDocumentNavigation(
       // does require a non-null uri if this is for a refresh load of the same
       // URI, but in that case mCurrentURI won't be null here.
       mBrowsingContext->SessionHistoryCommit(
-          *mLoadingEntry, mLoadType, mCurrentURI, hadActiveEntry, true, true,
+          *mLoadingEntry, mLoadType, mCurrentURI, previousActiveEntry.get(),
+          true, true,
           /* No expiration update on the same document loads*/
           false, cacheKey);
       // FIXME Need to set postdata.
@@ -13725,8 +13727,7 @@ void nsDocShell::MoveLoadingToActiveEntry(bool aPersist, bool aExpired,
   MOZ_LOG(gSHLog, LogLevel::Debug,
           ("nsDocShell %p MoveLoadingToActiveEntry", this));
 
-  bool hadActiveEntry = !!mActiveEntry;
-  mActiveEntry = nullptr;
+  UniquePtr<SessionHistoryInfo> previousActiveEntry(mActiveEntry.release());
   mozilla::UniquePtr<mozilla::dom::LoadingSessionHistoryInfo> loadingEntry;
   mActiveEntryIsLoadingFromSessionHistory =
       mLoadingEntry && mLoadingEntry->mLoadIsFromSessionHistory;
@@ -13762,8 +13763,8 @@ void nsDocShell::MoveLoadingToActiveEntry(bool aPersist, bool aExpired,
       // does require a non-null uri if this is for a refresh load of the same
       // URI, but in that case mCurrentURI won't be null here.
       mBrowsingContext->SessionHistoryCommit(
-          *loadingEntry, loadType, aPreviousURI, hadActiveEntry, aPersist,
-          false, aExpired, aCacheKey);
+          *loadingEntry, loadType, aPreviousURI, previousActiveEntry.get(),
+          aPersist, false, aExpired, aCacheKey);
     }
   }
 }

docshell/shistory/SessionHistoryEntry.h

@@ -158,6 +158,8 @@ class SessionHistoryInfo {
 
   void SetSaveLayoutStateFlag(bool aSaveLayoutStateFlag);
 
+  bool GetPersist() const { return mPersist; }
+
  private:
   friend class SessionHistoryEntry;
   friend struct mozilla::ipc::IPDLParamTraits<SessionHistoryInfo>;

testing/web-platform/meta/html/browsers/browsing-the-web/navigating-across-documents/initial-empty-document/window-open-aboutblank.html.ini

@@ -1,13 +0,0 @@
-[window-open-aboutblank.html]
-  [location.href]
-    expected:
-      if fission and (os == "linux") and not debug and (processor == "x86_64"): [PASS, FAIL]
-      if fission and (os == "linux") and not debug and (processor == "x86"): [PASS, FAIL]
-      if fission and (os == "linux") and debug: [PASS, FAIL]
-      if fission and (os == "mac"): [PASS, FAIL]
-
-  [location.assign]
-    expected:
-      if fission and (os == "mac") and not debug: [PASS, FAIL]
-      if fission and (os == "linux") and (processor == "x86_64"): [PASS, FAIL]
-      if fission and (os == "linux") and (processor == "x86"): [PASS, FAIL]