1375244 - Remove sync PContent::Msg_KeywordToURI IPC message

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Mike Conley (:mconley) (:⚙️)]

This seems to happen when loading the aljazeera page in the tp5o talos test with e10s enabled:

https://perf-html.io/public/e05b0638aa95a62ecfd046f71760abbf37be5c50/calltree/?invertCallstack&range=0.3706_0.7437~0.3730_0.7405~0.3918_0.5029&thread=1

Comment 1

[Andrew Overholt [:overholt]]

Tom, can you take a look here? Blame seems to say you had something to do with this.

Comment 2

[Tom S. (please needinfo tschuster)]

Seems like I implemented this in bug 905761. I honestly don't remember much of this anymore. I could imagine syncing the available keyword names between child and parent would help here. I currently don't have time to look into this myself.

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Stone, could you please help us here? Thank you!

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Gijs, you might have some opinion how this should be fixed. Should we mirror nsIBrowserSearchService to child process and update it asynchronously?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

That is http://searchfox.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#72

Comment 6

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai (55 Regression Engineering support) [:hsinyi] from comment #3)
> Stone, could you please help us here? Thank you!

Let's see if Gijs has more thoughts then to figure out how to help this bug. Cancelling the flag on Stone for now. Thanks.

Comment 7

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug] from comment #4)
> Gijs, you might have some opinion how this should be fixed. Should we mirror
> nsIBrowserSearchService to child process and update it asynchronously?

Well, that would be one solution...

TBH, I'm tempted to suggest we should just remove access to this method of fixing up in the content process, though I haven't done an exhaustive check on what the usecases for it are.

The call in the profile comes through browser frontend code for detecting RSS/atom feeds, which is trying to do a URI security check and passes a string that isn't a valid URI, which then in turn causes the CAPS code to try to fix up the URI the same way it would normally be fixed up. So this particular usecase we could just fix by making sure the feeds code obtains its own valid URI (potentially with non-search fixup, as search-based fixup will never yield the right 'feed' URI). This would be good to do anyway because it avoids useless busy-work.

I don't know how many other usecases there are for using this code in the content process in a sync fashion. At a high level, my thinking would be that we should never use search fixup for loads based on URIs provided by web content (ie if a webpage specifies an invalid <a href>, we shouldn't (and, I think, don't) open Google/DDG/whatever when the user clicks it), only for content provided by the user, which should only happen in the parent process anyway. Of course, in practice, it's quite possible there are other extant usecases that would break if we made this code just throw (ie return NS_ERROR_DOM_BAD_URI or whatever) in the content process... For instance, maybe there are some cases where loads from the browser URI arrive as a string in the content process that currently get turned into a search URI there - but if this is the case, it's already very inefficient and it would IMO make more sense to just do that transformation in the parent process anyway.

Comment 8

[:Gijs (he/him)]

I'll try to investigate this in a bit more detail this week to see how easy it is to just dump this type of fixup instead of making it possible to do these kinds of lookups in the content process.

Comment 9

[:Gijs (he/him)]

Attachment: WIP patches

Blake said he was willing to take this over. These are my WIP patches. I think #3 on its own is likely enough to fix the feed stupidity.

The others were mostly written because at one point I thought we could get rid of LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP, but that turned out to be a pipe dream because we use it to determine whether docshell is interested in having that type of fixup on a given load, and I assume we want to keep that to distinguish loads initiated by the user from loads initiated via e.g. website script setting location.href . As a result, I'm not sure to what extent the 3 other patches that try to do keyword lookup earlier (one for sessionrestore, one on android, one for firefox browser consumers [tabbrowser.xml,browser.js,utilityOverlay.js]) when possible are actually useful. Unless we're willing to drop extant functionality (which we could do, arguably, because the default configuration is now to do lookups for users typing just 'foo' in the url bar, though there is a pref to turn that off...), docshell itself will still want to do lookups even when URIs seem well-formed at the surface ("http://foo/") but don't actually end up going anywhere.

The topmost / last patch is genuinely WIP in that I was on the way to farm out the actual fixup calls into nsIBrowserChrome3, with async messaging from tab-content.js into the parent process. The trickiest bit about that is how to deal with the scenario where:

1) docshell does a load
2) the load errors and it tells browser code to fixup or call displayLoadError()
3) browser code contacts parent process
4) meanwhile, something else decides to load in the docshell
5) the parent process tells the content process the result of its fixup attempts

now we don't want to override the load from (4) with the result from (5). bz suggested using numbered loads in docshell to verify nothing else has attempted to load. I don't know if that's sufficient, I didn't get that far. :-)

Comment 10

[:Gijs (he/him)]

My final observation was that http://searchfox.org/mozilla-central/rev/8a61c71153a79cda2e1ae7d477564347c607cc5f/toolkit/components/search/nsSearchService.js#1044 seems like it could be potentially factored out into a separate script that we could load in both parent and child, and use to serialize/deserialize - see toJSON and _initializeFromJSON methods - (and thus broadcast from parent to child) what are essentially 'factories' for going from 'some input' to a valid search url+postdata combination. That way we could continue to do the search stuff sync in the content process. This might be simpler than pursuing the async option further, but I haven't tested that hypothesis.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

I broke the feedreader bit out into bug 1375243, I think that with that landed, this doesn't have to be a P1.

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

Re-prioritize based on comment 11

Comment 13

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Move to P2 per comment 11

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8890095 [details] [diff] [review]
WIP patches

I'm not going to get a chance to look at this, sorry.

Comment 16

[Marco Bonardo [:mak]]

! In D70607#2160695, @mattwoodrow wrote:
I think this is fine for the meantime, trying to remove callers using DocumentChannel is probably harder.

I had a look through the callers, it appears that handling fixups on failed loads is a common one. I think that code could live in DocumentChannel (along with error page selection) to avoid needing those fixups in the content process.

We might also want an nsDocShellLoadState with lazily-created nsIURI, so we can wait until DocumentChannel has sent it to the parent before resolving it with fixups.

Comment 17

[Sean Feng [:sefeng211]]

This bug no longer needs to depend on Bug 1640132 as after bug 1654922, we'd always fixing up the url in the parent process. So the IPC usage can be safely removed.

I also did a mistake by setting this bug depends on Bug 1653277, so I am also removing this dependency.

Comment 18

[:Gijs (he/him)]

Mark, what's left to do here? Can we just remove all of this now?

Comment 19

[Mark Banner (:standard8)]

Given comment 17, it looks like we can just remove it. I'll give it a shot.

Comment 20

[Andrew McCreight [:mccr8]]

Be sure to remove the sync-messages.ini entry or you'll get a build error.

Comment 21

[Mark Banner (:standard8)]

Try run seems good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5772ccb701fd564111a918b6486957b389c8ec78

Comment 22

[Mark Banner (:standard8)]

Attachment: Bug 1375244 - Remove sync KeywordToURI and related IPC messages as they are no longer required. r?gijs!,mak!

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05007ff436a5
Remove sync KeywordToURI and related IPC messages as they are no longer required. r=Gijs,mak,mccr8

Comment 24

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/05007ff436a5