1591032 - WebNavigationFlagsToFixupFlags discards LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP / FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP if string can be parsed as a URI

Status: NEW

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

Description

[:Gijs (he/him)]

Background: bug 1588118 comment 8:

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

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/rev/088e2cf29c59d733d57af43903eb0267dbf72e2a/toolkit/components/remotebrowserutils/RemoteWebNavigation.jsm#86-93 , we throw away that fixup flag for bogus protocol things.

I think in this case [ed.: for URL-like-but-not-URL input such as "foo:bar"] 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. :-(

This was reported as an issue in bug 762835, worked around in bug 982428 (I think by accident? There's no evidence that I realized at the time that the use of the "wrong" fixup flag to gate the check worked around this problem), but the fundamental problem is that we're treating parsed URIs as evidence that this is the URI the user wants, which isn't the case.

AFAICT from bug 263213 comment #0, the issue was that searches were happening for http(s) URLs without a "." in the hostname, instead of "just" showing the error page, even when http:// was input explicitly by the user (ie "http://foo" rather than just "foo").

My understanding is that, although in a clean/default profile we'll default to search for a single word after bug 693808, with the right preference flipped (used by enterprise and "supported" via enterprise policy...) we will not, and first do the DNS lookup, and in https://searchfox.org/mozilla-central/rev/088e2cf29c59d733d57af43903eb0267dbf72e2a/docshell/base/nsDocShell.cpp#6613 docshell itself takes responsibility for doing the search only once DNS lookup has failed.

I think the preferred solution here looks something like this:

1. make fixup code support an async path
2. call the async path from frontend code
3. in the async path, if the right prefs are set, frontload the DNS lookup and use its result to determine whether to return the original http://foo thing or the search
4. move fixup out of docshell - docshell should take an nsIURI. It's now frontend's responsibility to pass it a URI, and the URL bar and maybe very few other consumers can call uriFixup and/or just call new URL() and fail to do any loads if that doesn't work.
   (this has the added bonus that it enables us to finally get rid of the sync IPC that's implied in the existing docshell codepath, as the child has no functioning search service and needs to ask the parent - though admittedly there may be simpler solutions to that particular problem.)

In the short term, we could potentially improve the check that removes the keyword flag by making it specific to having a URI with the http/https protocol (or one that gets corrected, if the fixup_scheme_typos flag was passed)? This is a bit fragile and spreads fixup logic around which is annoying. But it'd be more correct...

Boris, how do you feel about the above options? Is there some case I'm missing?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Steps 1-3 make sense.

Step 4 would be lovely; I've wanted that for a while. The one caveat is that we may want to have a shared module multiple frontends (e.g. Firefox and Fenix) could use for this.

I looked at the consumers of the string version of LoadURI on docshell and they are:

1. The scripted caller; we'd have to check what those callsites are, but they're all in JS.
2. The "alternate URI" handling in the failed-load case. This could switch to just passing in the nsIURI it has instead of getting a string from it (!), though with the plan above it sounds like we might move that out into the frontend as well.
3. Drop event handling in nsDocShellTreeOwner::HandleEvent. Those links are coming from dom/base/ContentAreaDropListener.jsm as far as I can tell, and we could do fixup in there if we wanted to, though I'm not sure about the async bits.
4. BrowserChild::RecvLoadURL. This is sending the string over IPC. I think the original place it comes from is BrowserBridgeHost::LoadURL which starts off with an nsIURI to start with (this is a URI being loaded in a subframe), so we should just be able to NS_NewURI here, I would think. Need to check whether some of the "no host found" fixup currently happens in that situation...
5. A few places (FramingChecker::CheckFrameOptions, nsEditingSession::TimerCallback) that are loading about:blank and hence don't need any fixup.
6. nsWebBrowser::LoadURI. Pretty sure some of the callers above are actually calling into this, so all the same considerations apply.
7. nsWebShellWindow::Initialize. This starts with an nsIURI and gets its spec, then loads that. I'd think we could just switch it to passing in the nsIURI. In particular, the only places that pass in a non-null URI here are hidden window creation (which passes in the hidden window url), and nsXULWindow::CreateNewContentWindow, which always passes BROWSER_CHROME_URL_QUOTED, and neither of these should need any sort of fixup.