From 81d0b361751493d783fce7ff2e6d71734c54e63f Mon Sep 17 00:00:00 2001 From: Anny Gakhokidze Date: Thu, 16 Dec 2021 22:27:17 +0000 Subject: [PATCH] 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 | 19 +- .../tests/browser/browser_stop_pending.js | 239 ++++++++++++++++++ docshell/base/CanonicalBrowsingContext.cpp | 10 +- js/xpconnect/src/xpc.msg | 3 + xpcom/base/ErrorList.py | 5 + 5 files changed, 269 insertions(+), 7 deletions(-) diff --git a/browser/base/content/tabbrowser.js b/browser/base/content/tabbrowser.js index 8c6c4d382e58..2065617eb4c3 100644 --- a/browser/base/content/tabbrowser.js +++ b/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) { diff --git a/browser/components/urlbar/tests/browser/browser_stop_pending.js b/browser/components/urlbar/tests/browser/browser_stop_pending.js index 3b9c509f95ed..9d6aabda38fb 100644 --- a/browser/components/urlbar/tests/browser/browser_stop_pending.js +++ b/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); +}); diff --git a/docshell/base/CanonicalBrowsingContext.cpp b/docshell/base/CanonicalBrowsingContext.cpp index 8ae1e3a02a00..79ce21c42d83 100644 --- a/docshell/base/CanonicalBrowsingContext.cpp +++ b/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; diff --git a/js/xpconnect/src/xpc.msg b/js/xpconnect/src/xpc.msg index 3e90f43e8821..0c59109f2fb0 100644 --- a/js/xpconnect/src/xpc.msg +++ b/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") diff --git a/xpcom/base/ErrorList.py b/xpcom/base/ErrorList.py index 6341a3d6930b..00bd294a2b31 100755 --- a/xpcom/base/ErrorList.py +++ b/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