Closed Bug 1347425 Opened 3 years ago Closed 2 years ago

navigator.userAgent can do sync IPC

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: Ehsan, Assigned: Ehsan)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qf:p1][necko-active])

Attachments

(3 files)

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.
Whiteboard: [qf:p1]
high impact.. please find an owner
Flags: needinfo?(jduell.mcbugs)
Whiteboard: [qf:p1] → [qf:p1][necko-active]
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.
Depends on: 1310335
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.
Assignee: nobody → ehsan
Component: Networking: HTTP → DOM
Flags: needinfo?(jduell.mcbugs)
Actually let's not block on bug 1310335 here.
No longer depends on: 1310335
(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.
(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
See Also: → 1350579
(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!
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.
Attachment #8851275 - Flags: review?(schien)
Attachment #8851275 - Flags: review?(amarchesini)
This used to be the glue layer between DOM and Necko which is no longer
being used.
Attachment #8851277 - Flags: review?(nchen)
Attachment #8851277 - Flags: review?(amarchesini)
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.
Attachment #8851275 - Flags: review?(schien) → review+
(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.  :-)
Attachment #8851276 - Flags: review?(mcmanus) → review+
Attachment #8851277 - Flags: review?(nchen) → review+
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.
Attachment #8851275 - Flags: review?(amarchesini) → review+
Attachment #8851277 - Flags: review?(amarchesini) → review+
(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.
Pushed by eakhgari@mozilla.com:
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
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!
(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...)
https://hg.mozilla.org/mozilla-central/rev/9206126bc232
https://hg.mozilla.org/mozilla-central/rev/c4a14403d17a
https://hg.mozilla.org/mozilla-central/rev/35a915c6babc
Status: NEW → RESOLVED
Closed: 2 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
awesome
Depends on: 1352030
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.