Closed Bug 1560676 Opened 3 years ago Closed 2 months ago

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

Categories

(Firefox :: Address Bar, defect, P3)

defect
Points:
3

Tracking

()

VERIFIED FIXED
102 Branch
Iteration:
69.4 - Jun 24 - Jul 7
Tracking Status
firefox69 --- wontfix
firefox102 --- verified
firefox103 --- verified

People

(Reporter: adw, Assigned: jteow)

References

(Blocks 2 open bugs)

Details

(Keywords: papercut, Whiteboard: [snt-triaged][search-urlbar])

Attachments

(2 files)

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.

Assignee: nobody → adw
Status: NEW → ASSIGNED
Iteration: --- → 69.4 - Jun 24 - Jul 7
Points: 2 → 3

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.

Blocks: 1590487

(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.

Severity: normal → S3
Priority: P2 → P3
Keywords: papercut

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.

Assignee: adw → jteow
Whiteboard: [snt-triaged][search-urlbar]

Possible schemes without double slash:

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

Also helpful:

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:+15105550101?body=hello%20there
  • smsto
  • xmpp - xmpp:foo@example.com

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

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.

Modifying comment because it's acting a bit odd.

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();
});
Flags: needinfo?(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.

Flags: needinfo?(adw)
Attachment #9074932 - Attachment description: Bug 1560676 - Fix URL/path autofill for URLs whose hosts look like prefixes, like localhost:8888/foo. → Bug 1560676 - Fix URL/path autofill for URLs whose hosts look like prefixes, like localhost:8888/foo. r?adw
Pushed by jteow@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 102 Branch

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.

Status: RESOLVED → VERIFIED
Regressions: 1773298
You need to log in before you can comment on or make changes to this bug.