Bug 1000458 - stop races in location bar <return> handling code, r?mak draft
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Wed, 28 Sep 2016 19:54:25 +0100
changeset 419934 3188259ceffdce54b877b7b216ee065c75dda54c
parent 419507 5ffed033557e5b6f9694123f1948f867f913ede3
child 532680 0b5fd9ad2fb85f4ecb9239bd7d1f199616d601e9
push id31045
push usergijskruitbosch@gmail.com
push dateSun, 02 Oct 2016 15:59:36 +0000
reviewersmak
bugs1000458
milestone52.0a1
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) {