1347425 - navigator.userAgent can do sync IPC

Status: RESOLVED FIXED

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

Description

[(no longer active)]

See this profile <https://perfht.ml/2mYerI9> where the sync IPC is taking 861ms.

This happens from ssua_getUserAgentForURIAndWindow() through this code <http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/dom/base/SiteSpecificUserAgent.js#50>.

This API is extremely popular on the Web for UA detection, we need to fix this to avoid the sync IPC.  We need to avoid the sync IPC here at all costs.

Comment 1

[Patrick McManus [:mcmanus]]

high impact.. please find an owner

Comment 2

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

One idea: every time when a HttpChannel is opened for a document, we can piggyback the corresponding default UA string and try flush it to the userAgentCache in SiteSpecificUserAgent.js. In this case we can at least reduce the chances of doing sync IPC in most of the cases.

Comment 3

[(no longer active)]

I have a better idea: we can just check the pref directly, without going through the Necko layer at all and doing any IPC here.  Should be fairly straightforward.

Comment 4

[(no longer active)]

Actually let's not block on bug 1310335 here.

Comment 5

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to :Ehsan Akhgari (busy) from comment #3)
> I have a better idea: we can just check the pref directly, without going
> through the Necko layer at all and doing any IPC here.  Should be fairly
> straightforward.

UA override is more than just reading prefs. We have a JSON file "ua-update.json" that stores a list of UA override template for solving WebCompat issue. We'll need to provide async IPC for updating ua-update.json as well.

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #5)
> (In reply to :Ehsan Akhgari (busy) from comment #3)
> > I have a better idea: we can just check the pref directly, without going
> > through the Necko layer at all and doing any IPC here.  Should be fairly
> > straightforward.
> 
> UA override is more than just reading prefs. We have a JSON file
> "ua-update.json" that stores a list of UA override template for solving
> WebCompat issue. We'll need to provide async IPC for updating ua-update.json
> as well.

References:
[1] https://searchfox.org/mozilla-central/source/netwerk/protocol/http/UserAgentUpdates.jsm
[2] https://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/netwerk/protocol/http/UserAgentOverrides.jsm#54

Comment 7

[(no longer active)]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #5)
> (In reply to :Ehsan Akhgari (busy) from comment #3)
> > I have a better idea: we can just check the pref directly, without going
> > through the Necko layer at all and doing any IPC here.  Should be fairly
> > straightforward.
> 
> UA override is more than just reading prefs. We have a JSON file
> "ua-update.json" that stores a list of UA override template for solving
> WebCompat issue. We'll need to provide async IPC for updating ua-update.json
> as well.

My patches were not affecting Android, which is the only place where ua-update.json is used...  But actually, Shi-Chiang in a private email thread mentioned a smart suggestion: just looking at the User-Agent request header from the document header which is already available, and has already been subject to the UA override in the parent process if necessary, and that approach can be applied on both desktop and Android similarly!  Thank you for that suggestion!

Comment 8

[(no longer active)]

Attachment: Part 1: Move site-specific Navigator.userAgent overrides to a more efficient C++ implementation

Necko computes this information in order to set the User-Agent header in
the parent process.  This header is set on all outgoing requests, and
therefore in the content process we can easily copy the value of this
header from the document channel object instead of the inefficient sync
IPC that currently happens in order to access the JavaScript
implementation living in the parent process.

Comment 9

[(no longer active)]

Attachment: Part 2: Add a test for replacing multiple occurrences of a regex in the site-specific UA override

Comment 10

[(no longer active)]

Attachment: Part 3: Remove the site-specific user agent service

This used to be the glue layer between DOM and Necko which is no longer
being used.

Comment 11

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8851275 [details] [diff] [review]
Part 1: Move site-specific Navigator.userAgent overrides to a more efficient C++ implementation

Review of attachment 8851275 [details] [diff] [review]:
-----------------------------------------------------------------

For document loaded by HTTP this optimization looks good to me, however we should keep the original code for other protocols.

::: dom/base/Navigator.cpp
@@ +1922,5 @@
>      return NS_OK;
>    }
> +  nsCOMPtr<nsIHttpChannel> httpChannel =
> +    do_QueryInterface(doc->GetChannel());
> +  if (httpChannel) {

What if this document is not loaded by HttpChannel, e.g. file://. I'll suggest to preserve the original behavior for other protocols.

Comment 12

[(no longer active)]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #11)
> ::: dom/base/Navigator.cpp
> @@ +1922,5 @@
> >      return NS_OK;
> >    }
> > +  nsCOMPtr<nsIHttpChannel> httpChannel =
> > +    do_QueryInterface(doc->GetChannel());
> > +  if (httpChannel) {
> 
> What if this document is not loaded by HttpChannel, e.g. file://. I'll
> suggest to preserve the original behavior for other protocols.

As far as I can tell this does preserve the behavior, since asciiHost here <http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/netwerk/protocol/http/UserAgentOverrides.jsm#75> for a file:// URI would be an empty string so getOverrideForURI() would return null, so we would currently fall back to the default User-Agent string in such cases.  :-)

Comment 13

[Andrea Marchesini [:baku]]

Comment on attachment 8851275 [details] [diff] [review]
Part 1: Move site-specific Navigator.userAgent overrides to a more efficient C++ implementation

Review of attachment 8851275 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Navigator.cpp
@@ +1913,5 @@
>      return NS_OK;
>    }
>  
>    MOZ_ASSERT(aWindow->GetDocShell());
>  

Can we get rid of nsISiteSpecificUserAgent ?

::: dom/workers/WorkerNavigator.cpp
@@ +144,1 @@
>                                                 isCallerChrome, mUA);

move this up.

Comment 14

[(no longer active)]

(In reply to Andrea Marchesini [:baku] from comment #13)
> Comment on attachment 8851275 [details] [diff] [review]
> Part 1: Move site-specific Navigator.userAgent overrides to a more efficient
> C++ implementation
> 
> Review of attachment 8851275 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/Navigator.cpp
> @@ +1913,5 @@
> >      return NS_OK;
> >    }
> >  
> >    MOZ_ASSERT(aWindow->GetDocShell());
> >  
> 
> Can we get rid of nsISiteSpecificUserAgent ?

Part 3 does this, FWIW.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9206126bc232
Part 1: Move site-specific Navigator.userAgent overrides to a more efficient C++ implementation; r=baku,schien
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4a14403d17a
Part 2: Add a test for replacing multiple occurrences of a regex in the site-specific UA override; r=mcmanus
https://hg.mozilla.org/integration/mozilla-inbound/rev/35a915c6babc
Part 3: Remove the site-specific user agent service; r=baku,jchen

Comment 16

[(no longer active)]

So some of the discussion that we had around how to fix the bug happened in a private email thread, and I just realized that I didn't update the bug after comment 3 based on what happened...  I was going ahead with the idea of reading the pref, and I came up with a patch that did the pref processing in the content process, but I ran into bug 1350579 (the patch I had at the time is attached there) and I sort of got stuck and was about to reach out to the JS team when SC came up with the great suggestion of using the User-Agent header which is already available on the document's channel (which itself has been computed by this very code originally in the parent process!), so even my solution was a quite backwards way of doing this.  SC's solution which is what I ended up implementing came out correct on the first try, super simple and makes perfect sense without needing tons of comments, and I'm super happy with the resulting code.  Thank you so much for the great suggestion!

Comment 17

[(no longer active)]

(And now that I think about this, the part 2 here doesn't even make sense with the new part 1, but I guess more tests never hurt...)

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9206126bc232
https://hg.mozilla.org/mozilla-central/rev/c4a14403d17a
https://hg.mozilla.org/mozilla-central/rev/35a915c6babc

Comment 19

[Patrick McManus [:mcmanus]]

awesome