Bug 1721217 - Part 5: Change the error code when we cancel loads due to another one starting, r=nika

This allows us to move away from using IsNavigating field in parent-controlled
paths.  Use a new distinct error code in cases when we cancel loads in
Canonical BC due to another load starting. This way, we know to not reset the
urlbar if we are doing another load.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1721217#c10 for longer
explanation of what is going on here.

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

browser/base/content/tabbrowser.js

@@ -6541,9 +6541,24 @@
             // before the location changed.
 
             this.mBrowser.userTypedValue = null;
-
+            // When browser.tabs.documentchannel.parent-controlled pref and SHIP
+            // are enabled and a load gets cancelled due to another one
+            // starting, the error is NS_BINDING_CANCELLED_OLD_LOAD.
+            // When these prefs are not enabled, the error is different and
+            // that's why we still want to look at the isNavigating flag.
+            // We could add a workaround and make sure that in the alternative
+            // codepaths we would also omit the same error, but considering
+            // how we will be enabling fission by default soon, we can keep
+            // using isNavigating for now, and remove it when the
+            // parent-controlled pref and SHIP are enabled by default.
+            // Bug 1725716 has been filed to consider removing isNavigating
+            // field alltogether.
             let isNavigating = this.mBrowser.isNavigating;
-            if (this.mTab.selected && !isNavigating) {
+            if (
+              this.mTab.selected &&
+              aStatus != Cr.NS_BINDING_CANCELLED_OLD_LOAD &&
+              !isNavigating
+            ) {
               gURLBar.setURI();
             }
           } else if (isSuccessful) {

browser/components/urlbar/tests/browser/browser_stop_pending.js

@@ -218,3 +218,242 @@ add_task(async function() {
   BrowserTestUtils.removeTab(newTab);
   BrowserTestUtils.removeTab(tab);
 });
+
+/**
+ * 1) Try to load page 0 and wait for it to finish loading.
+ * 2) Try to load page 1 and wait for it to finish loading.
+ * 3) Try to load SLOW_PAGE, and then before it finishes loading, navigate back.
+ *    - We should be taken to page 0.
+ */
+add_task(async function testCorrectUrlBarAfterGoingBackDuringAnotherLoad() {
+  // Load example.org
+  let page0 = "http://example.org/";
+  let tab = await BrowserTestUtils.openNewForegroundTab(
+    gBrowser,
+    page0,
+    true,
+    true
+  );
+
+  // Load example.com in the same browser
+  let page1 = "http://example.com/";
+  let loaded = BrowserTestUtils.browserLoaded(tab.linkedBrowser, false, page1);
+  BrowserTestUtils.loadURI(tab.linkedBrowser, page1);
+  await loaded;
+
+  let initialValue = gURLBar.untrimmedValue;
+  let expectedURLBarChange = SLOW_PAGE;
+  let sawChange = false;
+  let goneBack = false;
+  let handler = () => {
+    if (!goneBack) {
+      isnot(
+        gURLBar.untrimmedValue,
+        initialValue,
+        `Should not revert URL bar value to ${initialValue}`
+      );
+    }
+
+    if (gURLBar.getAttribute("pageproxystate") == "valid") {
+      sawChange = true;
+      is(
+        gURLBar.untrimmedValue,
+        expectedURLBarChange,
+        `Should set expected URL bar value - ${expectedURLBarChange}`
+      );
+    }
+  };
+
+  let obs = new MutationObserver(handler);
+
+  obs.observe(gURLBar.textbox, { attributes: true });
+  // Set the value of url bar to SLOW_PAGE
+  gURLBar.value = SLOW_PAGE;
+  gURLBar.handleCommand();
+
+  // Copied from the first test case:
+  // If this ever starts going intermittent, we've broken this.
+  await new Promise(resolve => setTimeout(resolve, 200));
+
+  expectedURLBarChange = page0;
+  let pageLoadPromise = BrowserTestUtils.browserStopped(
+    tab.linkedBrowser,
+    page0
+  );
+
+  // Wait until we can go back
+  await TestUtils.waitForCondition(() => tab.linkedBrowser.canGoBack);
+  ok(tab.linkedBrowser.canGoBack, "can go back");
+
+  // Navigate back from SLOW_PAGE. We should be taken to page 0 now.
+  tab.linkedBrowser.goBack();
+  goneBack = true;
+  is(
+    gURLBar.untrimmedValue,
+    SLOW_PAGE,
+    "Should not have changed URL bar value synchronously."
+  );
+  // Wait until page 0 have finished loading.
+  await pageLoadPromise;
+  is(
+    gURLBar.untrimmedValue,
+    page0,
+    "Should not have changed URL bar value synchronously."
+  );
+  ok(
+    sawChange,
+    "The URL bar change handler should have been called by the time the page was loaded"
+  );
+  obs.disconnect();
+  obs = null;
+  BrowserTestUtils.removeTab(tab);
+});
+
+/**
+ * 1) Try to load page 1 and wait for it to finish loading.
+ * 2) Start loading SLOW_PAGE (it won't finish loading)
+ * 3) Reload the page. We should have loaded page 1 now.
+ */
+add_task(async function testCorrectUrlBarAfterReloadingDuringSlowPageLoad() {
+  // Load page 1 - example.com
+  let page1 = "http://example.com/";
+  let tab = await BrowserTestUtils.openNewForegroundTab(
+    gBrowser,
+    page1,
+    true,
+    true
+  );
+
+  let initialValue = gURLBar.untrimmedValue;
+  let expectedURLBarChange = SLOW_PAGE;
+  let sawChange = false;
+  let hasReloaded = false;
+  let handler = () => {
+    if (!hasReloaded) {
+      isnot(
+        gURLBar.untrimmedValue,
+        initialValue,
+        "Should not revert URL bar value!"
+      );
+    }
+    if (gURLBar.getAttribute("pageproxystate") == "valid") {
+      sawChange = true;
+      is(
+        gURLBar.untrimmedValue,
+        expectedURLBarChange,
+        "Should set expected URL bar value!"
+      );
+    }
+  };
+
+  let obs = new MutationObserver(handler);
+
+  obs.observe(gURLBar.textbox, { attributes: true });
+  // Start loading SLOW_PAGE
+  gURLBar.value = SLOW_PAGE;
+  gURLBar.handleCommand();
+
+  // Copied from the first test: If this ever starts going intermittent,
+  // we've broken this.
+  await new Promise(resolve => setTimeout(resolve, 200));
+
+  expectedURLBarChange = page1;
+  let pageLoadPromise = BrowserTestUtils.browserLoaded(
+    tab.linkedBrowser,
+    false,
+    page1
+  );
+  // Reload the page
+  tab.linkedBrowser.reload();
+  hasReloaded = true;
+  is(
+    gURLBar.untrimmedValue,
+    SLOW_PAGE,
+    "Should not have changed URL bar value synchronously."
+  );
+  // Wait for page1 to be loaded due to a reload while the slow page was still loading
+  await pageLoadPromise;
+  ok(
+    sawChange,
+    "The URL bar change handler should have been called by the time the page was loaded"
+  );
+  obs.disconnect();
+  obs = null;
+  BrowserTestUtils.removeTab(tab);
+});
+
+/**
+ * 1) Try to load example.com and wait for it to finish loading.
+ * 2) Start loading SLOW_PAGE and then stop the load before the load completes
+ * 3) Check that example.com has been loaded as a result of stopping SLOW_PAGE
+ *    load.
+ */
+add_task(async function testCorrectUrlBarAfterStoppingTheLoad() {
+  // Load page 1
+  let page1 = "http://example.com/";
+  let tab = await BrowserTestUtils.openNewForegroundTab(
+    gBrowser,
+    page1,
+    true,
+    true
+  );
+
+  let initialValue = gURLBar.untrimmedValue;
+  let expectedURLBarChange = SLOW_PAGE;
+  let sawChange = false;
+  let hasStopped = false;
+  let handler = () => {
+    if (!hasStopped) {
+      isnot(
+        gURLBar.untrimmedValue,
+        initialValue,
+        "Should not revert URL bar value!"
+      );
+    }
+    if (gURLBar.getAttribute("pageproxystate") == "valid") {
+      sawChange = true;
+      is(
+        gURLBar.untrimmedValue,
+        expectedURLBarChange,
+        "Should set expected URL bar value!"
+      );
+    }
+  };
+
+  let obs = new MutationObserver(handler);
+
+  obs.observe(gURLBar.textbox, { attributes: true });
+  // Start loading SLOW_PAGE
+  gURLBar.value = SLOW_PAGE;
+  gURLBar.handleCommand();
+
+  // Copied from the first test case:
+  // If this ever starts going intermittent, we've broken this.
+  await new Promise(resolve => setTimeout(resolve, 200));
+
+  // We expect page 1 to be loaded after the SLOW_PAGE load is stopped.
+  expectedURLBarChange = page1;
+  let pageLoadPromise = BrowserTestUtils.browserStopped(
+    tab.linkedBrowser,
+    SLOW_PAGE,
+    true
+  );
+  // Stop the SLOW_PAGE load
+  tab.linkedBrowser.stop();
+  hasStopped = true;
+  is(
+    gURLBar.untrimmedValue,
+    SLOW_PAGE,
+    "Should not have changed URL bar value synchronously."
+  );
+  // Wait for SLOW_PAGE load to stop
+  await pageLoadPromise;
+
+  ok(
+    sawChange,
+    "The URL bar change handler should have been called by the time the page was loaded"
+  );
+  obs.disconnect();
+  obs = null;
+  BrowserTestUtils.removeTab(tab);
+});

docshell/base/CanonicalBrowsingContext.cpp

@@ -1304,7 +1304,7 @@ void CanonicalBrowsingContext::GoBack(
 
   // Stop any known network loads if necessary.
   if (mCurrentLoad) {
-    mCurrentLoad->Cancel(NS_BINDING_ABORTED);
+    mCurrentLoad->Cancel(NS_BINDING_CANCELLED_OLD_LOAD);
   }
 
   if (nsDocShell* docShell = nsDocShell::Cast(GetDocShell())) {
@@ -1330,7 +1330,7 @@ void CanonicalBrowsingContext::GoForward(
 
   // Stop any known network loads if necessary.
   if (mCurrentLoad) {
-    mCurrentLoad->Cancel(NS_BINDING_ABORTED);
+    mCurrentLoad->Cancel(NS_BINDING_CANCELLED_OLD_LOAD);
   }
 
   if (auto* docShell = nsDocShell::Cast(GetDocShell())) {
@@ -1356,7 +1356,7 @@ void CanonicalBrowsingContext::GoToIndex(
 
   // Stop any known network loads if necessary.
   if (mCurrentLoad) {
-    mCurrentLoad->Cancel(NS_BINDING_ABORTED);
+    mCurrentLoad->Cancel(NS_BINDING_CANCELLED_OLD_LOAD);
   }
 
   if (auto* docShell = nsDocShell::Cast(GetDocShell())) {
@@ -1380,7 +1380,7 @@ void CanonicalBrowsingContext::Reload(uint32_t aReloadFlags) {
 
   // Stop any known network loads if necessary.
   if (mCurrentLoad) {
-    mCurrentLoad->Cancel(NS_BINDING_ABORTED);
+    mCurrentLoad->Cancel(NS_BINDING_CANCELLED_OLD_LOAD);
   }
 
   if (auto* docShell = nsDocShell::Cast(GetDocShell())) {
@@ -2095,7 +2095,7 @@ bool CanonicalBrowsingContext::StartDocumentLoad(
   // that we need to cancel any existing ones.
   if (StaticPrefs::browser_tabs_documentchannel_parent_controlled() &&
       mozilla::SessionHistoryInParent() && mCurrentLoad) {
-    mCurrentLoad->Cancel(NS_BINDING_ABORTED);
+    mCurrentLoad->Cancel(NS_BINDING_CANCELLED_OLD_LOAD);
   }
   mCurrentLoad = aLoad;
 

js/xpconnect/src/xpc.msg

@@ -250,3 +250,6 @@ XPC_MSG_DEF(NS_ERROR_SOCIALTRACKING_URI               , "The URI is social track
 
 /* Profile manager error codes */
 XPC_MSG_DEF(NS_ERROR_DATABASE_CHANGED                 , "Flushing the profiles to disk would have overwritten changes made elsewhere.")
+
+/* Codes related to URILoader */
+XPC_MSG_DEF(NS_BINDING_CANCELLED_OLD_LOAD           , "The async request has been cancelled by another async request")

xpcom/base/ErrorList.py

@@ -935,6 +935,11 @@ with modules["URILOADER"]:
     # doesn't need to be reparsed from the original source.
     errors["NS_ERROR_PARSED_DATA_CACHED"] = FAILURE(33)
 
+    # When browser.tabs.documentchannel.parent-controlled pref and SHIP
+    # are enabled and a load gets cancelled due to another one
+    # starting, the error is NS_BINDING_CANCELLED_OLD_LOAD.
+    errors["NS_BINDING_CANCELLED_OLD_LOAD"] = FAILURE(39)
+
 
 # =======================================================================
 # 25: NS_ERROR_MODULE_CONTENT