Bug 1594938 - Crash in [@ nsSHistory::Reload]. r=smaug

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

--HG--
extra : moz-landing-system : lando

docshell/shistory/SHistoryParent.cpp

@@ -24,7 +24,7 @@ LegacySHistory::LegacySHistory(SHistoryParent* aSHistoryParent,
   aRootBC->SetSessionHistory(this);
 }
 
-static void FillInLoadResult(PContentParent* aManager, nsresult aRv,
+static void FillInLoadResult(nsresult aRv,
                              const nsSHistory::LoadEntryResult& aLoadResult,
                              LoadSHEntryResult* aResult) {
   if (NS_SUCCEEDED(aRv)) {
@@ -111,7 +111,7 @@ bool SHistoryParent::RecvGotoIndex(int32_t aIndex,
                                    LoadSHEntryResult* aLoadResult) {
   nsSHistory::LoadEntryResult loadResult;
   nsresult rv = mHistory->GotoIndex(aIndex, loadResult);
-  FillInLoadResult(Manager(), rv, loadResult, aLoadResult);
+  FillInLoadResult(rv, loadResult, aLoadResult);
   return true;
 }
 
@@ -193,9 +193,13 @@ bool SHistoryParent::RecvRemoveFrameEntries(PSHEntryParent* aEntry) {
 
 bool SHistoryParent::RecvReload(const uint32_t& aReloadFlags,
                                 LoadSHEntryResult* aLoadResult) {
-  nsSHistory::LoadEntryResult loadResult;
+  Maybe<nsSHistory::LoadEntryResult> loadResult;
   nsresult rv = mHistory->Reload(aReloadFlags, loadResult);
-  FillInLoadResult(Manager(), rv, loadResult, aLoadResult);
+  if (NS_SUCCEEDED(rv) && !loadResult) {
+    *aLoadResult = NS_OK;
+  } else {
+    FillInLoadResult(rv, loadResult.ref(), aLoadResult);
+  }
   return true;
 }
 

docshell/shistory/nsSHistory.cpp

@@ -837,15 +837,21 @@ static nsresult LoadURI(nsSHistory::LoadEntryResult& aLoadResult) {
 
 NS_IMETHODIMP
 nsSHistory::Reload(uint32_t aReloadFlags) {
-  LoadEntryResult loadResult;
+  Maybe<LoadEntryResult> loadResult;
   nsresult rv = Reload(aReloadFlags, loadResult);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  return LoadURI(loadResult);
+  if (!loadResult) {
+    return NS_OK;
+  }
+
+  return LoadURI(loadResult.ref());
 }
 
 nsresult nsSHistory::Reload(uint32_t aReloadFlags,
-                            LoadEntryResult& aLoadResult) {
+                            Maybe<LoadEntryResult>& aLoadResult) {
+  MOZ_ASSERT(!aLoadResult.isSome());
+
   uint32_t loadType;
   if (aReloadFlags & nsIWebNavigation::LOAD_FLAGS_BYPASS_PROXY &&
       aReloadFlags & nsIWebNavigation::LOAD_FLAGS_BYPASS_CACHE) {
@@ -872,7 +878,14 @@ nsresult nsSHistory::Reload(uint32_t aReloadFlags,
     return NS_OK;
   }
 
-  return LoadEntry(mIndex, loadType, HIST_CMD_RELOAD, aLoadResult);
+  aLoadResult.emplace();
+  nsresult rv = LoadEntry(mIndex, loadType, HIST_CMD_RELOAD, aLoadResult.ref());
+  if (NS_FAILED(rv)) {
+    aLoadResult.reset();
+    return rv;
+  }
+
+  return NS_OK;
 }
 
 NS_IMETHODIMP

docshell/shistory/nsSHistory.h

@@ -137,7 +137,10 @@ class nsSHistory : public mozilla::LinkedListElement<nsSHistory>,
     RefPtr<nsDocShellLoadState> mLoadState;
   };
 
-  nsresult Reload(uint32_t aReloadFlags, LoadEntryResult& aLoadResult);
+  // 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);
 

docshell/test/browser/browser.ini

@@ -75,6 +75,7 @@ support-files =
 [browser_bug1543077-2.js]
 [browser_bug1543077-3.js]
 [browser_bug1543077-4.js]
+[browser_bug1594938.js]
 [browser_bug1206879.js]
 [browser_bug1309900_crossProcessHistoryNavigation.js]
 [browser_bug1328501.js]

docshell/test/browser/browser_bug1594938.js

@@ -0,0 +1,99 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/**
+ * Test for Bug 1594938
+ *
+ * If a session history listener blocks reloads we shouldn't crash.
+ */
+
+add_task(async function test() {
+  await BrowserTestUtils.withNewTab(
+    { gBrowser, url: "https://example.com/" },
+    async function(browser) {
+      if (!SpecialPowers.getBoolPref("fission.sessionHistoryInParent")) {
+        await SpecialPowers.spawn(browser, [], async function() {
+          let history = this.content.docShell.QueryInterface(
+            Ci.nsIWebNavigation
+          ).sessionHistory;
+
+          let testDone = {};
+          testDone.promise = new Promise(resolve => {
+            testDone.resolve = resolve;
+          });
+
+          let listenerCalled = false;
+          let listener = {
+            OnHistoryNewEntry: aNewURI => {},
+            OnHistoryReload: () => {
+              listenerCalled = true;
+              this.content.setTimeout(() => {
+                testDone.resolve();
+              });
+              return false;
+            },
+            OnHistoryGotoIndex: () => {},
+            OnHistoryPurge: () => {},
+            OnHistoryReplaceEntry: () => {},
+
+            QueryInterface: ChromeUtils.generateQI([
+              Ci.nsISHistoryListener,
+              Ci.nsISupportsWeakReference,
+            ]),
+          };
+
+          history.legacySHistory.addSHistoryListener(listener);
+
+          history.reload(Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE);
+          await testDone.promise;
+
+          Assert.ok(listenerCalled, "reloads were blocked");
+
+          history.legacySHistory.removeSHistoryListener(listener);
+        });
+
+        return;
+      }
+
+      let history = browser.browsingContext.sessionHistory;
+
+      let testDone = {};
+      testDone.promise = new Promise(resolve => {
+        testDone.resolve = resolve;
+      });
+
+      let listenerCalled = false;
+      let listener = {
+        OnHistoryNewEntry: aNewURI => {},
+        OnHistoryReload: () => {
+          listenerCalled = true;
+          setTimeout(() => {
+            testDone.resolve();
+          });
+          return false;
+        },
+        OnHistoryGotoIndex: () => {},
+        OnHistoryPurge: () => {},
+        OnHistoryReplaceEntry: () => {},
+
+        QueryInterface: ChromeUtils.generateQI([
+          Ci.nsISHistoryListener,
+          Ci.nsISupportsWeakReference,
+        ]),
+      };
+
+      history.addSHistoryListener(listener);
+
+      await SpecialPowers.spawn(browser, [], () => {
+        let history = this.content.docShell.QueryInterface(Ci.nsIWebNavigation)
+          .sessionHistory;
+        history.reload(Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE);
+      });
+      await testDone.promise;
+
+      Assert.ok(listenerCalled, "reloads were blocked");
+
+      history.removeSHistoryListener(listener);
+    }
+  );
+});