Closed Bug 1375244 Opened 4 years ago Closed 6 months ago

Remove sync PContent::Msg_KeywordToURI IPC message

Categories

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

enhancement
Points:
3

Tracking

()

RESOLVED FIXED
82 Branch
Iteration:
82.2 - Sep 7 - Sep 20
Tracking Status
firefox82 --- fixed

People

(Reporter: mconley, Assigned: standard8)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [qf:p3])

Attachments

(2 files)

Whiteboard: [qf] → [qf:p1]
Tom, can you take a look here? Blame seems to say you had something to do with this.
Flags: needinfo?(evilpies)
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.
Flags: needinfo?(evilpies)
Stone, could you please help us here? Thank you!
Flags: needinfo?(sshih)
Gijs, you might have some opinion how this should be fixed. Should we mirror nsIBrowserSearchService to child process and update it asynchronously?
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(sshih)
(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.
Flags: needinfo?(gijskruitbosch+bugs)
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.
Flags: needinfo?(gijskruitbosch+bugs)
Attached patch WIP patchesSplinter Review
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. :-)
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8890095 - Flags: feedback?(mrbkap)
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.
Priority: -- → P1
I broke the feedreader bit out into bug 1375243, I think that with that landed, this doesn't have to be a P1.
Re-prioritize based on comment 11
Whiteboard: [qf:p1] → [qf]
Whiteboard: [qf] → [qf:p3]
Move to P2 per comment 11
Priority: P1 → P2
Keywords: perf
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
Priority: P2 → P3
Comment on attachment 8890095 [details] [diff] [review]
WIP patches

I'm not going to get a chance to look at this, sorry.
Attachment #8890095 - Flags: feedback?(mrbkap)
Component: DOM → DOM: Core & HTML

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

Depends on: 1640132
Depends on: 1653277

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.

No longer depends on: 1640132, 1653277

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

Flags: needinfo?(standard8)

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

Assignee: nobody → standard8
Status: NEW → ASSIGNED
Iteration: --- → 82.2 - Sep 7 - Sep 20
Points: --- → 3
Flags: needinfo?(standard8)

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

Pushed by mbanner@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch
You need to log in before you can comment on or make changes to this bug.