Bug 1670848: Handling aborting history navigations when others run r=smaug

Handles edge cases around history.go() with one event loop spin

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

docshell/base/BrowsingContext.cpp

@@ -2793,15 +2793,20 @@ void BrowsingContext::RemoveFromSessionHistory() {
   }
 }
 
-void BrowsingContext::HistoryGo(int32_t aOffset,
+void BrowsingContext::HistoryGo(int32_t aOffset, uint64_t aHistoryEpoch,
                                 std::function<void(int32_t&&)>&& aResolver) {
   if (XRE_IsContentProcess()) {
     ContentChild::GetSingleton()->SendHistoryGo(
-        this, aOffset, std::move(aResolver),
+        this, aOffset, aHistoryEpoch, std::move(aResolver),
         [](mozilla::ipc::
                ResponseRejectReason) { /* FIXME Is ignoring this fine? */ });
   } else {
-    Canonical()->HistoryGo(aOffset, std::move(aResolver));
+    Canonical()->HistoryGo(
+        aOffset, aHistoryEpoch,
+        Canonical()->GetContentParent()
+            ? Some(Canonical()->GetContentParent()->ChildID())
+            : Nothing(),
+        std::move(aResolver));
   }
 }
 

docshell/base/BrowsingContext.h

@@ -717,7 +717,8 @@ class BrowsingContext : public nsILoadContext, public nsWrapperCache {
   Tuple<nsCOMPtr<nsIPrincipal>, nsCOMPtr<nsIPrincipal>>
   GetTriggeringAndInheritPrincipalsForCurrentLoad();
 
-  void HistoryGo(int32_t aOffset, std::function<void(int32_t&&)>&& aResolver);
+  void HistoryGo(int32_t aOffset, uint64_t aHistoryEpoch,
+                 std::function<void(int32_t&&)>&& aResolver);
 
   bool ShouldUpdateSessionHistory(uint32_t aLoadType);
 

docshell/base/CanonicalBrowsingContext.cpp

@@ -696,7 +696,8 @@ void CanonicalBrowsingContext::RemoveFromSessionHistory() {
 }
 
 void CanonicalBrowsingContext::HistoryGo(
-    int32_t aOffset, std::function<void(int32_t&&)>&& aResolver) {
+    int32_t aOffset, uint64_t aHistoryEpoch, Maybe<ContentParentId> aContentId,
+    std::function<void(int32_t&&)>&& aResolver) {
   nsSHistory* shistory = static_cast<nsSHistory*>(GetSessionHistory());
   if (!shistory) {
     return;
@@ -705,20 +706,44 @@ void CanonicalBrowsingContext::HistoryGo(
   CheckedInt<int32_t> index = shistory->GetRequestedIndex() >= 0
                                   ? shistory->GetRequestedIndex()
                                   : shistory->Index();
+  MOZ_LOG(gSHLog, LogLevel::Debug,
+          ("HistoryGo(%d->%d) epoch %" PRIu64 "/id %" PRIu64, aOffset,
+           (index + aOffset).value(), aHistoryEpoch,
+           (uint64_t)(aContentId.isSome() ? aContentId.value() : 0)));
   index += aOffset;
   if (!index.isValid()) {
+    MOZ_LOG(gSHLog, LogLevel::Debug, ("Invalid index"));
     return;
   }
 
   // FIXME userinteraction bits may needs tweaks here.
 
+  // Implement aborting additional history navigations from within the same
+  // event spin of the content process.
+
+  uint64_t epoch;
+  bool sameEpoch = false;
+  Maybe<ContentParentId> id;
+  shistory->GetEpoch(epoch, id);
+
+  if (aContentId == id && epoch >= aHistoryEpoch) {
+    sameEpoch = true;
+    MOZ_LOG(gSHLog, LogLevel::Debug, ("Same epoch/id"));
+  }
+  // Don't update the epoch until we know if the target index is valid
+
   // GoToIndex checks that index is >= 0 and < length.
   nsTArray<nsSHistory::LoadEntryResult> loadResults;
-  nsresult rv = shistory->GotoIndex(index.value(), loadResults);
+  nsresult rv = shistory->GotoIndex(index.value(), loadResults, sameEpoch);
   if (NS_FAILED(rv)) {
+    MOZ_LOG(gSHLog, LogLevel::Debug,
+            ("Dropping HistoryGo - bad index or same epoch (not in same doc)"));
     return;
   }
-
+  if (epoch < aHistoryEpoch || aContentId != id) {
+    MOZ_LOG(gSHLog, LogLevel::Debug, ("Set epoch"));
+    shistory->SetEpoch(aHistoryEpoch, aContentId);
+  }
   aResolver(shistory->GetRequestedIndex());
   nsSHistory::LoadURIs(loadResults);
 }

docshell/base/CanonicalBrowsingContext.h

@@ -10,6 +10,7 @@
 #include "mozilla/dom/BrowsingContext.h"
 #include "mozilla/dom/MediaControlKeySource.h"
 #include "mozilla/dom/BrowsingContextWebProgress.h"
+#include "mozilla/dom/ipc/IdType.h"
 #include "mozilla/RefPtr.h"
 #include "mozilla/MozPromise.h"
 #include "nsCycleCollectionParticipant.h"
@@ -138,7 +139,9 @@ class CanonicalBrowsingContext final : public BrowsingContext {
 
   void RemoveFromSessionHistory();
 
-  void HistoryGo(int32_t aIndex, std::function<void(int32_t&&)>&& aResolver);
+  void HistoryGo(int32_t aIndex, uint64_t aHistoryEpoch,
+                 Maybe<ContentParentId> aContentId,
+                 std::function<void(int32_t&&)>&& aResolver);
 
   JSObject* WrapObject(JSContext* aCx,
                        JS::Handle<JSObject*> aGivenProto) override;

docshell/base/nsDocShell.cpp

@@ -11090,7 +11090,17 @@ nsresult nsDocShell::UpdateURLAndHistory(Document* aDocument, nsIURI* aNewURI,
     // Step 2.
 
     // Step 2.2, "Remove any tasks queued by the history traversal task
-    // source..."
+    // source that are associated with any Document objects in the
+    // top-level browsing context's document family."  This is very hard in
+    // SessionHistoryInParent since we can't synchronously access the
+    // pending navigations that are already sent to the parent. We can
+    // abort any AsyncGo navigations that are waiting to be sent.  If we
+    // send a message to the parent, it would be processed after any
+    // navigations previously sent.  So long as we consider the "history
+    // traversal task source" to be the list in this process we match the
+    // spec.  If we move the entire list to the parent, we can handle the
+    // aborting of loads there, but we don't have a way to synchronously
+    // remove entries as we do here for non-SHIP.
     RefPtr<ChildSHistory> shistory = GetRootSessionHistory();
     if (shistory) {
       shistory->RemovePendingHistoryNavigations();

docshell/shistory/ChildSHistory.cpp

@@ -16,6 +16,8 @@
 #include "nsDocShell.h"
 #include "nsXULAppAPI.h"
 
+extern mozilla::LazyLogModule gSHLog;
+
 namespace mozilla {
 namespace dom {
 
@@ -134,6 +136,9 @@ bool ChildSHistory::CanGo(int32_t aOffset) {
 
 void ChildSHistory::Go(int32_t aOffset, bool aRequireUserInteraction,
                        ErrorResult& aRv) {
+  CheckedInt<int32_t> index = Index();
+  MOZ_LOG(gSHLog, LogLevel::Debug,
+          ("nsHistory::Go(%d), current index = %d", aOffset, index.value()));
   if (aRequireUserInteraction && aOffset != -1 && aOffset != 1) {
     NS_ERROR(
         "aRequireUserInteraction may only be used with an offset of -1 or 1");
@@ -141,7 +146,6 @@ void ChildSHistory::Go(int32_t aOffset, bool aRequireUserInteraction,
     return;
   }
 
-  CheckedInt<int32_t> index = Index();
   while (true) {
     index += aOffset;
     if (!index.isValid()) {
@@ -150,8 +154,14 @@ void ChildSHistory::Go(int32_t aOffset, bool aRequireUserInteraction,
     }
 
     // See Bug 1650095.
-    if (mozilla::SessionHistoryInParent()) {
-      break;
+    if (mozilla::SessionHistoryInParent() && !mPendingEpoch) {
+      mPendingEpoch = true;
+      RefPtr<ChildSHistory> self(this);
+      NS_DispatchToCurrentThread(
+          NS_NewRunnableFunction("UpdateEpochRunnable", [self] {
+            self->mHistoryEpoch++;
+            self->mPendingEpoch = false;
+          }));
     }
 
     // Check for user interaction if desired, except for the first and last
@@ -171,16 +181,17 @@ void ChildSHistory::Go(int32_t aOffset, bool aRequireUserInteraction,
 
 void ChildSHistory::AsyncGo(int32_t aOffset, bool aRequireUserInteraction,
                             CallerType aCallerType, ErrorResult& aRv) {
+  CheckedInt<int32_t> index = Index();
+  MOZ_LOG(
+      gSHLog, LogLevel::Debug,
+      ("nsHistory::AsyncGo(%d), current index = %d", aOffset, index.value()));
   nsresult rv = mBrowsingContext->CheckLocationChangeRateLimit(aCallerType);
   if (NS_FAILED(rv)) {
+    MOZ_LOG(gSHLog, LogLevel::Debug, ("Rejected"));
     aRv.Throw(rv);
     return;
   }
 
-  if (!CanGo(aOffset)) {
-    return;
-  }
-
   RefPtr<PendingAsyncHistoryNavigation> asyncNav =
       new PendingAsyncHistoryNavigation(this, aOffset, aRequireUserInteraction);
   mPendingNavigations.insertBack(asyncNav);
@@ -189,20 +200,32 @@ void ChildSHistory::AsyncGo(int32_t aOffset, bool aRequireUserInteraction,
 
 void ChildSHistory::GotoIndex(int32_t aIndex, int32_t aOffset,
                               ErrorResult& aRv) {
+  MOZ_LOG(gSHLog, LogLevel::Debug,
+          ("nsHistory::GotoIndex(%d, %d), epoch %" PRIu64, aIndex, aOffset,
+           mHistoryEpoch));
   if (mozilla::SessionHistoryInParent()) {
     nsCOMPtr<nsISHistory> shistory = mHistory;
-    mBrowsingContext->HistoryGo(aOffset, [shistory](int32_t&& aRequestedIndex) {
-      // FIXME Should probably only do this for non-fission.
-      if (shistory) {
-        shistory->InternalSetRequestedIndex(aRequestedIndex);
-      }
-    });
+    mBrowsingContext->HistoryGo(
+        aOffset, mHistoryEpoch, [shistory](int32_t&& aRequestedIndex) {
+          // FIXME Should probably only do this for non-fission.
+          if (shistory) {
+            shistory->InternalSetRequestedIndex(aRequestedIndex);
+          }
+        });
   } else {
     aRv = mHistory->GotoIndex(aIndex);
   }
 }
 
 void ChildSHistory::RemovePendingHistoryNavigations() {
+  // Per the spec, this generally shouldn't remove all navigations - it
+  // depends if they're in the same document family or not.  We don't do
+  // that.  Also with SessionHistoryInParent, this can only abort AsyncGo's
+  // that have not yet been sent to the parent - see discussion of point
+  // 2.2 in comments in nsDocShell::UpdateURLAndHistory()
+  MOZ_LOG(
+      gSHLog, LogLevel::Debug,
+      ("RemovePendingHistoryNavigations: %zu", mPendingNavigations.length()));
   mPendingNavigations.clear();
 }
 

docshell/shistory/ChildSHistory.h

@@ -125,6 +125,7 @@ class ChildSHistory : public nsISupports, public nsWrapperCache {
 
   RefPtr<BrowsingContext> mBrowsingContext;
   nsCOMPtr<nsISHistory> mHistory;
+  // Can be removed once history-in-parent is the only way
   mozilla::LinkedList<PendingAsyncHistoryNavigation> mPendingNavigations;
   int32_t mIndex = -1;
   int32_t mLength = 0;
@@ -137,6 +138,10 @@ class ChildSHistory : public nsISupports, public nsWrapperCache {
   AutoTArray<PendingSHistoryChange, 2> mPendingSHistoryChanges;
 
   bool mAsyncHistoryLength = false;
+
+  // Needs to start 1 above default epoch in parent
+  uint64_t mHistoryEpoch = 1;
+  bool mPendingEpoch = false;
 };
 
 }  // namespace dom

docshell/shistory/nsSHistory.cpp

@@ -1653,8 +1653,10 @@ nsSHistory::EnsureCorrectEntryAtCurrIndex(nsISHEntry* aEntry) {
 }
 
 nsresult nsSHistory::GotoIndex(int32_t aIndex,
-                               nsTArray<LoadEntryResult>& aLoadResults) {
-  return LoadEntry(aIndex, LOAD_HISTORY, HIST_CMD_GOTOINDEX, aLoadResults);
+                               nsTArray<LoadEntryResult>& aLoadResults,
+                               bool aSameEpoch) {
+  return LoadEntry(aIndex, LOAD_HISTORY, HIST_CMD_GOTOINDEX, aLoadResults,
+                   aSameEpoch);
 }
 
 NS_IMETHODIMP_(bool)
@@ -1682,29 +1684,60 @@ nsresult nsSHistory::LoadNextPossibleEntry(
 
 nsresult nsSHistory::LoadEntry(int32_t aIndex, long aLoadType,
                                uint32_t aHistCmd,
-                               nsTArray<LoadEntryResult>& aLoadResults) {
+                               nsTArray<LoadEntryResult>& aLoadResults,
+                               bool aSameEpoch) {
+  MOZ_LOG(gSHistoryLog, LogLevel::Debug,
+          ("LoadEntry(%d, 0x%lx, %u)", aIndex, aLoadType, aHistCmd));
   if (!mRootBC) {
     return NS_ERROR_FAILURE;
   }
 
   if (aIndex < 0 || aIndex >= Length()) {
+    MOZ_LOG(gSHistoryLog, LogLevel::Debug, ("Index out of range"));
     // The index is out of range
     return NS_ERROR_FAILURE;
   }
 
   // This is a normal local history navigation.
-  // Keep note of requested history index in mRequestedIndex.
-  mRequestedIndex = aIndex;
 
   nsCOMPtr<nsISHEntry> prevEntry;
   nsCOMPtr<nsISHEntry> nextEntry;
   GetEntryAtIndex(mIndex, getter_AddRefs(prevEntry));
-  GetEntryAtIndex(mRequestedIndex, getter_AddRefs(nextEntry));
+  GetEntryAtIndex(aIndex, getter_AddRefs(nextEntry));
   if (!nextEntry || !prevEntry) {
     mRequestedIndex = -1;
     return NS_ERROR_FAILURE;
   }
 
+  if (mozilla::SessionHistoryInParent()) {
+    if (aHistCmd == HIST_CMD_GOTOINDEX) {
+      // https://html.spec.whatwg.org/#history-traversal:
+      // To traverse the history
+      // "If entry has a different Document object than the current entry, then
+      // run the following substeps: Remove any tasks queued by the history
+      // traversal task source..."
+      //
+      // aSameEpoch is true only if the navigations would have been
+      // generated in the same spin of the event loop (i.e. history.go(-2);
+      // history.go(-1)) and from the same content process.
+      if (aSameEpoch) {
+        bool same_doc = false;
+        prevEntry->SharesDocumentWith(nextEntry, &same_doc);
+        if (!same_doc) {
+          MOZ_LOG(
+              gSHistoryLog, LogLevel::Debug,
+              ("Aborting GotoIndex %d - same epoch and not same doc", aIndex));
+          // Ignore this load. Before SessionHistoryInParent, this would
+          // have been dropped in InternalLoad after we filter out SameDoc
+          // loads.
+          return NS_ERROR_FAILURE;
+        }
+      }
+    }
+  }
+  // Keep note of requested history index in mRequestedIndex; after all bailouts
+  mRequestedIndex = aIndex;
+
   // Remember that this entry is getting loaded at this point in the sequence
 
   nextEntry->SetLastTouched(++gTouchCounter);

docshell/shistory/nsSHistory.h

@@ -16,6 +16,7 @@
 #include "nsTObserverArray.h"
 #include "nsWeakReference.h"
 
+#include "mozilla/dom/ipc/IdType.h"
 #include "mozilla/LinkedList.h"
 #include "mozilla/UniquePtr.h"
 
@@ -160,7 +161,8 @@ class nsSHistory : public mozilla::LinkedListElement<nsSHistory>,
   nsresult Reload(uint32_t aReloadFlags,
                   nsTArray<LoadEntryResult>& aLoadResults);
   nsresult ReloadCurrentEntry(nsTArray<LoadEntryResult>& aLoadResults);
-  nsresult GotoIndex(int32_t aIndex, nsTArray<LoadEntryResult>& aLoadResults);
+  nsresult GotoIndex(int32_t aIndex, nsTArray<LoadEntryResult>& aLoadResults,
+                     bool aSameEpoch = false);
 
   void WindowIndices(int32_t aIndex, int32_t* aOutStartIndex,
                      int32_t* aOutEndIndex);
@@ -187,6 +189,17 @@ class nsSHistory : public mozilla::LinkedListElement<nsSHistory>,
   // replacing entries.
   void UpdateRootBrowsingContextState();
 
+  void GetEpoch(uint64_t& aEpoch,
+                mozilla::Maybe<mozilla::dom::ContentParentId>& aId) const {
+    aEpoch = mEpoch;
+    aId = mEpochParentId;
+  }
+  void SetEpoch(uint64_t aEpoch,
+                mozilla::Maybe<mozilla::dom::ContentParentId> aId) {
+    mEpoch = aEpoch;
+    mEpochParentId = aId;
+  }
+
  protected:
   virtual ~nsSHistory();
 
@@ -205,7 +218,8 @@ class nsSHistory : public mozilla::LinkedListElement<nsSHistory>,
                     nsTArray<LoadEntryResult>& aLoadResult);
 
   nsresult LoadEntry(int32_t aIndex, long aLoadType, uint32_t aHistCmd,
-                     nsTArray<LoadEntryResult>& aLoadResults);
+                     nsTArray<LoadEntryResult>& aLoadResults,
+                     bool aSameEpoch = false);
 
 #ifdef DEBUG
   nsresult PrintHistory();
@@ -267,6 +281,14 @@ class nsSHistory : public mozilla::LinkedListElement<nsSHistory>,
 
   // Max viewers allowed total, across all SHistory objects
   static int32_t sHistoryMaxTotalViewers;
+
+  // The epoch (and id) tell us what navigations occured within the same
+  // event-loop spin in the child.  We need to know this in order to
+  // implement spec requirements for dropping pending navigations when we
+  // do a history navigation, if it's not same-document.  Content processes
+  // update the epoch via a runnable on each ::Go (including AsyncGo).
+  uint64_t mEpoch = 0;
+  mozilla::Maybe<mozilla::dom::ContentParentId> mEpochParentId;
 };
 
 // CallerWillNotifyHistoryIndexAndLengthChanges is used to prevent

dom/base/nsHistory.cpp

@@ -22,6 +22,10 @@
 using namespace mozilla;
 using namespace mozilla::dom;
 
+extern LazyLogModule gSHistoryLog;
+
+#define LOG(format) MOZ_LOG(gSHistoryLog, mozilla::LogLevel::Debug, format)
+
 //
 //  History class implementation
 //
@@ -133,6 +137,7 @@ void nsHistory::GetState(JSContext* aCx, JS::MutableHandle<JS::Value> aResult,
 }
 
 void nsHistory::Go(int32_t aDelta, CallerType aCallerType, ErrorResult& aRv) {
+  LOG(("nsHistory::Go(%d)", aDelta));
   nsCOMPtr<nsPIDOMWindowInner> win(do_QueryReferent(mInnerWindow));
   if (!win || !win->HasActiveDocument()) {
     return aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);

dom/ipc/ContentParent.cpp

@@ -6934,9 +6934,9 @@ mozilla::ipc::IPCResult ContentParent::RecvHistoryCommit(
 
 mozilla::ipc::IPCResult ContentParent::RecvHistoryGo(
     const MaybeDiscarded<BrowsingContext>& aContext, int32_t aOffset,
-    HistoryGoResolver&& aResolveRequestedIndex) {
+    uint64_t aHistoryEpoch, HistoryGoResolver&& aResolveRequestedIndex) {
   if (!aContext.IsDiscarded()) {
-    aContext.get_canonical()->HistoryGo(aOffset,
+    aContext.get_canonical()->HistoryGo(aOffset, aHistoryEpoch, Some(ChildID()),
                                         std::move(aResolveRequestedIndex));
   }
   return IPC_OK();

dom/ipc/ContentParent.h

@@ -1339,7 +1339,7 @@ class ContentParent final
 
   mozilla::ipc::IPCResult RecvHistoryGo(
       const MaybeDiscarded<BrowsingContext>& aContext, int32_t aOffset,
-      HistoryGoResolver&& aResolveRequestedIndex);
+      uint64_t aHistoryEpoch, HistoryGoResolver&& aResolveRequestedIndex);
 
   mozilla::ipc::IPCResult RecvSessionHistoryUpdate(
       const MaybeDiscarded<BrowsingContext>& aContext, const int32_t& aIndex,

dom/ipc/PContent.ipdl

@@ -1677,7 +1677,7 @@ parent:
                         uint64_t aLoadID, nsID aChangeID, uint32_t aLoadType);
 
     async HistoryGo(MaybeDiscardedBrowsingContext aContext,
-                    int32_t aOffset) returns(int32_t requestedIndex);
+                    int32_t aOffset, uint64_t aHistoryEpoch) returns(int32_t requestedIndex);
 
     async BlobURLDataRequest(nsCString aBlobURL,
                              nsIPrincipal aTriggeringPrincipal,

testing/web-platform/meta/html/browsers/history/the-history-interface/traverse_the_history_1.html.ini

@@ -1,3 +0,0 @@
-[traverse_the_history_1.html]
-  disabled:
-    if fission: https://bugzilla.mozilla.org/show_bug.cgi?id=1656208

testing/web-platform/meta/html/browsers/history/the-history-interface/traverse_the_history_2.html.ini

@@ -1,3 +0,0 @@
-[traverse_the_history_2.html]
-  disabled:
-    if fission: https://bugzilla.mozilla.org/show_bug.cgi?id=1656208

testing/web-platform/meta/html/browsers/history/the-history-interface/traverse_the_history_3.html.ini

@@ -1,3 +0,0 @@
-[traverse_the_history_3.html]
-  disabled:
-    if fission: https://bugzilla.mozilla.org/show_bug.cgi?id=1656208

testing/web-platform/meta/html/browsers/history/the-history-interface/traverse_the_history_4.html.ini

@@ -1,3 +0,0 @@
-[traverse_the_history_4.html]
-  disabled:
-    if fission: https://bugzilla.mozilla.org/show_bug.cgi?id=1656208

testing/web-platform/meta/html/browsers/history/the-history-interface/traverse_the_history_5.html.ini

@@ -1,3 +0,0 @@
-[traverse_the_history_5.html]
-  disabled:
-    if fission: https://bugzilla.mozilla.org/show_bug.cgi?id=1656208