Bug 1000458 - stop races in location bar <return> handling code, r=mak
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Wed, 28 Sep 2016 19:54:25 +0100
changeset 316207 adb484f84dec4b9d6a216ef3f4e1887fc3ae8084
parent 316206 5083173624d5d3ef87034601a8acecbc83b3060e
child 316262 c8a660c5f105e60ad536ddde0c3edd637ab5b7c1
child 316263 1a4288cc3cead7e7026ea5e2594b6041e69dd13b
push id30765
push userphilringnalda@gmail.com
push dateTue, 04 Oct 2016 03:06:46 +0000
treeherdermozilla-central@adb484f84dec [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1000458
milestone52.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 1000458 - stop races in location bar <return> handling code, r=mak MozReview-Commit-ID: IcQCNj0FcCu
browser/base/content/test/urlbar/browser.ini
browser/base/content/test/urlbar/browser_urlbarRaceWithTabs.js
browser/base/content/urlbarBindings.xml
browser/base/content/utilityOverlay.js
testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
--- 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) {