Bug 1609475, history.go(>1) should possibly load several iframes, r=annyG

Differential Revision: https://phabricator.services.mozilla.com/D63726 --HG-- extra : moz-landing-system : lando
2020-02-22 16:44:18 +00:00 · 2020-02-22 16:44:18 +00:00 · 4ecbebad86
--- a/docshell/shistory/PSHistory.ipdl
+++ b/docshell/shistory/PSHistory.ipdl
@ -26,7 +26,7 @@ struct LoadSHEntryData

 union LoadSHEntryResult {
  nsresult;
-  LoadSHEntryData;
+  LoadSHEntryData[];
 };

 sync protocol PSHistory {
--- a/docshell/shistory/SHistoryChild.cpp
+++ b/docshell/shistory/SHistoryChild.cpp
@ -369,23 +369,27 @@ SHistoryChild::CreateEntry(nsISHEntry** aEntry) {
  return NS_OK;
 }

-nsresult SHistoryChild::LoadURI(LoadSHEntryData& aLoadData) {
-  if (NS_WARN_IF(aLoadData.browsingContext().IsNullOrDiscarded())) {
-    return NS_ERROR_FAILURE;
+nsresult SHistoryChild::LoadURI(nsTArray<LoadSHEntryData>& aLoadData) {
+  for (LoadSHEntryData& l : aLoadData) {
+     if (l.browsingContext().IsNullOrDiscarded()) {
+       continue;
     }

-  nsCOMPtr<nsIDocShell> docShell =
-      aLoadData.browsingContext().get()->GetDocShell();
-  NS_ENSURE_TRUE(docShell, NS_ERROR_FAILURE);
+    nsCOMPtr<nsIDocShell> docShell = l.browsingContext().get()->GetDocShell();
+    if (!docShell) {
+      continue;
+    }

    RefPtr<SHEntryChild> entry;
-  if (aLoadData.shEntry()) {
-    entry = aLoadData.shEntry()->ToSHEntryChild();
+    if (l.shEntry()) {
+      entry = l.shEntry()->ToSHEntryChild();
    }

    // FIXME Should this be sent through IPC?
-  aLoadData.loadState()->SetSHEntry(entry);
-  return docShell->LoadURI(aLoadData.loadState(), false);
+    l.loadState()->SetSHEntry(entry);
+    docShell->LoadURI(l.loadState(), false);
+  }
+  return NS_OK;
 }

 }  // namespace dom
--- a/docshell/shistory/SHistoryChild.h
+++ b/docshell/shistory/SHistoryChild.h
@ -69,7 +69,7 @@ class SHistoryChild final : public PSHistoryChild,

  ~SHistoryChild() = default;

-  nsresult LoadURI(LoadSHEntryData& aLoadData);
+  nsresult LoadURI(nsTArray<LoadSHEntryData>& aLoadData);

  // Track all bfcache entries and evict on expiration.
  mozilla::UniquePtr<HistoryTracker> mHistoryTracker;
--- a/docshell/shistory/SHistoryParent.cpp
+++ b/docshell/shistory/SHistoryParent.cpp
@ -24,13 +24,19 @@ LegacySHistory::LegacySHistory(SHistoryParent* aSHistoryParent,
  aRootBC->SetSessionHistory(this);
 }

-static void FillInLoadResult(nsresult aRv,
-                             const nsSHistory::LoadEntryResult& aLoadResult,
+static void FillInLoadResult(
+    nsresult aRv, const nsTArray<nsSHistory::LoadEntryResult>& aLoadResults,
    LoadSHEntryResult* aResult) {
  if (NS_SUCCEEDED(aRv)) {
-    *aResult = LoadSHEntryData(
-        static_cast<LegacySHEntry*>(aLoadResult.mLoadState->SHEntry()),
-        aLoadResult.mBrowsingContext, aLoadResult.mLoadState);
+    nsTArray<LoadSHEntryData> data;
+    data.SetCapacity(aLoadResults.Length());
+    for (const nsSHistory::LoadEntryResult& l : aLoadResults) {
+      data.AppendElement(
+          LoadSHEntryData(static_cast<LegacySHEntry*>(l.mLoadState->SHEntry()),
+                          l.mBrowsingContext, l.mLoadState));
+    }
+
+    *aResult = data;
  } else {
    *aResult = aRv;
  }
@ -95,12 +101,10 @@ bool SHistoryParent::RecvPurgeHistory(int32_t aNumEntries, nsresult* aResult) {
 }

 bool SHistoryParent::RecvReloadCurrentEntry(LoadSHEntryResult* aLoadResult) {
-  nsSHistory::LoadEntryResult loadResult;
-  nsresult rv = mHistory->ReloadCurrentEntry(loadResult);
+  nsTArray<nsSHistory::LoadEntryResult> loadResults;
+  nsresult rv = mHistory->ReloadCurrentEntry(loadResults);
  if (NS_SUCCEEDED(rv)) {
-    *aLoadResult = LoadSHEntryData(
-        static_cast<LegacySHEntry*>(loadResult.mLoadState->SHEntry()),
-        loadResult.mBrowsingContext, loadResult.mLoadState);
+    FillInLoadResult(rv, loadResults, aLoadResult);
  } else {
    *aLoadResult = rv;
  }
@ -109,9 +113,9 @@ bool SHistoryParent::RecvReloadCurrentEntry(LoadSHEntryResult* aLoadResult) {

 bool SHistoryParent::RecvGotoIndex(int32_t aIndex,
                                   LoadSHEntryResult* aLoadResult) {
-  nsSHistory::LoadEntryResult loadResult;
-  nsresult rv = mHistory->GotoIndex(aIndex, loadResult);
-  FillInLoadResult(rv, loadResult, aLoadResult);
+  nsTArray<nsSHistory::LoadEntryResult> loadResults;
+  nsresult rv = mHistory->GotoIndex(aIndex, loadResults);
+  FillInLoadResult(rv, loadResults, aLoadResult);
  return true;
 }

@ -193,12 +197,12 @@ bool SHistoryParent::RecvRemoveFrameEntries(PSHEntryParent* aEntry) {

 bool SHistoryParent::RecvReload(const uint32_t& aReloadFlags,
                                LoadSHEntryResult* aLoadResult) {
-  Maybe<nsSHistory::LoadEntryResult> loadResult;
-  nsresult rv = mHistory->Reload(aReloadFlags, loadResult);
-  if (NS_SUCCEEDED(rv) && !loadResult) {
+  nsTArray<nsSHistory::LoadEntryResult> loadResults;
+  nsresult rv = mHistory->Reload(aReloadFlags, loadResults);
+  if (NS_SUCCEEDED(rv) && loadResults.IsEmpty()) {
    *aLoadResult = NS_OK;
  } else {
-    FillInLoadResult(rv, loadResult.ref(), aLoadResult);
+    FillInLoadResult(rv, loadResults, aLoadResult);
  }
  return true;
 }
--- a/docshell/shistory/nsSHistory.cpp
+++ b/docshell/shistory/nsSHistory.cpp
@ -830,27 +830,29 @@ nsSHistory::EvictAllContentViewers() {
  return NS_OK;
 }

-static nsresult LoadURI(nsSHistory::LoadEntryResult& aLoadResult) {
-  return aLoadResult.mBrowsingContext->LoadURI(nullptr, aLoadResult.mLoadState,
-                                               false);
+static void LoadURIs(nsTArray<nsSHistory::LoadEntryResult>& aLoadResults) {
+  for (nsSHistory::LoadEntryResult& loadEntry : aLoadResults) {
+    loadEntry.mBrowsingContext->LoadURI(nullptr, loadEntry.mLoadState, false);
+  }
 }

 NS_IMETHODIMP
 nsSHistory::Reload(uint32_t aReloadFlags) {
-  Maybe<LoadEntryResult> loadResult;
-  nsresult rv = Reload(aReloadFlags, loadResult);
+  nsTArray<LoadEntryResult> loadResults;
+  nsresult rv = Reload(aReloadFlags, loadResults);
  NS_ENSURE_SUCCESS(rv, rv);

-  if (!loadResult) {
+  if (loadResults.IsEmpty()) {
    return NS_OK;
  }

-  return LoadURI(loadResult.ref());
+  LoadURIs(loadResults);
+  return NS_OK;
 }

 nsresult nsSHistory::Reload(uint32_t aReloadFlags,
-                            Maybe<LoadEntryResult>& aLoadResult) {
-  MOZ_ASSERT(!aLoadResult.isSome());
+                            nsTArray<LoadEntryResult>& aLoadResults) {
+  MOZ_ASSERT(aLoadResults.IsEmpty());

  uint32_t loadType;
  if (aReloadFlags & nsIWebNavigation::LOAD_FLAGS_BYPASS_PROXY &&
@ -878,10 +880,9 @@ nsresult nsSHistory::Reload(uint32_t aReloadFlags,
    return NS_OK;
  }

-  aLoadResult.emplace();
-  nsresult rv = LoadEntry(mIndex, loadType, HIST_CMD_RELOAD, aLoadResult.ref());
+  nsresult rv = LoadEntry(mIndex, loadType, HIST_CMD_RELOAD, aLoadResults);
  if (NS_FAILED(rv)) {
-    aLoadResult.reset();
+    aLoadResults.Clear();
    return rv;
  }

@ -890,18 +891,20 @@ nsresult nsSHistory::Reload(uint32_t aReloadFlags,

 NS_IMETHODIMP
 nsSHistory::ReloadCurrentEntry() {
-  LoadEntryResult loadResult;
-  nsresult rv = ReloadCurrentEntry(loadResult);
+  nsTArray<LoadEntryResult> loadResults;
+  nsresult rv = ReloadCurrentEntry(loadResults);
  NS_ENSURE_SUCCESS(rv, rv);

-  return LoadURI(loadResult);
+  LoadURIs(loadResults);
+  return NS_OK;
 }

-nsresult nsSHistory::ReloadCurrentEntry(LoadEntryResult& aLoadResult) {
+nsresult nsSHistory::ReloadCurrentEntry(
+    nsTArray<LoadEntryResult>& aLoadResults) {
  // Notify listeners
  NOTIFY_LISTENERS(OnHistoryGotoIndex, ());

-  return LoadEntry(mIndex, LOAD_HISTORY, HIST_CMD_RELOAD, aLoadResult);
+  return LoadEntry(mIndex, LOAD_HISTORY, HIST_CMD_RELOAD, aLoadResults);
 }

 void nsSHistory::EvictOutOfRangeWindowContentViewers(int32_t aIndex) {
@ -1382,11 +1385,12 @@ nsSHistory::UpdateIndex() {

 NS_IMETHODIMP
 nsSHistory::GotoIndex(int32_t aIndex) {
-  LoadEntryResult loadResult;
-  nsresult rv = GotoIndex(aIndex, loadResult);
+  nsTArray<LoadEntryResult> loadResults;
+  nsresult rv = GotoIndex(aIndex, loadResults);
  NS_ENSURE_SUCCESS(rv, rv);

-  return LoadURI(loadResult);
+  LoadURIs(loadResults);
+  return NS_OK;
 }

 NS_IMETHODIMP_(void)
@ -1397,26 +1401,27 @@ nsSHistory::EnsureCorrectEntryAtCurrIndex(nsISHEntry* aEntry) {
  }
 }

-nsresult nsSHistory::GotoIndex(int32_t aIndex, LoadEntryResult& aLoadResult) {
-  return LoadEntry(aIndex, LOAD_HISTORY, HIST_CMD_GOTOINDEX, aLoadResult);
+nsresult nsSHistory::GotoIndex(int32_t aIndex,
+                               nsTArray<LoadEntryResult>& aLoadResults) {
+  return LoadEntry(aIndex, LOAD_HISTORY, HIST_CMD_GOTOINDEX, aLoadResults);
 }

-nsresult nsSHistory::LoadNextPossibleEntry(int32_t aNewIndex, long aLoadType,
-                                           uint32_t aHistCmd,
-                                           LoadEntryResult& aLoadResult) {
+nsresult nsSHistory::LoadNextPossibleEntry(
+    int32_t aNewIndex, long aLoadType, uint32_t aHistCmd,
+    nsTArray<LoadEntryResult>& aLoadResults) {
  mRequestedIndex = -1;
  if (aNewIndex < mIndex) {
-    return LoadEntry(aNewIndex - 1, aLoadType, aHistCmd, aLoadResult);
+    return LoadEntry(aNewIndex - 1, aLoadType, aHistCmd, aLoadResults);
  }
  if (aNewIndex > mIndex) {
-    return LoadEntry(aNewIndex + 1, aLoadType, aHistCmd, aLoadResult);
+    return LoadEntry(aNewIndex + 1, aLoadType, aHistCmd, aLoadResults);
  }
  return NS_ERROR_FAILURE;
 }

 nsresult nsSHistory::LoadEntry(int32_t aIndex, long aLoadType,
                               uint32_t aHistCmd,
-                               LoadEntryResult& aLoadResult) {
+                               nsTArray<LoadEntryResult>& aLoadResults) {
  if (!mRootBC) {
    return NS_ERROR_FAILURE;
  }
@ -1457,16 +1462,16 @@ nsresult nsSHistory::LoadEntry(int32_t aIndex, long aLoadType,

  if (mRequestedIndex == mIndex) {
    // Possibly a reload case
-    return InitiateLoad(nextEntry, mRootBC, aLoadType, aLoadResult);
+    return InitiateLoad(nextEntry, mRootBC, aLoadType, aLoadResults);
  }

  // Going back or forward.
  bool differenceFound = false;
  nsresult rv = LoadDifferingEntries(prevEntry, nextEntry, mRootBC, aLoadType,
-                                     differenceFound, aLoadResult);
+                                     differenceFound, aLoadResults);
  if (!differenceFound) {
    // We did not find any differences. Go further in the history.
-    return LoadNextPossibleEntry(aIndex, aLoadType, aHistCmd, aLoadResult);
+    return LoadNextPossibleEntry(aIndex, aLoadType, aHistCmd, aLoadResults);
  }

  return rv;
@ -1474,7 +1479,8 @@ nsresult nsSHistory::LoadEntry(int32_t aIndex, long aLoadType,

 nsresult nsSHistory::LoadDifferingEntries(
    nsISHEntry* aPrevEntry, nsISHEntry* aNextEntry, BrowsingContext* aParent,
-    long aLoadType, bool& aDifferenceFound, LoadEntryResult& aLoadResult) {
+    long aLoadType, bool& aDifferenceFound,
+    nsTArray<LoadEntryResult>& aLoadResults) {
  if (!aPrevEntry || !aNextEntry || !aParent) {
    return NS_ERROR_FAILURE;
  }
@ -1489,7 +1495,7 @@ nsresult nsSHistory::LoadDifferingEntries(

    // Set the Subframe flag if not navigating the root docshell.
    aNextEntry->SetIsSubFrame(aParent != mRootBC);
-    return InitiateLoad(aNextEntry, aParent, aLoadType, aLoadResult);
+    return InitiateLoad(aNextEntry, aParent, aLoadType, aLoadResults);
  }

  // The entries are the same, so compare any child frames
@ -1543,17 +1549,18 @@ nsresult nsSHistory::LoadDifferingEntries(
    // This will either load a new page to shell or some subshell or
    // do nothing.
    LoadDifferingEntries(pChild, nChild, bcChild, aLoadType, aDifferenceFound,
-                         aLoadResult);
+                         aLoadResults);
  }
  return result;
 }

 nsresult nsSHistory::InitiateLoad(nsISHEntry* aFrameEntry,
                                  BrowsingContext* aFrameBC, long aLoadType,
-                                  LoadEntryResult& aLoadResult) {
+                                  nsTArray<LoadEntryResult>& aLoadResults) {
  NS_ENSURE_STATE(aFrameBC && aFrameEntry);

-  aLoadResult.mBrowsingContext = aFrameBC;
+  LoadEntryResult* loadResult = aLoadResults.AppendElement();
+  loadResult->mBrowsingContext = aFrameBC;

  nsCOMPtr<nsIURI> newURI = aFrameEntry->GetURI();
  RefPtr<nsDocShellLoadState> loadState = new nsDocShellLoadState(newURI);
@ -1580,7 +1587,7 @@ nsresult nsSHistory::InitiateLoad(nsISHEntry* aFrameEntry,
  nsCOMPtr<nsIContentSecurityPolicy> csp = aFrameEntry->GetCsp();
  loadState->SetCsp(csp);

-  aLoadResult.mLoadState = std::move(loadState);
+  loadResult->mLoadState = std::move(loadState);

  return NS_OK;
 }
--- a/docshell/shistory/nsSHistory.h
+++ b/docshell/shistory/nsSHistory.h
@ -140,9 +140,10 @@ class nsSHistory : public mozilla::LinkedListElement<nsSHistory>,
  // If this doesn't return an error then either aLoadResult is set to nothing,
  // in which case the caller should ignore the load, or it returns a valid
  // LoadEntryResult in aLoadResult which the caller should use to do the load.
-  nsresult Reload(uint32_t aReloadFlags, Maybe<LoadEntryResult>& aLoadResult);
-  nsresult ReloadCurrentEntry(LoadEntryResult& aLoadResult);
-  nsresult GotoIndex(int32_t aIndex, LoadEntryResult& aLoadResult);
+  nsresult Reload(uint32_t aReloadFlags,
+                  nsTArray<LoadEntryResult>& aLoadResults);
+  nsresult ReloadCurrentEntry(nsTArray<LoadEntryResult>& aLoadResults);
+  nsresult GotoIndex(int32_t aIndex, nsTArray<LoadEntryResult>& aLoadResults);

  void WindowIndices(int32_t aIndex, int32_t* aOutStartIndex,
                     int32_t* aOutEndIndex);
@ -160,13 +161,13 @@ class nsSHistory : public mozilla::LinkedListElement<nsSHistory>,
  nsresult LoadDifferingEntries(nsISHEntry* aPrevEntry, nsISHEntry* aNextEntry,
                                mozilla::dom::BrowsingContext* aRootBC,
                                long aLoadType, bool& aDifferenceFound,
-                                LoadEntryResult& aLoadResult);
+                                nsTArray<LoadEntryResult>& aLoadResults);
  nsresult InitiateLoad(nsISHEntry* aFrameEntry,
                        mozilla::dom::BrowsingContext* aFrameBC, long aLoadType,
-                        LoadEntryResult& aLoadResult);
+                        nsTArray<LoadEntryResult>& aLoadResult);

  nsresult LoadEntry(int32_t aIndex, long aLoadType, uint32_t aHistCmd,
-                     LoadEntryResult& aLoad);
+                     nsTArray<LoadEntryResult>& aLoadResults);

 #ifdef DEBUG
  nsresult PrintHistory();
@ -190,7 +191,7 @@ class nsSHistory : public mozilla::LinkedListElement<nsSHistory>,

  nsresult LoadNextPossibleEntry(int32_t aNewIndex, long aLoadType,
                                 uint32_t aHistCmd,
-                                 LoadEntryResult& aLoadResult);
+                                 nsTArray<LoadEntryResult>& aLoadResults);

  // aIndex is the index of the entry which may be removed.
  // If aKeepNext is true, aIndex is compared to aIndex + 1,
--- a/docshell/test/navigation/file_bug1609475.html
+++ b/docshell/test/navigation/file_bug1609475.html
@ -0,0 +1,51 @@
+<html>
+  <head>
+    <script>
+
+    var loadCount = 0;
+    function loadListener(event) {
+      ++loadCount;
+      if (loadCount == 2) {
+        // Use a timer to ensure we don't get extra load events.
+        setTimeout(function() {
+          var doc1URI = document.getElementById("i1").contentDocument.documentURI;
+          opener.ok(doc1URI.includes("frame1.html"),
+                    "Should have loaded the initial page to the first iframe. Got " + doc1URI);
+          var doc2URI = document.getElementById("i2").contentDocument.documentURI;
+          opener.ok(doc2URI.includes("frame1.html"),
+                    "Should have loaded the initial page to the second iframe. Got " + doc2URI);
+          opener.nextTest();
+          window.close();
+        }, 1000);
+      } else if (loadCount > 2) {
+        opener.ok(false, "Too many load events");
+      }
+      // if we don't get enough load events, the test will time out.
+    }
+
+    function setupIframe(id) {
+      var ifr = document.getElementById(id);
+      return new Promise(function(resolve) {
+        ifr.onload = function() {
+          // Replace load listener to catch page loads from the session history.
+          ifr.onload = loadListener;
+          // Need to use setTimeout, because triggering loads inside
+          // load event listener has special behavior.
+          setTimeout(resolve);
+        }
+        ifr.contentWindow.location.href = "frame2.html";
+      });
+    }
+
+    async function test() {
+      await setupIframe("i1");
+      await setupIframe("i2");
+      history.go(-2);
+    }
+    </script>
+  </head>
+  <body onload="setTimeout(test)">
+    <iframe id="i1" src="frame1.html"></iframe>
+    <iframe id="i2" src="frame1.html"></iframe>
+  </body>
+</html>
--- a/docshell/test/navigation/mochitest.ini
+++ b/docshell/test/navigation/mochitest.ini
@ -57,6 +57,7 @@ support-files =
  file_bug1375833-frame1.html
  file_bug1375833-frame2.html
  test_bug145971.html
+  file_bug1609475.html

 [test_bug13871.html]
 [test_bug270414.html]
--- a/docshell/test/navigation/test_sessionhistory.html
+++ b/docshell/test/navigation/test_sessionhistory.html
@ -34,6 +34,7 @@ var testFiles =
    "file_bug1300461.html",
    "file_bug1326251.html",
    "file_bug1379762-1.html",
+    "file_bug1609475.html",
  ];
 var testCount = 0; // Used by the test files.