1560676 - URL/path autofill is broken for URLs like localhost:8888/foo

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P3)

Description

[Drew Willcoxon :adw]

STR

1. Run a local server at some non-standard port (e.g. 8888) that serves a subdirectory (e.g. localhost:8888/foo)
2. Visit localhost:8000/foo
3. Start to type localhost so that localhost:8888 autofills
4. Move the caret to the end of the input and start typing foo

foo never autofills.

The problem is that stripPrefix() in UnifiedComplete thinks that "localhost:" is a prefix, so _strippedPrefix ends up getting set to "localhost:", which causes urlQuery() to return the wrong SQL query for URL/path autofill. If you force stripPrefix to return ["", str], then the STR above work as expected.

To be clear, this isn't a quantumbar problem but a UnifiedComplete problem that affects awesomebar too.

imo it would be good if we could fix this soon since it disproportionately affects developers, and developer mindshare is important. Seems like a simple fix, too.

I found this while investigating bug 1556425.

Comment 1

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0a9e41affafe964776ea9112c222ae85d2ac54b

Comment 2

[Drew Willcoxon :adw]

Attachment: Bug 1560676 - Fix URL/path autofill for URLs whose hosts look like prefixes, like localhost:8888/foo. r?adw

This is trickier than it seemed for a couple of reasons.

(1) It's ambiguous whether "foo:8888" is a full URI whose scheme is foo and whose path is 8888, or a substring of a URI whose host is foo and whose port is 8888. In the first case, "foo:" is the prefix. In the second case, there's no prefix. (Services.io.newURI isn't helpful here because for "localhost:8888" it returns a URI whose scheme is localhost of course, which is not what we want.)

(2) _strippedPrefix is used for other things besides autofill, so we need to be careful to either not change its meaning too much, or if we do change its meaning then we need to update other places that use it.

The simplest fix I found that still passes all our tests is limiting _strippedPrefix to a set of well known schemes. An alternative would be to reject a set of substrings that we know aren't schemes, like localhost. But it seems more likely that people would be accessing local servers than they would be accessing non-well known schemes.

Comment 3

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #2)

The simplest fix I found that still passes all our tests is limiting _strippedPrefix to a set of well known schemes.

we used to do this, iirc just for http and https, we changed it along with new origins autofill, that change caused other regressions, like for example bug 1503293 and bug 1642133 (maybe bug 1531335?)... I think we should revert it to how it was before.

Comment 4

[Drew Willcoxon :adw]

Based on Marco's phabricator comment, looks like what we need to do here is a two-pronged approach: (1) have a regex for detecting schemes with :// (double slashes), (2) on top of that, have a separate safe list for schemes that don't use double slashes, like about. In the phabricator I mentioned I'd try to find a list of such schemes or see if we could determine one at runtime. I don't have an update on that, so figuring that out is part of this bug.

Comment 5

[Drew Willcoxon :adw]

Possible schemes without double slash:

• about
• data
• javascript
• mailto
• news
• tel
• urn

Comment 6

[James Teow [:jteow]]

Also helpful:

• https://en.wikipedia.org/wiki/List_of_URI_schemes
• https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/protocol_handlers
• https://searchfox.org/mozilla-central/rev/cda5e5c662c2e0da5ee63dc4669899e3ed4ccb18/dom/base/Navigator.cpp#880

One thing we were missing that loads in the browser:

• view-source - view-source:https://www.example.com/

Map features:

• bingmaps - bingmaps:?cp=47.36667~8.55 - Possibly Windows?
• geo - geo:40.726966,-74.006076 - Tested on Android
• maps - maps:q=<physical location> - OSX

I've highlighted some schemes that can use no slashes and entered in the urlbar have a unique interaction with the browser, namely they're mentioned in the code, or will launch an external application:

• bitcoin - bitcoin:mjSk1Ny9spzU2fouzYgLqGUD8U41iR35QN
• callto - callto:<screenname>
• im - im:pepp=example.com/fred@relay-domain
• message - message:example
• sip and sips
• sms - sms:[phone]?body=hello%20there
• smsto
• xmpp - xmpp:[e-mail]

Other applications can be launched via keywords, too many to list here: https://searchfox.org/mozilla-central/rev/cda5e5c662c2e0da5ee63dc4669899e3ed4ccb18/toolkit/mozapps/handling/ContentDispatchChooser.jsm#37

Comment 7

[James Teow [:jteow]]

After talking with :adw, I think we've decided to create a safelist that's limited to those presented in the browser and potentially bookmarked rather than try to capture all scheme's, as there are dozens supported by the browser that will launch an external program and they'll also depend on the user's machine.

So the shortlist is:

• about
• data
• javascript
• view-source

I did ask the networking channel on Matrix to see if there's a way to retrieve a list of schemes, but wasn't able to get an answer.

Comment 8

[James Teow [:jteow]]

Attachment: Dump of context.results

Modifying comment because it's acting a bit odd.

Comment 9

[James Teow [:jteow]]

Hey Drew, I have a couple questions related to the bug. I think the algorithm we talked about for stripURLPrefix is fine, but I am running up into some issues with the tests:

When I add localhost to the history via PlacesTestUtils.addVisits, I'm expecting only to see one result but I'm seeing a second one specifically for lo and localhost. I attempt to run check_results with the following:

add_task(async function originLooksLikePrefix() {
  let hostAndPort = "localhost:8888";
  let address = `http://${hostAndPort}/`;
  await PlacesTestUtils.addVisits([{ uri: address }]);
  for (let search of ["lo", "localhost", "localhost:", "localhost:8888"]) {
    let context = createContext(search, { isPrivate: false });
    await check_results({
      context,
      autofilled: hostAndPort + "/",
      completed: address,
      matches: [
        makeVisitResult(context, {
          uri: address,
          title: hostAndPort,
          heuristic: true,
        }),
      ],
    });
  }
  await cleanup();
});

Here is a dump of context.results when the test fails on lo and localhost example:

[{
    "type ": 3,
    "source ": 2,
    "rowIndex ": -1,
    "heuristic ": true,
    "payload": {
        "title": "localhost:8888",
        "url": "http://localhost:8888/",
        "icon": "page-icon:http://localhost:8888/",
        "displayUrl": "http://localhost:8888"
    },
    "payloadHighlights": {
        "title": [
            [0, 2]
        ],
        "url": [
            [7, 2]
        ],
        "displayUrl": [
            [7, 2]
        ],
        "icon": []
    },
    "autofill": {
        "value": "localhost:8888/",
        "selectionStart": 2,
        "selectionEnd": 15
    },
    "providerName": "Autofill",
    "providerType": 1
}, {
    "type": 8,
    "source": 3,
    "rowIndex": -1,
    "heuristic": false,
    "payload": {
        "engine": "Suggestions",
        "url": "localhost",
        "providesSearchMode": true,
        "icon": "chrome://global/skin/icons/search-glass.svg",
        "dynamicType": "onboardTabToSearch",
        "satisfiesAutofillThreshold": false
    },
    "payloadHighlights": {
        "engine": [],
        "url": [],
        "providesSearchMode": [],
        "icon": [],
        "dynamicType": [],
        "satisfiesAutofillThreshold": []
    },
    "resultSpan": 2,
    "suggestedIndex": 1,
    "providerName": "TabToSearch",
    "providerType": 2
}]

Note that if I change localhost:8888 to something gibberish with a port e.g. mocalhost:8888, and change the strings in the array to some variant of mocalhost, the tests pass, which implies a different treatment for localhost somewhere along the way?

Additionally, for testing the URI safelist, history seems nuanced. It seems like there's a back/forward history and a History that's managed by History.jsm? So a user can go back and forth between non-https pages and https pages using the back/forward functionality, but they won't see those pages in the list of urls from History, which is relevant since the tests use insertMany before entering terms in the URL bar. Tossing a non-standard URI will result in No items were added to history. error.

So I'm assuming that for javascript , data-url, and view-source, I'm going to have to add a bookmark. This is how I was able to make tests pass but I believe I'm missing the point of not including autofilled?:

add_task(async function nonStandardBookmarkPrefix() {
  Services.prefs.setBoolPref("browser.urlbar.filter.javascript", false);
  let address = "javascript:(()=>())();";
  let title = "Bookmarklet";
  await PlacesTestUtils.addBookmarkWithDetails({
    uri: address,
    title,
  });
  for (let search of ["ja", "javascript"]) {
    let context = createContext(search, { isPrivate: false });
    await check_results({
      context,
      completed: address,
      matches: [
        makeSearchResult(context, {
          query: search,
          engineName: Services.search.defaultEngine.name,
          heuristic: true,
        }),
        makeBookmarkResult(context, {
          uri: address,
          title: "Bookmarklet",
          iconUri: "chrome://global/skin/icons/defaultFavicon.svg",
        }),
      ],
    });
  }

  for (let search of ["javascript:(", "javascript:(()=>())();"]) {
    let context = createContext(search, { isPrivate: false });
    await check_results({
      context,
      completed: address,
      matches: [
        makeVisitResult(context, {
          uri: search,
          source: UrlbarUtils.RESULT_SOURCE.OTHER_LOCAL,
          title: search,
          heuristic: true,
        }),
        makeBookmarkResult(context, {
          uri: address,
          title: "Bookmarklet",
          iconUri: "chrome://global/skin/icons/defaultFavicon.svg",
        }),
      ],
    });
  }
  await cleanupPlaces();
});

Comment 10

[Drew Willcoxon :adw]

Hi James, sorry for the delay.

When I add localhost to the history via PlacesTestUtils.addVisits, I'm expecting only to see one result but I'm seeing a second one specifically for lo and localhost.

The second result is called a tab-to-search result and is created by UrlbarProviderTabToSearch. You can see this result in Firefox by typing "google", "bing", "duckduckgo", or any other domain name for an installed search engine.

As best I can tell, the reason it appears in the test for localhost is that the test creates a local server to install a mock search engine. See the call to testEngine_setup() in test_autofill_origins.js, which is defined here. If you follow that code path, you'll see it sets up a server.

The way tab-to-search (TTS) works is that it'll show a TTS result for an installed engine when you start typing the engine's domain name. I guess in this case since the server is local, it shows the TTS when the test types "localhost".

To fix that, you can disable the TTS result by calling UrlbarPrefs.set("suggest.engines", false). That sets the browser.urlbar.suggest.engines pref to false. The TTS provider checks that pref in its isActive() implementation; if the pref is false, then isActive() returns false, meaning the provider is saying "I won't provide a result for this search."

So I'm assuming that for javascript , data-url, and view-source, I'm going to have to add a bookmark. This is how I was able to make tests pass but I believe I'm missing the point of not including autofilled?:

Yes, that's what I would do, and it's totally fine for the test to trigger autofill that way. The point of the test is to test how autofill works; we don't really care about why or how a particular URL becomes eligible for autofill. (There are two ways a domain can be eligible for autofill: it's bookmarked or it's been visited enough that its frecency is >= the average frecency of all domains.)

but I believe I'm missing the point of not including autofilled?:

Yes :-) We should definitely be checking autofilled here, and the first expected result should be a visit result.

If I create a bookmark in Firefox with URL javascript:alert('hi'); and then I start typing the URL, autofill doesn't kick in until I type the "a" in "alert". (I'm testing stock Firefox without your patch.)

If I create one with URL data:text/html,hi, it doesn't start autofilling until I type the "t" in "text".

So there may be some other problem that's preventing autofill from kicking in too late in these cases, I'm not sure. It may also be related to the fact that these types of URLs don't have domains, so domain autofill won't kick in for them, of course. So the only autofill method that can possibly autofill them is URL up-to-the-next-slash autofill. But URL autofill only kicks in after you've typed a domain. So IOW these are weird corner-case URLs that our autofill system just probably isn't handling well to begin with. We can investigate this when we meet tomorrow, and of course please try on your own too if you have time.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e478e0b2552
Fix URL/path autofill for URLs whose hosts look like prefixes, like localhost:8888/foo. r=adw

Comment 12

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/2e478e0b2552

Comment 13

[Cristian Baica [:cbaica], Release Desktop QA]

I've managed to reproduce the issue in Fx101.0a1.
The issue is verified fixed using the latest Fx103.0a1 and Fx102.0b5. Firefox correcty autofills URLs for hosts that look like prefixes.