1588118 - Investigate why URIFixup may fixup to a search when only FIXUP_FLAG_FIX_SCHEME_TYPOS is passed-in

Status: ASSIGNED

(Core :: DOM: Navigation, defect, P3)

Description

[Marco Bonardo [:mak]]

URI Fixup should likely fix to a search only when FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP is set, but passing just FIXUP_FLAG_FIX_SCHEME_TYPOS is still fixing to a search.
Example:
Services.uriFixup.createFixupURI("blah://test", Services.uriFixup.FIXUP_FLAG_FIX_SCHEME_TYPOS).spec

This looks strange/wrong, but we may have call points expecting this behavior, so fixing this requires a full inspection of each call point to ensure whether it's expecting the search url or not.
Plus understanding how we arrived here.

Comment 1

[:Gijs (he/him)]

It seems we have a test that is supposed to check this doesn't happen at https://searchfox.org/mozilla-central/rev/5e830ac8f56fe191cb58a264e01cdbf6b6e847bd/docshell/test/unit/test_nsDefaultURIFixup.js#51 , and running the exact check that's there inside Firefox does produce a search URL. The difference is that in xpcshell, the keyword.enabled pref is going to be false, whereas it's true (by default) in Firefox.

I suspect we need to ensure that we don't do a search here unless the flag is passed in, and then fix up any frontend consumers (if any) that rely on the current behaviour.

Comment 2

[:Gijs (he/him)]

Fixing this should be straightforward. Auditing callsites that currently pass the TYPOS flag to see if they pass LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP / FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP as well:

browser/base/content/tabbrowser.js -> both callsites pass LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP
browser/base/content/utilityOverlay.js -> passes LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP
browser/components/BrowserContentHandler.jsm -> wants to check a file: URI, probably better if we now avoid keyword lookup.
browser/components/urlbar/UrlbarInput.jsm -> passes FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP
browser/components/urlbar/UrlbarValueFormatter.jsm -> ditto
browser/fxr/content/fxrui.js -> ditto
caps/nsScriptSecurityManager.cpp -> tries to enumerate them, this should be OK.
dom/base/ContentAreaDropListener.jsm -> both callsites pass FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP
mobile/android/chrome/content/browser.js -> passes LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP
toolkit/components/places/UnifiedComplete.jsm -> passes FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP
toolkit/modules/BrowserUtils.jsm -> passes FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP
toolkit/modules/E10SUtils.jsm -> only processes them from load flags to fixup flags

So I think we're all good to just fix this, though there's fortunately also no callsites that pass FIXUP_FLAG_FIX_SCHEME_TYPOS without FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP, so this probably doesn't really matter much.

Comment 3

[:Gijs (he/him)]

Attachment: Bug 1588118 - use the right flag to guard search service use from the URI fixup code, r?mak

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/578e4005a84b
use the right flag to guard search service use from the URI fixup code, r=mak

Comment 5

[Dorel Luca [:dluca]]

Backed out changeset 578e4005a84b (bug 1588118) for Browser-chrome failures in docshell/test/browser/browser_uriFixupIntegration.js. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=272485081&repo=autoland&lineNumber=15485

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=578e4005a84b7b108dab4c8bc3da82abe4df9457

Backout:
https://hg.mozilla.org/integration/autoland/rev/1799f803b23d18b1cde95b4974425a6dd7e2787d

Comment 6

[:Gijs (he/him)]

(In reply to Dorel Luca [:dluca] from comment #5)

Backed out changeset 578e4005a84b (bug 1588118) for Browser-chrome failures in docshell/test/browser/browser_uriFixupIntegration.js. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=272485081&repo=autoland&lineNumber=15485

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=578e4005a84b7b108dab4c8bc3da82abe4df9457

Backout:
https://hg.mozilla.org/integration/autoland/rev/1799f803b23d18b1cde95b4974425a6dd7e2787d

This is failing when inputting a broken protocol and expecting a search - we get an error page instead. I can reproduce this when running the automated test locally with the patch applied, but I don't understand why, because if I manually try to reproduce with ./mach run with the patch applied, I get the expected behaviour (ie a search). So there's a subtle difference in what happens in the test vs. "real" behaviour (again). Marco, can you help me understand what that is / what the location bar is doing here?

Comment 7

[Marco Bonardo [:mak]]

(In reply to :Gijs (he/him) from comment #6)

This is failing when inputting a broken protocol and expecting a search - we get an error page instead. I can reproduce this when running the automated test locally with the patch applied, but I don't understand why, because if I manually try to reproduce with ./mach run with the patch applied, I get the expected behaviour (ie a search). So there's a subtle difference in what happens in the test vs. "real" behaviour (again). Marco, can you help me understand what that is / what the location bar is doing here?

The test tries 2 different ways of inputting text (setValueFns):

1. UrlbarTestUtils.promiseAutocompleteResultPopup... this simulates the user typing, usually in this case the urlbar code makes a decision, based on URIFixup + additional heuristics. It actually waits for the first result and execute it.
2. win.gURLBar.value in this case we just throw a value and confirm it, the urlbar doesn't even try to fetch results, it just passes down the value to the docshell. This can happen for a user in the Paste & Go or Drop & go cases, for example.

Ideally we'd like to revert to a search for "something:somethingelse", that makes me think maybe the docshell is not allowing keyword fixup for this specific path?

Comment 8

[:Gijs (he/him)]

The URL bar passes LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP correctly (well, it passes params.allowThirdPartyFixup which gets converted in utilityOverlay.js) but because of this:

NS_IMETHODIMP
nsDefaultURIFixup::WebNavigationFlagsToFixupFlags(const nsACString& aStringURI,
                                                  uint32_t aDocShellFlags,
                                                  uint32_t* aFixupFlags) {
  nsCOMPtr<nsIURI> uri;
  NS_NewURI(getter_AddRefs(uri), aStringURI);
  if (uri) {
    aDocShellFlags &= ~nsIWebNavigation::LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;
  }
  ...

in nsDefaultURIFixup.cpp, called from https://searchfox.org/mozilla-central/source/toolkit/components/remotebrowserutils/RemoteWebNavigation.jsm#86-93 , we throw away that fixup flag for bogus protocol things.

I think in this case we shouldn't be removing that fixup flag. The docshell code that was replaced by a call to webNavigationFlagsToFixupFlags did the same thing before bug 1552017. It seems the only reason this whole thing worked was the "wrong" flag check that I'm fixing here. :-(

I can change this inside remotewebnav, but then I guess the same thing would presumably still happen for non-remote webnavigations being invoked, as that's effectively the same as calling docshell's loadURI directly. Admittedly, that's pretty rare these days but still worth bearing in mind...

I could change it in the URL bar code (ie I could make it enforce fixup before passing a URI into docshell), but then the same problem would happen for other callers of loadURI...

(Aside: in a way this is a continuing result of having docshell APIs that take strings and then take weird generic decisions - we should ideally push the fixup stuff out of docshell, though where exactly it should live is unclear to me off-hand and probably warrants more discussion)

Marco, thoughts?

Comment 9

[Marco Bonardo [:mak]]

(In reply to :Gijs (he/him) from comment #8)

in nsDefaultURIFixup.cpp, called from https://searchfox.org/mozilla-central/source/toolkit/components/remotebrowserutils/RemoteWebNavigation.jsm#86-93 , we throw away that fixup flag for bogus protocol things.

In practice that checks if we can build a uri from the string, and because we can, it decides we don't want to fix it up.
It seems... wrong? Or at least, just building a URI sounds like a bad check without ensuring whether the result makes sense.

I can change this inside remotewebnav, but then I guess the same thing would presumably still happen for non-remote webnavigations being invoked, as that's effectively the same as calling docshell's loadURI directly. Admittedly, that's pretty rare these days but still worth bearing in mind...

When may it happen? I'm not up-to-date regarding remotewebnav.

I could change it in the URL bar code (ie I could make it enforce fixup before passing a URI into docshell), but then the same problem would happen for other callers of loadURI...

See below, how many cases are effectively problematic? This only affects features taking urls from the user.
I think for now I'd go the remotewebnav path... if we have an idea of the cases where we don't go through it but we still take user's input, it would be useful to re-evaluate.

(Aside: in a way this is a continuing result of having docshell APIs that take strings and then take weird generic decisions - we should ideally push the fixup stuff out of docshell, though where exactly it should live is unclear to me off-hand and probably warrants more discussion)

In my vision the only things that should be trying to fixup strings are the ones accepting user input, that pretty much boils down to the urlbar and the command line. Do you have other use-cases in mind?
Reducing the number of effective need would also reduce the likelihood of unwated fixing.
Why are we allowing to fixup things from session restore or the tabbrowser for example?
If we'd have a list of call points where we effectively want to fixup loaded things, it would be easier to evaluate where to move fixup.
Just looking at the list of call points for LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP and FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP, it's unclear why some of them would want to fixup things that are expected to be URIs.

Comment 10

[Jim Porter (:squib)]

Following along on this. So long as whatever changes here still ensure that the URL we fix up for RemoteWebNavigation matches the URL the docshell ultimately loads, the stuff in bug 1552017 should still work fine.

Comment 11

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #9)

(In reply to :Gijs (he/him) from comment #8)

in nsDefaultURIFixup.cpp, called from https://searchfox.org/mozilla-central/source/toolkit/components/remotebrowserutils/RemoteWebNavigation.jsm#86-93 , we throw away that fixup flag for bogus protocol things.

In practice that checks if we can build a uri from the string, and because we can, it decides we don't want to fix it up.
It seems... wrong? Or at least, just building a URI sounds like a bad check without ensuring whether the result makes sense.

Right, I think we agree here, but also the docshell code has done this since 2006 and so changing that seems like a can of worms. Adding an arg to the utility function that was introduced and shared into RemoteWebNavigation feels like a wallpaper-fix, and ugly, and then also the next point:

I can change this inside remotewebnav, but then I guess the same thing would presumably still happen for non-remote webnavigations being invoked, as that's effectively the same as calling docshell's loadURI directly. Admittedly, that's pretty rare these days but still worth bearing in mind...

When may it happen? I'm not up-to-date regarding remotewebnav.

browsers and other "frame-like" things have an associated nsIWebNavigation instance - which is really just another interface generally implemented directly by nsDocShell. So there's a webNav getter on browser that returns this. For remote browsers, obviously the <browser> and the docshell are in a separate process, so RemoteWebNavigation is supposed to remote/IPC/message-manager things across so there's an identical interface and things more or less Just Work. In fact, on trunk, the remoting then gets redirected through BrowsingContext (bug 1578624) but that's not super relevant here. As far as frontend consumers are concerned, they just load stuff using the webnavigation associated with the browser.

For browser tabs, we'll pretty much always have a remote webnavigation instance. However, there are exceptions, e.g. for things loaded in the parent process (like the prefs etc.). Practically, those edgecases may not matter much for fixup because we hopefully determine we want a different process well before the URL goes anywhere near a docshell. In any case, it'd be inconsistent not to fix the "real"/"base" WebNavigation implementation, which is docshell... Which as said above, has been doing this since 2006. Yay!

I could change it in the URL bar code (ie I could make it enforce fixup before passing a URI into docshell), but then the same problem would happen for other callers of loadURI...

See below, how many cases are effectively problematic? This only affects features taking urls from the user.

This is a fair point.

I think for now I'd go the remotewebnav path... if we have an idea of the cases where we don't go through it but we still take user's input, it would be useful to re-evaluate.

Well, as said, it'd be a bit inconsistent / wall-papery. The remotewebnav just happens to be in front of the docshell for remote tabs, but won't be for same-process tabs, so it doesn't really solve the core issue.

(Aside: in a way this is a continuing result of having docshell APIs that take strings and then take weird generic decisions - we should ideally push the fixup stuff out of docshell, though where exactly it should live is unclear to me off-hand and probably warrants more discussion)

In my vision the only things that should be trying to fixup strings are the ones accepting user input, that pretty much boils down to the urlbar and the command line. Do you have other use-cases in mind?

In principle I think this is right and it's music to my ears. As I said in the aside, we really need to stop passing arbitrary strings into docshell.

In practice, things like https://searchfox.org/mozilla-central/rev/088e2cf29c59d733d57af43903eb0267dbf72e2a/docshell/base/nsDocShell.cpp#6613-6624 would need removing and ensuring there are no more edgecases (e.g. with the fixup_dns_first_for_single_words pref turned on) where we hit it. I think right now there are still some cases where we hit this, and I don't think we can transparently fix this issue outside of docshell, though I could be wrong...

Reducing the number of effective need would also reduce the likelihood of unwated fixing.
Why are we allowing to fixup things from session restore or the tabbrowser for example?
If we'd have a list of call points where we effectively want to fixup loaded things, it would be easier to evaluate where to move fixup.
Just looking at the list of call points for LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP and FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP, it's unclear why some of them would want to fixup things that are expected to be URIs.

I guess quite some of them are wanting to be "safe" - fixup pretty much guarantees a valid URL if search is enabled. Otherwise you get to deal with the mess of "a bookmark pointed to something we don't (any longer) understand as a URI, what do we do now?", having to come up with UI, etc.
For session restore, I think we keep the original input in the location bar until the web progress listener notifies us the request started (which really means: we have headers back from the server (!), which is a bit counter-intuitive imo). So I imagine the fixup is there to make sure that if we session restore such a tab, we restart the request correctly.

You suggested fixing this in RemoteWebNavigation in the short term. Given all of the above, do you still think that's OK? For the purposes of this test, we could also fix in the url bar (and commandline handler, if that doesn't already do it), which would need to happen anyway for the longer-term goal of ensuring we pass "finished" URLs to webnavigation.loadURI from the frontend consumer(s)...

Comment 12

[Marco Bonardo [:mak]]

(In reply to :Gijs (he/him) from comment #11)

I guess quite some of them are wanting to be "safe" - fixup pretty much guarantees a valid URL if search is enabled. Otherwise you get to deal with the mess of "a bookmark pointed to something we don't (any longer) understand as a URI, what do we do now?", having to come up with UI, etc.

Yep, I'd just consider it error not found.

For session restore, I think we keep the original input in the location bar until the web progress listener notifies us the request started (which really means: we have headers back from the server (!), which is a bit counter-intuitive imo). So I imagine the fixup is there to make sure that if we session restore such a tab, we restart the request correctly.

I still don't understand, we restore the original input in the urlbar, it will still go through the usual docshell dance.

You suggested fixing this in RemoteWebNavigation in the short term. Given all of the above, do you still think that's OK? For the purposes of this test, we could also fix in the url bar (and commandline handler, if that doesn't already do it), which would need to happen anyway for the longer-term goal of ensuring we pass "finished" URLs to webnavigation.loadURI from the frontend consumer(s)...

It's a complicate choice, also because there's no ETA on anything of this fixup work.
If we decide our future target is "stop having the docshell fixup things", and we seem to agree here, surely fixing this in the urlbar sounds more future proof.
I don't mind that much about the command line case, it's really an edge case and one can already specify if something is a search by using --search, for which I think we should not even fixup what is passed to the command line. If it's broken, it's broken.

That said, what we do in WebNavigationFlagsToFixupFlags is still wrong, if we have a decent idea to fix it, I'd also do that, otherwise we should at least comment on it, so what you found doesn't get lost.

I figure my answer is basically "let's do both".

Comment 13

[:Gijs (he/him)]

Attachment: Bug 1588118 - do fixup in the URL bar instead of delegating to docshell, r?mak

Depends on D50051

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:Gijs, could you have a look please?
For more information, please visit auto_nag documentation.