Bug 1578856 - browser.fixup.dns_first_for_single_words and the keyword-uri-fixup UI are broken. r=adw

In the Quantum Bar it's usually the urlbar code that decides whether a search
string should be visited or searched. if dns_first_for_single_words is set,
we can't make a final decision, because that depends on a dns lookup. For now
we don't want to duplicate the docshell code, also because we must keep the
old behavior functioning for cases where the urlbar value is set without input.

Similarly, when the docshell decides to search for a single word host, and a
dns lookup resolves it, it also shows a prompt asking the user if he meant to
visit it instead of searching. Because the urlbar skips the docshell decision
making, we must manually call the fixup prompt code from the urlbar.

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

--HG--
extra : moz-landing-system : lando

browser/base/content/browser.js

@@ -1256,7 +1256,12 @@ XPCOMUtils.defineLazyPreferenceGetter(
 );
 
 function gKeywordURIFixup({ target: browser, data: fixupInfo }) {
-  let deserializeURI = spec => (spec ? makeURI(spec) : null);
+  let deserializeURI = url => {
+    if (url instanceof Ci.nsIURI) {
+      return url;
+    }
+    return url ? makeURI(url) : null;
+  };
 
   // We get called irrespective of whether we did a keyword search, or
   // whether the original input would be vaguely interpretable as a URL,

browser/components/urlbar/UrlbarInput.jsm

@@ -490,6 +490,7 @@ class UrlbarInput {
    * @param {Event} event The event that picked the result.
    */
   pickResult(result, event) {
+    let originalUntrimmedValue = this.untrimmedValue;
     let isCanonized = this.setValueFromResult(result, event);
     let where = this._whereToOpen(event);
     let openParams = {
@@ -517,6 +518,34 @@ class UrlbarInput {
     openParams.postData = postData;
 
     switch (result.type) {
+      case UrlbarUtils.RESULT_TYPE.URL: {
+        // Bug 1578856: both the provider and the docshell run heuristics to
+        // decide how to handle a non-url string, either fixing it to a url, or
+        // searching for it.
+        // Some preferences can control the docshell behavior, for example
+        // if dns_first_for_single_words is true, the docshell looks up the word
+        // against the dns server, and either loads it as an url or searches for
+        // it, depending on the lookup result. The provider instead will always
+        // return a fixed url in this case, because URIFixup is synchronous and
+        // can't do a synchronous dns lookup. A possible long term solution
+        // would involve sharing the docshell logic with the provider, along
+        // with the dns lookup.
+        // For now, in this specific case, we'll override the result's url
+        // with the input value, and let it pass through to _loadURL(), and
+        // finally to the docshell.
+        // This also means that in some cases the heuristic result will show a
+        // Visit entry, but the docshell will instead execute a search. It's a
+        // rare case anyway, most likely to happen for enterprises customizing
+        // the urifixup prefs.
+        if (
+          result.heuristic &&
+          UrlbarPrefs.get("browser.fixup.dns_first_for_single_words") &&
+          UrlbarUtils.looksLikeSingleWordHost(originalUntrimmedValue)
+        ) {
+          url = originalUntrimmedValue;
+        }
+        break;
+      }
       case UrlbarUtils.RESULT_TYPE.KEYWORD: {
         // If this result comes from a bookmark keyword, let it inherit the
         // current document's principal, otherwise bookmarklets would break.
@@ -573,6 +602,37 @@ class UrlbarInput {
           this.startQuery();
           return;
         }
+
+        if (
+          result.heuristic &&
+          UrlbarUtils.looksLikeSingleWordHost(originalUntrimmedValue)
+        ) {
+          // The docshell when fixes a single word to a search, also checks the
+          // dns and prompts the user whether they wanted to rather visit that
+          // as a host. On a positive answer, it adds to the domains whitelist
+          // that we use to make decisions. Because here we are directly asking
+          // for a search, bypassing the docshell, we must do it here.
+          // See URIFixupChild.jsm and keyword-uri-fixup.
+          let flags =
+            Ci.nsIURIFixup.FIXUP_FLAG_FIX_SCHEME_TYPOS |
+            Ci.nsIURIFixup.FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP;
+          // Don't interrupt the load action in case of errors.
+          try {
+            let fixupInfo = Services.uriFixup.getFixupURIInfo(
+              originalUntrimmedValue.trim(),
+              flags
+            );
+            this.window.gKeywordURIFixup({
+              target: this.window.gBrowser.selectedBrowser,
+              data: fixupInfo,
+            });
+          } catch (ex) {
+            Cu.reportError(
+              `An error occured while trying to fixup "${originalUntrimmedValue.trim()}": ${ex}`
+            );
+          }
+        }
+
         const actionDetails = {
           isSuggestion: !!result.payload.suggestion,
           alias: result.payload.keyword,

browser/components/urlbar/UrlbarPrefs.jsm

@@ -149,6 +149,7 @@ const PREF_OTHER_DEFAULTS = new Map([
   ["keyword.enabled", true],
   ["browser.search.suggest.enabled", true],
   ["ui.popup.disable_autohide", false],
+  ["browser.fixup.dns_first_for_single_words", false],
 ]);
 
 // Maps preferences under browser.urlbar.suggest to behavior names, as defined

browser/components/urlbar/UrlbarUtils.jsm

@@ -158,6 +158,10 @@ var UrlbarUtils = {
   // unit separator.
   TITLE_TAGS_SEPARATOR: "\x1F",
 
+  // Regex matching single words (no spaces, dots or url-like chars).
+  // We accept a trailing dot though.
+  REGEXP_SINGLE_WORD: /^[^\s.?@:/]+\.?$/,
+
   /**
    * Adds a url to history as long as it isn't in a private browsing window,
    * and it is valid.
@@ -462,6 +466,19 @@ var UrlbarUtils = {
         event.inputType == "insertFromYank")
     );
   },
+
+  /**
+   * Given a string, checks if it looks like a single word host, not containing
+   * spaces nor dots (apart from a possible trailing one).
+   * @note This matching should stay in sync with the related code in
+   * nsDefaultURIFixup::KeywordURIFixup
+   * @param {string} value
+   * @returns {boolean} Whether the value looks like a single word host.
+   */
+  looksLikeSingleWordHost(value) {
+    let str = value.trim();
+    return this.REGEXP_SINGLE_WORD.test(str);
+  },
 };
 
 XPCOMUtils.defineLazyGetter(UrlbarUtils.ICON, "DEFAULT", () => {

browser/components/urlbar/tests/browser/browser.ini

@@ -42,6 +42,7 @@ skip-if = fission
 [browser_caret_navigation.js]
 [browser_customizeMode.js]
 [browser_deleteAllText.js]
+[browser_dns_first_for_single_words.js]
 [browser_doubleClickSelectsAll.js]
 [browser_downArrowKeySearch.js]
 [browser_dragdropURL.js]

browser/components/urlbar/tests/browser/browser_dns_first_for_single_words.js

@@ -0,0 +1,40 @@
+/* Any copyright is dedicated to the Public Domain.
+ * https://creativecommons.org/publicdomain/zero/1.0/ */
+
+// Checks that if browser.fixup.dns_first_for_single_words pref is set, we pass
+// the original search string to the docshell and not a search url.
+
+add_task(async function test() {
+  const { sinon } = ChromeUtils.import("resource://testing-common/Sinon.jsm");
+  const sandbox = sinon.createSandbox();
+
+  const PREF = "browser.fixup.dns_first_for_single_words";
+  Services.prefs.setBoolPref(PREF, true);
+
+  registerCleanupFunction(function() {
+    Services.prefs.clearUserPref(PREF);
+    sandbox.restore();
+  });
+
+  async function testVal(str, passthrough) {
+    sandbox.stub(gURLBar, "_loadURL").callsFake(url => {
+      if (passthrough) {
+        Assert.equal(url, str, "Should pass the unmodified search string");
+      } else {
+        Assert.ok(url.startsWith("http"), "Should pass an url");
+      }
+    });
+    await promiseAutocompleteResultPopup(str);
+    EventUtils.synthesizeKey("KEY_Enter");
+    sandbox.restore();
+  }
+
+  await testVal("test", true);
+  await testVal("te-st", true);
+  await testVal("test ", true);
+  await testVal(" test", true);
+  await testVal(" test", true);
+  await testVal("test test", false);
+  await testVal("test.test", false);
+  await testVal("test/test", false);
+});

browser/components/urlbar/tests/browser/browser_urlbarSearchSingleWordNotification.js

@@ -39,33 +39,70 @@ async function runURLBarSearchTest({
   expectNotification,
   aWindow = window,
 }) {
-  aWindow.gURLBar.value = valueToOpen;
-  let expectedURI;
-  if (!expectSearch) {
-    expectedURI = "http://" + valueToOpen + "/";
-  } else {
-    expectedURI = (await Services.search.getDefault()).getSubmission(
-      valueToOpen,
-      null,
-      "keyword"
-    ).uri.spec;
-  }
-  aWindow.gURLBar.focus();
-  let docLoadPromise = BrowserTestUtils.waitForDocLoadAndStopIt(
-    expectedURI,
-    aWindow.gBrowser.selectedBrowser
-  );
-  EventUtils.synthesizeKey("VK_RETURN", {}, aWindow);
+  // Test both directly setting a value and pressing enter, or setting the
+  // value through input events, like the user would do.
+  const setValueFns = [
+    value => {
+      aWindow.gURLBar.value = value;
+    },
+    value => {
+      return UrlbarTestUtils.promiseAutocompleteResultPopup({
+        window,
+        waitForFocus,
+        value,
+      });
+    },
+  ];
 
-  await Promise.all([
-    docLoadPromise,
-    promiseNotification(
-      aWindow.gBrowser,
-      "keyword-uri-fixup",
-      expectNotification,
-      valueToOpen
-    ),
-  ]);
+  for (let i = 0; i < setValueFns.length; ++i) {
+    await setValueFns[i](valueToOpen);
+    let expectedURI;
+    if (!expectSearch) {
+      expectedURI = "http://" + valueToOpen + "/";
+    } else {
+      expectedURI = (await Services.search.getDefault()).getSubmission(
+        valueToOpen,
+        null,
+        "keyword"
+      ).uri.spec;
+    }
+    aWindow.gURLBar.focus();
+    let docLoadPromise = BrowserTestUtils.waitForDocLoadAndStopIt(
+      expectedURI,
+      aWindow.gBrowser.selectedBrowser
+    );
+    EventUtils.synthesizeKey("VK_RETURN", {}, aWindow);
+
+    await Promise.all([
+      docLoadPromise,
+      promiseNotification(
+        aWindow.gBrowser,
+        "keyword-uri-fixup",
+        expectNotification,
+        valueToOpen
+      ),
+    ]);
+
+    if (expectNotification) {
+      let notificationBox = aWindow.gBrowser.getNotificationBox(
+        aWindow.gBrowser.selectedBrowser
+      );
+      let notification = notificationBox.getNotificationWithValue(
+        "keyword-uri-fixup"
+      );
+      // Confirm the notification only on the last loop.
+      if (i == setValueFns.length - 1) {
+        docLoadPromise = BrowserTestUtils.waitForDocLoadAndStopIt(
+          "http://" + valueToOpen + "/",
+          aWindow.gBrowser.selectedBrowser
+        );
+        notification.querySelector("button").click();
+        await docLoadPromise;
+      } else {
+        notificationBox.currentNotification.close();
+      }
+    }
+  }
 }
 
 add_task(async function test_navigate_full_domain() {
@@ -75,7 +112,7 @@ add_task(async function test_navigate_full_domain() {
   ));
   await BrowserTestUtils.browserLoaded(tab.linkedBrowser);
   await runURLBarSearchTest({
-    valueToOpen: "www.mozilla.org",
+    valueToOpen: "www.singlewordtest.org",
     expectSearch: false,
     expectNotification: false,
   });
@@ -166,45 +203,44 @@ function get_test_function_for_localhost_with_hostname(hostName, isPrivate) {
     } else {
       win = window;
     }
-    let browser = win.gBrowser;
-    let tab = await BrowserTestUtils.openNewForegroundTab(browser);
 
+    // Remove the domain from the whitelist.
     Services.prefs.setBoolPref(pref, false);
-    await runURLBarSearchTest({
-      valueToOpen: hostName,
-      expectSearch: true,
-      expectNotification: true,
-      aWindow: win,
-    });
 
-    let notificationBox = browser.getNotificationBox(tab.linkedBrowser);
-    let notification = notificationBox.getNotificationWithValue(
-      "keyword-uri-fixup"
+    await BrowserTestUtils.withNewTab(
+      {
+        gBrowser: win.gBrowser,
+        url: "about:blank",
+      },
+      browser =>
+        runURLBarSearchTest({
+          valueToOpen: hostName,
+          expectSearch: true,
+          expectNotification: true,
+          aWindow: win,
+        })
     );
-    let docLoadPromise = BrowserTestUtils.waitForDocLoadAndStopIt(
-      "http://" + hostName + "/",
-      tab.linkedBrowser
-    );
-    notification.querySelector("button").click();
 
     // check pref value
     let prefValue = Services.prefs.getBoolPref(pref);
     is(prefValue, !isPrivate, "Pref should have the correct state.");
 
-    await docLoadPromise;
-    browser.removeTab(tab);
-
     // Now try again with the pref set.
-    tab = browser.selectedTab = BrowserTestUtils.addTab(browser, "about:blank");
-    await BrowserTestUtils.browserLoaded(tab.linkedBrowser);
     // In a private window, the notification should appear again.
-    await runURLBarSearchTest({
-      valueToOpen: hostName,
-      expectSearch: isPrivate,
-      expectNotification: isPrivate,
-      aWindow: win,
-    });
-    browser.removeTab(tab);
+    await BrowserTestUtils.withNewTab(
+      {
+        gBrowser: win.gBrowser,
+        url: "about:blank",
+      },
+      browser =>
+        runURLBarSearchTest({
+          valueToOpen: hostName,
+          expectSearch: isPrivate,
+          expectNotification: isPrivate,
+          aWindow: win,
+        })
+    );
+
     if (isPrivate) {
       info("Waiting for private window to close");
       await BrowserTestUtils.closeWindow(win);

docshell/test/browser/browser_uriFixupAlternateRedirects.js

@@ -1,44 +1,65 @@
 "use strict";
 
+const { UrlbarTestUtils } = ChromeUtils.import(
+  "resource://testing-common/UrlbarTestUtils.jsm"
+);
+
 const REDIRECTURL =
   "http://www.example.com/browser/docshell/test/browser/redirect_to_example.sjs";
 
 add_task(async function() {
-  let tab = await BrowserTestUtils.openNewForegroundTab(
-    gBrowser,
-    "about:blank"
-  );
-  gURLBar.value = REDIRECTURL;
-  gURLBar.select();
-  let errorPageLoaded = BrowserTestUtils.waitForErrorPage(tab.linkedBrowser);
-  EventUtils.sendKey("return");
-  await errorPageLoaded;
-  let [contentURL, originalURL] = await ContentTask.spawn(
-    tab.linkedBrowser,
-    null,
-    () => {
-      return [
-        content.document.documentURI,
-        content.document.mozDocumentURIIfNotForErrorPages.spec,
-      ];
-    }
-  );
-  info("Page that loaded: " + contentURL);
-  const errorURI = "about:neterror?";
-  ok(contentURL.startsWith(errorURI), "Should be on an error page");
+  // Test both directly setting a value and pressing enter, or setting the
+  // value through input events, like the user would do.
+  const setValueFns = [
+    value => {
+      gURLBar.value = value;
+    },
+    value => {
+      return UrlbarTestUtils.promiseAutocompleteResultPopup({
+        window,
+        waitForFocus,
+        value,
+      });
+    },
+  ];
+  for (let setValueFn of setValueFns) {
+    let tab = await BrowserTestUtils.openNewForegroundTab(
+      gBrowser,
+      "about:blank"
+    );
+    // Enter search terms and start a search.
+    gURLBar.focus();
+    await setValueFn(REDIRECTURL);
+    let errorPageLoaded = BrowserTestUtils.waitForErrorPage(tab.linkedBrowser);
+    EventUtils.synthesizeKey("KEY_Enter");
+    await errorPageLoaded;
+    let [contentURL, originalURL] = await ContentTask.spawn(
+      tab.linkedBrowser,
+      null,
+      () => {
+        return [
+          content.document.documentURI,
+          content.document.mozDocumentURIIfNotForErrorPages.spec,
+        ];
+      }
+    );
+    info("Page that loaded: " + contentURL);
+    const errorURI = "about:neterror?";
+    ok(contentURL.startsWith(errorURI), "Should be on an error page");
 
-  const contentPrincipal = tab.linkedBrowser.contentPrincipal;
-  ok(
-    contentPrincipal.URI.spec.startsWith(errorURI),
-    "Principal should be for the error page"
-  );
+    const contentPrincipal = tab.linkedBrowser.contentPrincipal;
+    ok(
+      contentPrincipal.URI.spec.startsWith(errorURI),
+      "Principal should be for the error page"
+    );
 
-  originalURL = new URL(originalURL);
-  is(
-    originalURL.host,
-    "example",
-    "Should be an error for http://example, not http://www.example.com/"
-  );
+    originalURL = new URL(originalURL);
+    is(
+      originalURL.host,
+      "example",
+      "Should be an error for http://example, not http://www.example.com/"
+    );
 
-  BrowserTestUtils.removeTab(tab);
+    BrowserTestUtils.removeTab(tab);
+  }
 });

docshell/test/browser/browser_uriFixupIntegration.js

@@ -3,6 +3,10 @@
 
 "use strict";
 
+const { UrlbarTestUtils } = ChromeUtils.import(
+  "resource://testing-common/UrlbarTestUtils.jsm"
+);
+
 const kSearchEngineID = "browser_urifixup_search_engine";
 const kSearchEngineURL = "http://example.com/?search={searchTerms}";
 
@@ -32,27 +36,48 @@ add_task(async function setup() {
 });
 
 add_task(async function test() {
+  // Test both directly setting a value and pressing enter, or setting the
+  // value through input events, like the user would do.
+  const setValueFns = [
+    value => {
+      gURLBar.value = value;
+    },
+    value => {
+      return UrlbarTestUtils.promiseAutocompleteResultPopup({
+        window,
+        waitForFocus,
+        value,
+      });
+    },
+  ];
+
   for (let searchParams of ["foo bar", "brokenprotocol:somethingelse"]) {
-    // Add a new blank tab.
-    gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser, "about:blank");
-    await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
+    for (let setValueFn of setValueFns) {
+      // Add a new blank tab.
+      gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser, "about:blank");
+      await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
 
-    // Enter search terms and start a search.
-    gURLBar.value = searchParams;
-    gURLBar.focus();
-    EventUtils.synthesizeKey("KEY_Enter");
-    await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
+      // Enter search terms and start a search.
+      gURLBar.focus();
+      await setValueFn(searchParams);
 
-    // Check that we arrived at the correct URL.
-    let escapedParams = encodeURIComponent(searchParams).replace("%20", "+");
-    let expectedURL = kSearchEngineURL.replace("{searchTerms}", escapedParams);
-    is(
-      gBrowser.selectedBrowser.currentURI.spec,
-      expectedURL,
-      "New tab should have loaded with expected url."
-    );
+      EventUtils.synthesizeKey("KEY_Enter");
+      await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
 
-    // Cleanup.
-    gBrowser.removeCurrentTab();
+      // Check that we arrived at the correct URL.
+      let escapedParams = encodeURIComponent(searchParams).replace("%20", "+");
+      let expectedURL = kSearchEngineURL.replace(
+        "{searchTerms}",
+        escapedParams
+      );
+      is(
+        gBrowser.selectedBrowser.currentURI.spec,
+        expectedURL,
+        "New tab should have loaded with expected url."
+      );
+
+      // Cleanup.
+      gBrowser.removeCurrentTab();
+    }
   }
 });