Bug 1664656 - Decide whether to add a new entry based on the load type. r=smaug

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

docshell/base/BrowsingContext.cpp

@@ -2745,6 +2745,14 @@ void BrowsingContext::SetChildSHistory(ChildSHistory* aChildSHistory) {
   mFields.SetWithoutSyncing<IDX_HasSessionHistory>(true);
 }
 
+bool BrowsingContext::ShouldUpdateSessionHistory(uint32_t aLoadType) {
+  // We don't update session history on reload unless we're loading
+  // an iframe in shift-reload case.
+  return nsDocShell::ShouldUpdateGlobalHistory(aLoadType) &&
+         (!(aLoadType & nsIDocShell::LOAD_CMD_RELOAD) ||
+          (IsForceReloadType(aLoadType) && IsFrame()));
+}
+
 }  // namespace dom
 
 namespace ipc {

docshell/base/BrowsingContext.h

@@ -688,6 +688,8 @@ class BrowsingContext : public nsILoadContext, public nsWrapperCache {
 
   void HistoryGo(int32_t aIndex, std::function<void(int32_t&&)>&& aResolver);
 
+  bool ShouldUpdateSessionHistory(uint32_t aLoadType);
+
  protected:
   virtual ~BrowsingContext();
   BrowsingContext(WindowContext* aParentWindow, BrowsingContextGroup* aGroup,

docshell/base/CanonicalBrowsingContext.cpp

@@ -376,7 +376,8 @@ CanonicalBrowsingContext::CreateLoadingSessionHistoryEntryForLoad(
 }
 
 void CanonicalBrowsingContext::SessionHistoryCommit(uint64_t aLoadId,
-                                                    const nsID& aChangeID) {
+                                                    const nsID& aChangeID,
+                                                    uint32_t aLoadType) {
   for (size_t i = 0; i < mLoadingEntries.Length(); ++i) {
     if (mLoadingEntries[i].mLoadId == aLoadId) {
       nsISHistory* shistory = GetSessionHistory();
@@ -405,12 +406,13 @@ void CanonicalBrowsingContext::SessionHistoryCommit(uint64_t aLoadId,
             [](nsISHEntry* aEntry) { aEntry->SetName(EmptyString()); });
       }
 
+      bool addEntry = ShouldUpdateSessionHistory(aLoadType);
       if (IsTop()) {
         mActiveEntry = newActiveEntry;
         if (loadFromSessionHistory) {
           // XXX Synchronize browsing context tree and session history tree?
           shistory->UpdateIndex();
-        } else {
+        } else if (addEntry) {
           shistory->AddEntry(mActiveEntry,
                              /* FIXME aPersist = */ true);
         }
@@ -431,24 +433,28 @@ void CanonicalBrowsingContext::SessionHistoryCommit(uint64_t aLoadId,
           // FIXME UpdateIndex() here may update index too early (but even the
           //       old implementation seems to have similar issues).
           shistory->UpdateIndex();
-        } else if (oldActiveEntry) {
+        } else if (addEntry) {
-          // AddChildSHEntryHelper does update the index of the session history!
+          if (oldActiveEntry) {
-          // FIXME Need to figure out the right value for aCloneChildren.
+            // AddChildSHEntryHelper does update the index of the session
-          shistory->AddChildSHEntryHelper(oldActiveEntry, newActiveEntry, Top(),
+            // history!
-                                          true);
+            // FIXME Need to figure out the right value for aCloneChildren.
-          mActiveEntry = newActiveEntry;
+            shistory->AddChildSHEntryHelper(oldActiveEntry, newActiveEntry,
-        } else {
+                                            Top(), true);
-          SessionHistoryEntry* parentEntry =
-              static_cast<CanonicalBrowsingContext*>(GetParent())->mActiveEntry;
-          // XXX What should happen if parent doesn't have mActiveEntry?
-          //     Or can that even happen ever?
-          if (parentEntry) {
             mActiveEntry = newActiveEntry;
-            // FIXME The docshell code sometime uses -1 for aChildOffset!
+          } else {
-            // FIXME Using IsInProcess for aUseRemoteSubframes isn't quite
+            SessionHistoryEntry* parentEntry =
-            //       right, but aUseRemoteSubframes should be going away.
+                static_cast<CanonicalBrowsingContext*>(GetParent())
-            parentEntry->AddChild(mActiveEntry, Children().Length() - 1,
+                    ->mActiveEntry;
-                                  IsInProcess());
+            // XXX What should happen if parent doesn't have mActiveEntry?
+            //     Or can that even happen ever?
+            if (parentEntry) {
+              mActiveEntry = newActiveEntry;
+              // FIXME The docshell code sometime uses -1 for aChildOffset!
+              // FIXME Using IsInProcess for aUseRemoteSubframes isn't quite
+              //       right, but aUseRemoteSubframes should be going away.
+              parentEntry->AddChild(mActiveEntry, Children().Length() - 1,
+                                    IsInProcess());
+            }
           }
         }
       }

docshell/base/CanonicalBrowsingContext.h

@@ -100,7 +100,8 @@ class CanonicalBrowsingContext final : public BrowsingContext {
 
   UniquePtr<LoadingSessionHistoryInfo> CreateLoadingSessionHistoryEntryForLoad(
       nsDocShellLoadState* aLoadState, nsIChannel* aChannel);
-  void SessionHistoryCommit(uint64_t aLoadId, const nsID& aChangeID);
+  void SessionHistoryCommit(uint64_t aLoadId, const nsID& aChangeID,
+                            uint32_t aLoadType);
 
   // Calls the session history listeners' OnHistoryReload, storing the result in
   // aCanReload. If aCanReload is set to true and we have an active or a loading

docshell/base/nsDocShell.cpp

@@ -5713,7 +5713,7 @@ nsresult nsDocShell::Embed(nsIContentViewer* aContentViewer,
       nsID changeID = {};
       if (XRE_IsParentProcess()) {
         mBrowsingContext->Canonical()->SessionHistoryCommit(
-            loadingEntry->mLoadId, changeID);
+            loadingEntry->mLoadId, changeID, mLoadType);
       } else {
         RefPtr<ChildSHistory> rootSH = GetRootSessionHistory();
         if (rootSH) {
@@ -5729,7 +5729,7 @@ nsresult nsDocShell::Embed(nsIContentViewer* aContentViewer,
         }
         ContentChild* cc = ContentChild::GetSingleton();
         mozilla::Unused << cc->SendHistoryCommit(
-            mBrowsingContext, loadingEntry->mLoadId, changeID);
+            mBrowsingContext, loadingEntry->mLoadId, changeID, mLoadType);
       }
     }
   }
@@ -8895,7 +8895,7 @@ nsresult nsDocShell::HandleSameDocumentNavigation(
     nsID changeID = {};
     if (XRE_IsParentProcess()) {
       mBrowsingContext->Canonical()->SessionHistoryCommit(
-          mLoadingEntry->mLoadId, changeID);
+          mLoadingEntry->mLoadId, changeID, mLoadType);
     } else {
       RefPtr<ChildSHistory> rootSH = GetRootSessionHistory();
       if (rootSH) {
@@ -8907,7 +8907,7 @@ nsresult nsDocShell::HandleSameDocumentNavigation(
       }
       ContentChild* cc = ContentChild::GetSingleton();
       mozilla::Unused << cc->SendHistoryCommit(
-          mBrowsingContext, mLoadingEntry->mLoadId, changeID);
+          mBrowsingContext, mLoadingEntry->mLoadId, changeID, mLoadType);
     }
   }
 
@@ -10630,9 +10630,7 @@ bool nsDocShell::OnNewURI(nsIURI* aURI, nsIChannel* aChannel,
 
   // We don't update session history on reload unless we're loading
   // an iframe in shift-reload case.
-  bool updateSHistory =
+  bool updateSHistory = mBrowsingContext->ShouldUpdateSessionHistory(aLoadType);
-      updateGHistory && (!(aLoadType & LOAD_CMD_RELOAD) ||
-                         (IsForceReloadType(aLoadType) && IsFrame()));
 
   // Create SH Entry (mLSHE) only if there is a SessionHistory object in the
   // root browsing context.

dom/ipc/ContentParent.cpp

@@ -6836,9 +6836,10 @@ mozilla::ipc::IPCResult ContentParent::RecvNotifyOnHistoryReload(
 
 mozilla::ipc::IPCResult ContentParent::RecvHistoryCommit(
     const MaybeDiscarded<BrowsingContext>& aContext, const uint64_t& aLoadID,
-    const nsID& aChangeID) {
+    const nsID& aChangeID, const uint32_t& aLoadType) {
   if (!aContext.IsDiscarded()) {
-    aContext.get_canonical()->SessionHistoryCommit(aLoadID, aChangeID);
+    aContext.get_canonical()->SessionHistoryCommit(aLoadID, aChangeID,
+                                                   aLoadType);
   }
 
   return IPC_OK();

dom/ipc/ContentParent.h

@@ -1314,7 +1314,7 @@ class ContentParent final
 
   mozilla::ipc::IPCResult RecvHistoryCommit(
       const MaybeDiscarded<BrowsingContext>& aContext, const uint64_t& aLoadID,
-      const nsID& aChangeID);
+      const nsID& aChangeID, const uint32_t& aLoadType);
 
   mozilla::ipc::IPCResult RecvHistoryGo(
       const MaybeDiscarded<BrowsingContext>& aContext, int32_t aOffset,

dom/ipc/PContent.ipdl

@@ -1671,7 +1671,7 @@ parent:
                      bool? reloadActiveEntry);
 
     async HistoryCommit(MaybeDiscardedBrowsingContext aContext,
-                        uint64_t aLoadID, nsID aChangeID);
+                        uint64_t aLoadID, nsID aChangeID, uint32_t aLoadType);
 
     async HistoryGo(MaybeDiscardedBrowsingContext aContext,
                     int32_t aOffset) returns(int32_t requestedIndex);