Bug 1000458 - stop races in location bar <return> handling code, r?mak
MozReview-Commit-ID: IcQCNj0FcCu
--- a/browser/base/content/test/urlbar/browser.ini
+++ b/browser/base/content/test/urlbar/browser.ini
@@ -54,16 +54,17 @@ support-files =
[browser_urlbarDelete.js]
[browser_urlbarEnter.js]
[browser_urlbarEnterAfterMouseOver.js]
skip-if = os == "linux" # Bug 1073339 - Investigate autocomplete test unreliability on Linux/e10s
[browser_urlbarHashChangeProxyState.js]
[browser_urlbarKeepStateAcrossTabSwitches.js]
[browser_urlbarOneOffs.js]
[browser_urlbarPrivateBrowsingWindowChange.js]
+[browser_urlbarRaceWithTabs.js]
[browser_urlbarRevert.js]
[browser_urlbarSearchSingleWordNotification.js]
[browser_urlbarSearchSuggestions.js]
support-files =
searchSuggestionEngine.xml
searchSuggestionEngine.sjs
[browser_urlbarSearchSuggestionsNotification.js]
support-files =
new file mode 100644
--- /dev/null
+++ b/browser/base/content/test/urlbar/browser_urlbarRaceWithTabs.js
@@ -0,0 +1,57 @@
+const kURL = "http://example.org/browser/browser/base/content/test/urlbar/dummy_page.html";
+
+function* addBookmark(bookmark) {
+ if (bookmark.keyword) {
+ yield PlacesUtils.keywords.insert({
+ keyword: bookmark.keyword,
+ url: bookmark.url,
+ });
+ }
+
+ let bm = yield PlacesUtils.bookmarks.insert({
+ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+ url: bookmark.url,
+ title: bookmark.title,
+ });
+
+ registerCleanupFunction(function* () {
+ yield PlacesUtils.bookmarks.remove(bm);
+ if (bookmark.keyword) {
+ yield PlacesUtils.keywords.remove(bookmark.keyword);
+ }
+ });
+}
+
+/**
+ * Check that if the user hits enter and ctrl-t at the same time, we open the URL in the right tab.
+ */
+add_task(function* hitEnterLoadInRightTab() {
+ info("Opening new tab");
+ let oldTabCreatedPromise = BrowserTestUtils.waitForEvent(gBrowser.tabContainer, "TabOpen");
+ BrowserOpenTab();
+ let oldTab = (yield oldTabCreatedPromise).target;
+ let oldTabLoadedPromise = BrowserTestUtils.browserLoaded(oldTab.linkedBrowser, false, kURL);
+ oldTabLoadedPromise.then(() => info("Old tab loaded"));
+ let newTabCreatedPromise = BrowserTestUtils.waitForEvent(gBrowser.tabContainer, "TabOpen");
+
+ info("Creating bookmark and keyword");
+ yield addBookmark({title: "Test for keyword bookmark and URL", url: kURL, keyword: "urlbarkeyword"});
+ info("Filling URL bar, sending <return> and opening a tab");
+ gURLBar.value = "urlbarkeyword";
+ gURLBar.select();
+ EventUtils.sendKey("return");
+ BrowserOpenTab();
+ info("Waiting for new tab");
+ let newTab = (yield newTabCreatedPromise).target;
+ info("Created new tab; waiting for either tab to load");
+ let newTabLoadedPromise = BrowserTestUtils.browserLoaded(newTab.linkedBrowser, false, kURL);
+ newTabLoadedPromise.then(() => info("New tab loaded"));
+ yield Promise.race([newTabLoadedPromise, oldTabLoadedPromise]);
+ is(newTab.linkedBrowser.currentURI.spec, "about:newtab", "New tab still has about:newtab");
+ is(oldTab.linkedBrowser.currentURI.spec, kURL, "Old tab loaded URL");
+ info("Closing new tab");
+ yield BrowserTestUtils.removeTab(newTab);
+ info("Closing old tab");
+ yield BrowserTestUtils.removeTab(oldTab);
+ info("Finished");
+});
--- a/browser/base/content/urlbarBindings.xml
+++ b/browser/base/content/urlbarBindings.xml
@@ -394,16 +394,17 @@ file, You can obtain one at http://mozil
let url = this.value;
if (!url) {
return;
}
let mayInheritPrincipal = false;
let postData = null;
+ let browser = gBrowser.selectedBrowser;
let lastLocationChange = gBrowser.selectedBrowser.lastLocationChange;
let matchLastLocationChange = true;
let action = this._parseActionUrl(url);
if (action) {
switch (action.type) {
case "visiturl":
case "keyword":
@@ -430,17 +431,17 @@ file, You can obtain one at http://mozil
action.params.engineName,
action.params.searchSuggestion || action.params.searchQuery,
event,
where,
openUILinkParams
);
break;
}
- this._loadURL(url, postData, where, openUILinkParams,
+ this._loadURL(url, browser, postData, where, openUILinkParams,
matchLastLocationChange, mayInheritPrincipal);
return;
}
// If there's a selected one-off button and the input value is a
// search query (or "keyword" in URI-fixup terminology), then load a
// search using the one-off's engine.
if (selectedOneOff && selectedOneOff.engine) {
@@ -451,28 +452,28 @@ file, You can obtain one at http://mozil
value,
Services.uriFixup.FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP
);
} catch (ex) {}
if (fixup && fixup.keywordProviderName) {
[url, postData] =
this._recordSearchEngineLoad(selectedOneOff.engine, value,
event, where, openUILinkParams);
- this._loadURL(url, postData, where, openUILinkParams,
+ this._loadURL(url, browser, postData, where, openUILinkParams,
matchLastLocationChange, mayInheritPrincipal);
return;
}
}
url = this._canonizeURL(event, url);
getShortcutOrURIAndPostData(url).then(({url, postData, mayInheritPrincipal}) => {
if (url) {
matchLastLocationChange =
- lastLocationChange == gBrowser.selectedBrowser.lastLocationChange;
- this._loadURL(url, postData, where, openUILinkParams,
+ lastLocationChange == browser.lastLocationChange;
+ this._loadURL(url, browser, postData, where, openUILinkParams,
matchLastLocationChange, mayInheritPrincipal);
}
});
]]></body>
</method>
<property name="oneOffSearchQuery">
<getter><![CDATA[
@@ -482,38 +483,40 @@ file, You can obtain one at http://mozil
return (this.handleEnterInstance && this.handleEnterInstance.searchString) ||
this.mController.searchString ||
this.textValue;
]]></getter>
</property>
<method name="_loadURL">
<parameter name="url"/>
+ <parameter name="browser"/>
<parameter name="postData"/>
<parameter name="openUILinkWhere"/>
<parameter name="openUILinkParams"/>
<parameter name="matchLastLocationChange"/>
<parameter name="mayInheritPrincipal"/>
<body><![CDATA[
this.value = url;
- gBrowser.userTypedValue = url;
+ browser.userTypedValue = url;
if (gInitialPages.includes(url)) {
- gBrowser.selectedBrowser.initialPageLoadedFromURLBar = url;
+ browser.initialPageLoadedFromURLBar = url;
}
try {
addToUrlbarHistory(url);
} catch (ex) {
// Things may go wrong when adding url to session history,
// but don't let that interfere with the loading of the url.
Cu.reportError(ex);
}
let params = {
postData: postData,
allowThirdPartyFixup: true,
+ currentBrowser: browser,
};
if (openUILinkWhere == "current") {
params.indicateErrorPageLoad = true;
params.allowPinnedTabHostChange = true;
params.disallowInheritPrincipal = !mayInheritPrincipal;
params.allowPopups = url.startsWith("javascript:");
} else {
params.initiatingDoc = document;
@@ -523,17 +526,17 @@ file, You can obtain one at http://mozil
for (let key in openUILinkParams) {
params[key] = openUILinkParams[key];
}
}
// Focus the content area before triggering loads, since if the load
// occurs in a new tab, we want focus to be restored to the content
// area when the current tab is re-selected.
- gBrowser.selectedBrowser.focus();
+ browser.focus();
if (openUILinkWhere == "current" && !matchLastLocationChange) {
return;
}
if (openUILinkWhere != "current") {
this.handleRevert();
}
--- a/browser/base/content/utilityOverlay.js
+++ b/browser/base/content/utilityOverlay.js
@@ -221,16 +221,29 @@ function openLinkIn(url, where, params)
var aNoReferrer = params.noReferrer;
var aAllowPopups = !!params.allowPopups;
var aUserContextId = params.userContextId;
var aIndicateErrorPageLoad = params.indicateErrorPageLoad;
var aPrincipal = params.originPrincipal;
var aForceAboutBlankViewerInCurrent =
params.forceAboutBlankViewerInCurrent;
+ // Establish a window in which we're running this code.
+ var w = getTopWin();
+
+ if ((where == "tab" || where == "tabshifted") &&
+ w && !w.toolbar.visible) {
+ w = getTopWin(true);
+ aRelatedToCurrent = false;
+ }
+
+ // Can only do this after we're sure of what |w| will be the rest of this function.
+ // Note that if |w| is null we might have no current browser (we'll open a new window).
+ var aCurrentBrowser = params.currentBrowser || (w && w.gBrowser.selectedBrowser);
+
if (where == "save") {
// TODO(1073187): propagate referrerPolicy.
// ContentClick.jsm passes isContentWindowPrivate for saveURL instead of passing a CPOW initiatingDoc
if ("isContentWindowPrivate" in params) {
saveURL(url, null, null, true, true, aNoReferrer ? null : aReferrerURI, null, params.isContentWindowPrivate);
}
else {
@@ -239,23 +252,16 @@ function openLinkIn(url, where, params)
"where == 'save' but without initiatingDoc. See bug 814264.");
return;
}
saveURL(url, null, null, true, true, aNoReferrer ? null : aReferrerURI, aInitiatingDoc);
}
return;
}
- var w = getTopWin();
- if ((where == "tab" || where == "tabshifted") &&
- w && !w.toolbar.visible) {
- w = getTopWin(true);
- aRelatedToCurrent = false;
- }
-
if (!w || where == "window") {
// This propagates to window.arguments.
var sa = Cc["@mozilla.org/supports-array;1"].
createInstance(Ci.nsISupportsArray);
var wuri = Cc["@mozilla.org/supports-string;1"].
createInstance(Ci.nsISupportsString);
wuri.data = url;
@@ -313,22 +319,27 @@ function openLinkIn(url, where, params)
let uriObj;
if (where == "current") {
try {
uriObj = Services.io.newURI(url, null, null);
} catch (e) {}
}
- if (where == "current" && w.gBrowser.selectedTab.pinned &&
+ // NB: we avoid using |w| here because in the 'popup window' case,
+ // if we pass a currentBrowser param |w.gBrowser| might not be the
+ // tabbrowser that contains |aCurrentBrowser|. We really only care
+ // about the tab linked to |aCurrentBrowser|.
+ let tab = aCurrentBrowser.ownerGlobal.gBrowser.getTabForBrowser(aCurrentBrowser);
+ if (where == "current" && tab.pinned &&
!aAllowPinnedTabHostChange) {
try {
// nsIURI.host can throw for non-nsStandardURL nsIURIs.
if (!uriObj || (!uriObj.schemeIs("javascript") &&
- w.gBrowser.currentURI.host != uriObj.host)) {
+ aCurrentBrowser.currentURI.host != uriObj.host)) {
where = "tab";
loadInBackground = false;
}
} catch (err) {
where = "tab";
loadInBackground = false;
}
}
@@ -360,17 +371,17 @@ function openLinkIn(url, where, params)
if (aIndicateErrorPageLoad) {
flags |= Ci.nsIWebNavigation.LOAD_FLAGS_ERROR_LOAD_CHANGES_RV;
}
if (aForceAboutBlankViewerInCurrent) {
w.gBrowser.selectedBrowser.createAboutBlankContentViewer(aPrincipal);
}
- w.gBrowser.loadURIWithFlags(url, {
+ aCurrentBrowser.loadURIWithFlags(url, {
flags: flags,
referrerURI: aNoReferrer ? null : aReferrerURI,
referrerPolicy: aReferrerPolicy,
postData: aPostData,
userContextId: aUserContextId
});
break;
case "tabshifted":
@@ -389,17 +400,17 @@ function openLinkIn(url, where, params)
allowMixedContent: aAllowMixedContent,
noReferrer: aNoReferrer,
userContextId: aUserContextId,
originPrincipal: aPrincipal,
});
break;
}
- w.gBrowser.selectedBrowser.focus();
+ aCurrentBrowser.focus();
if (!loadInBackground && w.isBlankPageURL(url)) {
w.focusAndSelectUrlBar();
}
}
// Used as an onclick handler for UI elements with link-like behavior.
// e.g. onclick="checkForMiddleClick(this, event);"
--- a/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
+++ b/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
@@ -268,16 +268,19 @@ this.BrowserTestUtils = {
*
* @param {tabbrowser} tabbrowser
* The tabbrowser to look for the next new tab in.
* @param {string} url
* A string URL to look for in the new tab. If null, allows any non-blank URL.
*
* @return {Promise}
* @resolves With the {xul:tab} when a tab is opened and its location changes to the given URL.
+ *
+ * NB: this method will not work if you open a new tab with e.g. BrowserOpenTab
+ * and the tab does not load a URL, because no onLocationChange will fire.
*/
waitForNewTab(tabbrowser, url) {
return new Promise((resolve, reject) => {
tabbrowser.tabContainer.addEventListener("TabOpen", function onTabOpen(openEvent) {
tabbrowser.tabContainer.removeEventListener("TabOpen", onTabOpen);
let progressListener = {
onLocationChange(aBrowser) {