Bug 1594938 - Crash in [@ nsSHistory::Reload]. r=smaug
authorPeter Van der Beken <peterv@propagandism.org>
Fri, 15 Nov 2019 11:12:54 +0000
changeset 502154 8157c99955b178839710ca42af23925478a680b2
parent 502153 0f20566c7b2fa7b1204e432828940fd81ab2a197
child 502155 4158e16441d6fe5762cf55c6466468007ee456e7
push id114172
push userdluca@mozilla.com
push dateTue, 19 Nov 2019 11:31:10 +0000
treeherdermozilla-inbound@b5c5ba07d3db [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1594938
milestone72.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1594938 - Crash in [@ nsSHistory::Reload]. r=smaug Differential Revision: https://phabricator.services.mozilla.com/D53010
docshell/shistory/SHistoryParent.cpp
docshell/shistory/nsSHistory.cpp
docshell/shistory/nsSHistory.h
docshell/test/browser/browser.ini
docshell/test/browser/browser_bug1594938.js
--- a/docshell/shistory/SHistoryParent.cpp
+++ b/docshell/shistory/SHistoryParent.cpp
@@ -19,17 +19,17 @@ namespace dom {
 LegacySHistory::LegacySHistory(SHistoryParent* aSHistoryParent,
                                CanonicalBrowsingContext* aRootBC,
                                const nsID& aDocShellID)
     : nsSHistory(aRootBC, aDocShellID), mSHistoryParent(aSHistoryParent) {
   mIsRemote = true;
   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)) {
     *aResult = LoadSHEntryData(
         static_cast<LegacySHEntry*>(aLoadResult.mLoadState->SHEntry()),
         aLoadResult.mBrowsingContext, aLoadResult.mLoadState);
   } else {
     *aResult = aRv;
@@ -106,17 +106,17 @@ bool SHistoryParent::RecvReloadCurrentEn
   }
   return true;
 }
 
 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;
 }
 
 bool SHistoryParent::RecvGetIndexOfEntry(PSHEntryParent* aEntry,
                                          int32_t* aIndex) {
   MOZ_ASSERT(Manager() == aEntry->Manager());
   *aIndex =
       mHistory->GetIndexOfEntry(static_cast<SHEntryParent*>(aEntry)->mEntry);
@@ -188,19 +188,23 @@ bool SHistoryParent::RecvRemoveEntries(n
 bool SHistoryParent::RecvRemoveFrameEntries(PSHEntryParent* aEntry) {
   mHistory->RemoveFrameEntries(
       static_cast<SHEntryParent*>(aEntry)->GetSHEntry());
   return true;
 }
 
 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;
 }
 
 bool SHistoryParent::RecvGetAllEntries(
     nsTArray<RefPtr<CrossProcessSHEntry>>* aEntries) {
   nsTArray<nsCOMPtr<nsISHEntry>>& entries = mHistory->Entries();
   uint32_t length = entries.Length();
   aEntries->AppendElements(length);
--- a/docshell/shistory/nsSHistory.cpp
+++ b/docshell/shistory/nsSHistory.cpp
@@ -832,25 +832,31 @@ nsSHistory::EvictAllContentViewers() {
 
 static nsresult LoadURI(nsSHistory::LoadEntryResult& aLoadResult) {
   return aLoadResult.mBrowsingContext->LoadURI(nullptr, aLoadResult.mLoadState,
                                                false);
 }
 
 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) {
     loadType = LOAD_RELOAD_BYPASS_PROXY_AND_CACHE;
   } else if (aReloadFlags & nsIWebNavigation::LOAD_FLAGS_BYPASS_PROXY) {
     loadType = LOAD_RELOAD_BYPASS_PROXY;
   } else if (aReloadFlags & nsIWebNavigation::LOAD_FLAGS_BYPASS_CACHE) {
     loadType = LOAD_RELOAD_BYPASS_CACHE;
@@ -867,17 +873,24 @@ nsresult nsSHistory::Reload(uint32_t aRe
   // is public. So send the reload notifications with the
   // nsIWebNavigation flags.
   bool canNavigate = true;
   NOTIFY_LISTENERS_CANCELABLE(OnHistoryReload, canNavigate, (&canNavigate));
   if (!canNavigate) {
     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
 nsSHistory::ReloadCurrentEntry() {
   LoadEntryResult loadResult;
   nsresult rv = ReloadCurrentEntry(loadResult);
   NS_ENSURE_SUCCESS(rv, rv);
 
--- a/docshell/shistory/nsSHistory.h
+++ b/docshell/shistory/nsSHistory.h
@@ -132,17 +132,20 @@ class nsSHistory : public mozilla::Linke
   // index + VIEWER_WINDOW alive.
   static const int32_t VIEWER_WINDOW = 3;
 
   struct LoadEntryResult {
     RefPtr<mozilla::dom::BrowsingContext> mBrowsingContext;
     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);
 
   void WindowIndices(int32_t aIndex, int32_t* aOutStartIndex,
                      int32_t* aOutEndIndex);
   void NotifyListenersContentViewerEvicted(uint32_t aNumEvicted);
 
  protected:
--- a/docshell/test/browser/browser.ini
+++ b/docshell/test/browser/browser.ini
@@ -70,16 +70,17 @@ support-files =
   file_cross_process_csp_inheritance.html
   file_open_about_blank.html
   file_slow_load.sjs
 
 [browser_bug1543077-1.js]
 [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]
 [browser_bug1347823.js]
 [browser_bug134911.js]
 [browser_bug1415918_beforeunload_options.js]
 [browser_bug234628-1.js]
 [browser_bug234628-10.js]
new file mode 100644
--- /dev/null
+++ b/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);
+    }
+  );
+});