Bug 1635033 - Set an untrimmed value when an invalid url is picked from address bar results. r=dao

Trimming would convert a url with an invalid origin into a string like
"origin/something" and if the user keeps editing it, we'd end up converting the
url into a search.

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

browser/components/urlbar/UrlbarInput.jsm

@@ -667,26 +667,13 @@ class UrlbarInput {
           // we use to make decisions. Because we are directly asking for a
           // search here, bypassing the docShell, we need invoke the same check
           // ourselves. See also URIFixupChild.jsm and keyword-uri-fixup.
-          let flags =
-            Ci.nsIURIFixup.FIXUP_FLAG_FIX_SCHEME_TYPOS |
-            Ci.nsIURIFixup.FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP;
-          if (this.isPrivate) {
-            flags |= Ci.nsIURIFixup.FIXUP_FLAG_PRIVATE_CONTEXT;
-          }
           // Don't interrupt the load action in case of errors.
-          try {
-            let fixupInfo = Services.uriFixup.getFixupURIInfo(
-              originalUntrimmedValue.trim(),
-              flags
-            );
+          let fixupInfo = this._getURIFixupInfo(originalUntrimmedValue.trim());
+          if (fixupInfo) {
             this.window.gKeywordURIFixup.check(
               this.window.gBrowser.selectedBrowser,
               fixupInfo
             );
-          } catch (ex) {
-            Cu.reportError(
-              `An error occured while trying to fixup "${originalUntrimmedValue.trim()}": ${ex}`
-            );
           }
         }
 
@@ -848,7 +835,23 @@ class UrlbarInput {
         let { value, selectionStart, selectionEnd } = result.autofill;
         this._autofillValue(value, selectionStart, selectionEnd);
       } else {
-        this.value = this._getValueFromResult(result);
+        // If the url is trimmed but it's invalid (for example it has a non
+        // whitelisted single word host, or an unknown domain suffix), trimming
+        // it would end up executing a search instead of visiting it.
+        let allowTrim = true;
+        if (
+          result.type == UrlbarUtils.RESULT_TYPE.URL &&
+          UrlbarPrefs.get("trimURLs") &&
+          result.payload.url.startsWith(BrowserUtils.trimURLProtocol)
+        ) {
+          let fixupInfo = this._getURIFixupInfo(
+            BrowserUtils.trimURL(result.payload.url)
+          );
+          if (fixupInfo?.keywordAsSent) {
+            allowTrim = false;
+          }
+        }
+        this._setValue(this._getValueFromResult(result), allowTrim);
       }
     }
     this._resultForCurrentValue = result;
@@ -1210,6 +1213,23 @@ class UrlbarInput {
 
   // Private methods below.
 
+  _getURIFixupInfo(searchString) {
+    let flags =
+      Ci.nsIURIFixup.FIXUP_FLAG_FIX_SCHEME_TYPOS |
+      Ci.nsIURIFixup.FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP;
+    if (this.isPrivate) {
+      flags |= Ci.nsIURIFixup.FIXUP_FLAG_PRIVATE_CONTEXT;
+    }
+    try {
+      return Services.uriFixup.getFixupURIInfo(searchString, flags);
+    } catch (ex) {
+      Cu.reportError(
+        `An error occured while trying to fixup "${searchString}": ${ex}`
+      );
+    }
+    return null;
+  }
+
   _afterTabSelectAndFocusChange() {
     // We must have seen both events to proceed safely.
     if (!this._gotFocusChange || !this._gotTabSelect) {
@@ -1579,9 +1599,7 @@ class UrlbarInput {
   }
 
   /**
-   * Shortens the given value, usually by removing http:// and trailing slashes,
-   * such that calling nsIURIFixup::createFixupURI with the result will produce
-   * the same URI.
+   * Shortens the given value, usually by removing http:// and trailing slashes.
    *
    * @param {string} val
    *   The string to be trimmed if it appears to be URI
@@ -1999,20 +2017,15 @@ class UrlbarInput {
     // invalid parts, like the origin, then editing and confirming the trimmed
     // value would execute a search instead of visiting the typed url.
     if (this.value != this._untrimmedValue) {
-      // It doesn't really matter which search engine is used here, thus it's ok
-      // to ignore whether we are in a private context. The keyword lookup will
-      // also distinguish between whitelisted and not whitelisted hosts.
       let untrim = false;
-      try {
-        let fixedSpec = Services.uriFixup.createFixupURI(
-          this.value,
-          Services.uriFixup.FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP |
-            Services.uriFixup.FIXUP_FLAG_FIX_SCHEME_TYPOS
-        ).displaySpec;
-        let expectedSpec = Services.io.newURI(this._untrimmedValue).displaySpec;
-        untrim = fixedSpec != expectedSpec;
-      } catch (ex) {
-        untrim = true;
+      let fixedURI = this._getURIFixupInfo(this.value)?.preferredURI;
+      if (fixedURI) {
+        try {
+          let expectedURI = Services.io.newURI(this._untrimmedValue);
+          untrim = fixedURI.displaySpec != expectedURI.displaySpec;
+        } catch (ex) {
+          untrim = true;
+        }
       }
       if (untrim) {
         this.inputField.value = this._focusUntrimmedValue = this._untrimmedValue;

browser/components/urlbar/UrlbarView.jsm

@@ -1169,7 +1169,7 @@ class UrlbarView {
     this._selectedElement = item;
 
     if (updateInput) {
-      this.input.setValueFromResult(item && item.result);
+      this.input.setValueFromResult(item?.result);
     }
   }
 

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

@@ -59,6 +59,7 @@ support-files =
 skip-if = verify && os == 'linux' # Bug 1581635
 [browser_downArrowKeySearch.js]
 [browser_dragdropURL.js]
+[browser_edit_invalid_url.js]
 [browser_enter.js]
 [browser_enterAfterMouseOver.js]
 [browser_focusedCmdK.js]

browser/components/urlbar/tests/browser/browser_edit_invalid_url.js

+/* Any copyright is dedicated to the Public Domain.
+ * https://creativecommons.org/publicdomain/zero/1.0/ */
+
+// Checks that we trim invalid urls when they are selected, so that if the user
+// modifies the selected url, or just closes the results pane, we do a visit
+// rather than searching for the trimmed string.
+
+const url = BrowserUtils.trimURLProtocol + "invalid.somehost/mytest";
+
+add_task(async function setup() {
+  await SpecialPowers.pushPrefEnv({
+    set: [["browser.urlbar.trimURLs", true]],
+  });
+  await PlacesTestUtils.addVisits(url);
+  registerCleanupFunction(PlacesUtils.history.clear);
+});
+
+add_task(async function test_escape() {
+  await UrlbarTestUtils.promiseAutocompleteResultPopup({
+    window,
+    waitForFocus,
+    value: "invalid",
+  });
+  // Look for our result.
+  let resultCount = UrlbarTestUtils.getResultCount(window);
+  Assert.greater(resultCount, 1, "There should be at least two results");
+  for (let i = 0; i < resultCount; i++) {
+    let result = await UrlbarTestUtils.getDetailsOfResultAt(window, i);
+    info(`Result at ${i} has url ${result.url}`);
+    if (result.url.startsWith(url)) {
+      break;
+    }
+    EventUtils.synthesizeKey("KEY_ArrowDown");
+  }
+  Assert.equal(
+    gURLBar.value,
+    url,
+    "The string displayed in the textbox should be the untrimmed url"
+  );
+  // Close the results pane by ESC.
+  await UrlbarTestUtils.promisePopupClose(window, () => {
+    EventUtils.synthesizeKey("KEY_Escape");
+  });
+  // Confirm the result and check the loaded page.
+  let promise = waitforLoadURL();
+  EventUtils.synthesizeKey("KEY_Enter");
+  let loadedUrl = await promise;
+  Assert.equal(loadedUrl, url, "Should try to load a url");
+});
+
+add_task(async function test_edit_url() {
+  await UrlbarTestUtils.promiseAutocompleteResultPopup({
+    window,
+    waitForFocus,
+    value: "invalid",
+  });
+  // Look for our result.
+  let resultCount = UrlbarTestUtils.getResultCount(window);
+  Assert.greater(resultCount, 1, "There should be at least two results");
+  for (let i = 1; i < resultCount; i++) {
+    EventUtils.synthesizeKey("KEY_ArrowDown");
+    let result = await UrlbarTestUtils.getDetailsOfResultAt(window, i);
+    info(`Result at ${i} has url ${result.url}`);
+    if (result.url.startsWith(url)) {
+      break;
+    }
+  }
+  Assert.equal(
+    gURLBar.value,
+    url,
+    "The string displayed in the textbox should be the untrimmed url"
+  );
+  // Modify the url.
+  EventUtils.synthesizeKey("2");
+  let result = await UrlbarTestUtils.getDetailsOfResultAt(window, 0);
+  Assert.equal(result.type, UrlbarUtils.RESULT_TYPE.URL, "Should visit a url");
+  Assert.equal(result.url, url + "2", "Should visit the modified url");
+
+  // Confirm the result and check the loaded page.
+  let promise = waitforLoadURL();
+  EventUtils.synthesizeKey("KEY_Enter");
+  let loadedUrl = await promise;
+  Assert.equal(loadedUrl, url + "2", "Should try to load the modified url");
+});
+
+async function waitforLoadURL() {
+  let sandbox = sinon.createSandbox();
+  let loadedUrl = await new Promise(resolve =>
+    sandbox.stub(gURLBar, "_loadURL").callsFake(resolve)
+  );
+  sandbox.restore();
+  return loadedUrl;
+}

toolkit/modules/BrowserUtils.jsm

@@ -815,10 +815,15 @@ var BrowserUtils = {
    * @param {string} aURL The URL to trim.
    * @returns {string} The trimmed string.
    */
+  get trimURLProtocol() {
+    return "http://";
+  },
   trimURL(aURL) {
     let url = this.removeSingleTrailingSlashFromURL(aURL);
     // Remove "http://" prefix.
-    return url.startsWith("http://") ? url.substring(7) : url;
+    return url.startsWith(this.trimURLProtocol)
+      ? url.substring(this.trimURLProtocol.length)
+      : url;
   },
 
   recordSiteOriginTelemetry(aWindows, aIsGeckoView) {