1640132 - Make the urlbar always pass a url to the docshell

Status: NEW

(Firefox :: Address Bar, task, P3)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

At a certain point the docshell should stop doing keyword fixup, so we can remove some of the synchronous IPC calls we have for it. For that, the urlbar should always pass a url and never a string to keyword fixup.

After bug 1636583, one case where we still pass a string is for dns_first_for_single_words:
https://searchfox.org/mozilla-central/rev/df4c90d4b8c92c99f76334acfe4813c573c12661/browser/components/urlbar/UrlbarInput.jsm#556-580

For that we should do a dns lookup before invoking _loadURL.

Once that's done, we should probably add fatal asserts in the docshell to check if there are other consumers passing strings to be fixed up (maybe the command line?)... But in general the urlbar should be the only point where the user can introduce a string that could either be a url or a search.

Comment 1

[Sean Feng [:sefeng]]

Attachment: Bug 1640132 - Part 1: Move the logics of dns_first_for_single_words to the Chrome side

The current behaviour of dns_first_for_single_words is asking
the docShell to load http://<word> which involves doing a DNS
query, and if it fails, docShell would do a nsIURIFixup::keywordToURI
to get a search url to search the word.

We move this logic the Chrome side to avoid calling
sync IPC nsIURIFixup::keywordToURI call in the docShell.

Comment 2

[Sean Feng [:sefeng]]

Attachment: Bug 1640132 - Part 2: Attempt to stop doing keywordFixup in DocShell

Depends on D84434

Comment 3

[Sean Feng [:sefeng]]

I see what is happening here, if browser.tabs.documentchannel is true, the fixup happens at https://searchfox.org/mozilla-central/rev/828f2319c0195d7f561ed35533aef6fe183e68e3/netwerk/ipc/DocumentLoadListener.cpp#1935-1939 which is in the parent process. If it's false, the fixup happens at https://searchfox.org/mozilla-central/rev/828f2319c0195d7f561ed35533aef6fe183e68e3/docshell/base/nsDocShell.cpp#6238-6240 which is in the content process.

I don't see there's a case that browser.tabs.documentchannel is true and we end up in the content process. I get this conclusion by doing a try push which I added a MOZ_RELEASE_ASSERT(!XRE_IsContentProcess()); to nsDocShell::KeywordToURI. There was only one test crash browser_progress_keyword_search_handling.js, which flipped the browser.tabs.documentchannel pref accordingly.

If we can confirm that we'd always end up fixing the URI in the parent process if browser.tabs.documentchannel is on, then once we removed the ability to turn off the document channel, there'd no keywordToURI sync IPC message usage.

So this bug is probably not going to be a blocker bug 1375244 anymore once we removed we removed the ability to turn off the document channel.

I wonder if Matt or Anny can confirm that we always end up fixing the URI in the parent process if browser.tabs.documentchannel is on.

Comment 4

[Anny Gakhokidze [:annyG]]

I double checked with Matt and can confirm that we always end up fixing the URI in the parent process. I'm planning to remove the document channel pref in bug 1654922.

Comment 5

[Sean Feng [:sefeng]]

cool, thanks Anny.

Comment 6

[:Gijs (he/him)]

With the document channel pref removed, what's left to do here? Can we just rip out the child / ipc code?

I also wonder if this is the right place - bug 1375244 is specific to the removal of the sync IPC message and can probably be used to remove that and its docshell callsites.

In this bug, I think originally we were hoping to move to a situation where callers of docshell/browsingcontext's loadURI APIs use valid URIs instead of strings. Marco, is that right, and if so, is that something with which we can move ahead?

Comment 7

[Sean Feng [:sefeng]]

I think this bug no longer needs to be a blocker for bug 1375244. And we can keep this bug around to remain it's purpose?

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

In this bug, I think originally we were hoping to move to a situation where callers of docshell/browsingcontext's loadURI APIs use valid URIs instead of strings. Marco, is that right, and if so, is that something with which we can move ahead?

The original plan was certainly to have the docshell stop doing any kind of fixup of non-urls to urls, mostly for separation of concerns (and safety), and so that we could eventually move URIFixup into the urlbar component.
At that point the urlbar would be the only component doing "search string => url", while the docshell would at a maximum fixup http(s) => http(s) or alternate fixup with www.*.com.
There is no perf gain in doing that of course, if we can already remove the IPC, but we can keep this bug around, to implement that separation of concerns.

For future reference, Sean pointed out the problem with progress listeners not being invoked if we move the dns_first lookup into the urlbar code, for which we want to do something to indicate we're in the process of loading a page.

Comment 9

[Sean Feng [:sefeng]]

Unassign this from myself as I am not actively working on this.