822551 - navigator.userAgent is not affected by UA override in content processes

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Lawrence Mandel [:lmandel] (use needinfo)]

Bug 800157 made navigator.userAgent use the value specified in a domain UA override. AFAICT, this functionality has regressed as I reported in bug 805663 comment 22. My guess is that the regression was caused by moving the UA override declarations to Gaia in bug 798694 comment 19. This functionality is required for the UA override to be effective on certain sites, like that reported in bug 805663.

Comment 1

[Jason Smith [:jsmith]]

Probably would be worthwhile also looking at other connected properties that should be affected by the override. For example, navigator.platform should be affected as well.

Comment 2

[Dão Gottwald [:dao]]

This still works on desktop Firefox. Can someone reproduce this on b2g and if so, see whether reverting bug 798694 comment 19 fixes it?

(In reply to Jason Smith [:jsmith] from comment #1)
> Probably would be worthwhile also looking at other connected properties that
> should be affected by the override. For example, navigator.platform should
> be affected as well.

The override can be an arbitrary string. We can't reasonably deduce navigator.platform from that.

Comment 3

[Gervase Markham [:gerv]]

navigator.platform is "" in all cases for B2G. If we find sites requiring it to be "Android" or something, we may have to put in a very specific hack, but let's only do that when we find a significant use case.

Gerv

Comment 4

[[:fabrice] Fabrice Desré]

Here's the user.js on a fresh build:

pref("browser.manifestURL", "app://system.gaiamobile.org/manifest.webapp");
pref("browser.homescreenURL", "app://system.gaiamobile.org/index.html");
pref("dom.send_after_paint_to_content", true);
pref("network.http.max-connections-per-server", 15);
pref("ril.debugging.enabled", false);
pref("network.dns.localDomains", "gaiamobile.org,keyboard.gaiamobile.org,communications.gaiamobile.org,system.gaiamobile.org,fm.gaiamobile.org,costcontrol.gaiamobile.org,gallery.gaiamobile.org,feedback.gaiamobile.org,email.gaiamobile.org,clock.gaiamobile.org,calendar.gaiamobile.org,music.gaiamobile.org,sms.gaiamobile.org,bluetooth.gaiamobile.org,browser.gaiamobile.org,homescreen.gaiamobile.org,calculator.gaiamobile.org,camera.gaiamobile.org,wallpaper.gaiamobile.org,pdfjs.gaiamobile.org,video.gaiamobile.org,settings.gaiamobile.org");
pref("dom.payment.provider.0.name", "mockpayprovider");
pref("dom.payment.provider.0.description", "Mock Payment Provider");
pref("dom.payment.provider.0.type", "mock/payments/inapp/v1");
pref("dom.payment.provider.0.uri", "https://mockpayprovider.phpfogapp.com/?req=");
pref("dom.payment.provider.0.requestMethod", "GET");

// Send these sites a custom user-agent.
pref("general.useragent.override.facebook.com", "\(Mobile#(Android; Mobile");
pref("general.useragent.override.youtube.com", "\(Mobile#(Android; Mobile");
pref("general.useragent.override.yelp.com", "\(Mobile#(Android; Mobile");
pref("general.useragent.override.dailymotion.com", "\(Mobile#(Android; Mobile");
pref("general.useragent.override.accounts.google.com", "\(Mobile#(Android; Mobile");
pref("general.useragent.override.maps.google.com", "\(Mobile#(Android; Mobile");


That looks fine to me, and going to yelp.com I get the mobile version, along with a "Get the Yelp app on your Android"... WFM

Comment 5

[Lawrence Mandel [:lmandel] (use needinfo)]

The currently UA override does work for the set of sites that performs UA detection based on the UA that is sent along with each HTTP request. The issue is with sites that perform UA detection in JS using navigator.userAgent. Bug 805663 contains a specific example.

Here is the script that I used to set an override on my stable build. 

https://gist.github.com/4291503

In this case I added a the UA override from bug 805663. With the UA override added to user.js the desktop site is still served. The mobile site is served when overriding the UA globally on B2G. I also tried this with my people account. I added a UA override for people.mozilla.org and hit a page in my people account that displays the UA via navigator.userAgent.
http://people.mozilla.org/~lmandel/navua.html

Comment 6

[[:fabrice] Fabrice Desré]

Ha ok, sorry for misunderstanding.

Dao, can you check why the fix in bug 800157 could not work here?

Comment 7

[Dão Gottwald [:dao]]

Someone equipped for building and running b2g and gaia needs to check if and when this regressed.

Comment 8

[[:fabrice] Fabrice Desré]

(In reply to Dão Gottwald [:dao] from comment #7)
> Someone equipped for building and running b2g and gaia needs to check if and
> when this regressed.

This is not different from running firefox, just do --enable-application=b2g and do a local clone of gaia.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I didn't even notice this was assigned to me.  I'm not the right owner.

Comment 10

[Doug Turner (:dougt)]

nsm, can you take a look?

Comment 11

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

I can't reproduce this build on a latest b2g (hg tip 4f74d77d6d8b) and gaia (7c07c4bf026).

Running the desktop emulator version of b2g and gaia
Modifying profile/user.js
ensures that UA change is reflected in navigator.userAgent (used lmandel's people page for test)

Did I understand the report incorrectly?

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Did you enable out-of-process content? [1]

[1] http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#241

Comment 13

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #12)
> Did you enable out-of-process content? [1]
> 
> [1] http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#241

that did it. thanks!

Comment 14

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Call UserAgentOverrides.init() from SiteSpecificUserAgent constructor

When dom.ipc.tabs.disabled = false, and the DOM code is running in a child process, UserAgentOverrides.init() has not been called. The list of overrides isn't loaded from preferences.

Comment 15

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 2: Catch error on registering for http-on-modify-request

Bug 806753 removed support for child processes to add observers on http-on-* messages. This leads to AddObserver() returning an error. So even with UserAgentOverrides.init() being called by SiteSpecificUserAgent constructor, the initialization hasn't actually happened.

http://hg.mozilla.org/mozilla-central/rev/4525ca87c142

Comment 16

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 3: Move gInitialized to end of init()

Seems like a good idea to set gInitialized only once all initialization is done.

Comment 17

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

So this regression had two causes. I haven't tried to track down the cause of UAO.init() never being called when tabs are separate processes, since I think that isn't a regression but just a case that wasn't handled before.

On the other hand, fixing bug 806753 did cause a regression in the addObserver call to http-on-modify-request as noted in comment 15.

Here is a matrix of results I got before and after bug 806753

UA declarations already moved to gaia, so 798694 is not the cause.

before applying fix for bug 806753
  in process tab
     no UAO init in SiteSpecificUserAgent constructor - site specific user agent works
     init - works
  out process tab
     no init - does not work
     init - works

after applying fix for bug 806753
  in process tab
     no init - works
     init - works
  out process tab
     no init - does not work
     init - does not work
     init AND try-catch - works

I'm not sure if there is a cleaner approach than surrounding the block by try-catch, maybe a check in JS to detect if this is a child.

Comment 18

[Josh Matthews [:jdm]]

init is currently only called in http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#263, which I presume only runs in the chrome process when OOP is enabled.

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 696366 [details] [diff] [review]
Patch 2: Catch error on registering for http-on-modify-request

Can you add a comment to this code noting when and why it would throw?

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 696367 [details] [diff] [review]
Patch 3: Move gInitialized to end of init()

add an empty line before "gInitialized = true;"

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 696363 [details] [diff] [review]
Call UserAgentOverrides.init() from SiteSpecificUserAgent constructor

Looks like this would cause the UserAgentOverrides module to monitor HTTP requests in any Gecko application even if there are no overrides, which seems undesirable. Can you call UserAgentOverrides.init() for the child process in b2g code?

Comment 22

[Jason Duell]

>  out of process tab
>     init AND try-catch - works

I don't understand how the code works in this case.  Registering for http-on-modify-request will fail on the child, so while you don't avoid an error if you catch it, you still don't have a observer called for modifying the UA header in the child process.  Are we modifying it in the parent somehow? (I assume not, else we'd already have working behavior).  So I'm confused.

If you need http-on-modify-request in the child, we can make the work again--just let me know.  It's broken for cookies (which is why I turned it off), but that's moot here.

> Looks like this would cause the UserAgentOverrides module to monitor HTTP requests 
> in any Gecko application even if there are no overrides... Can you call 
> UserAgentOverrides.init() for the child process in b2g code?

I think the most logical thing is to only add the http-on-request observer if PREF_OVERRIDES_ENABLED=true.  I.e. base it on the pref, not whether we're in b2g code.  But either way is fine I guess.

Comment 23

[Josh Matthews [:jdm]]

(In reply to Jason Duell (:jduell) from comment #22)
> >  out of process tab
> >     init AND try-catch - works
> 
> I don't understand how the code works in this case.  Registering for
> http-on-modify-request will fail on the child, so while you don't avoid an
> error if you catch it, you still don't have a observer called for modifying
> the UA header in the child process.  Are we modifying it in the parent
> somehow? (I assume not, else we'd already have working behavior).  So I'm
> confused.

The UAO module is also initialized successfully in the parent, so it watches the requests there and modifies them.

Comment 24

[Jason Duell]

> The UAO module is also initialized successfully in the parent, so it watches the 
> requests there and modifies them.

So what's our bug here, then?  It's unclear to me what the child needs to do here if the parent process is already doing the UA override.

Also: I'm seeing this WFM for yelp.com when I use my unagi phone, but when I use b2g desktop I don't get a UA override and I get the regular Yelp desktop site.  Any reason things would be different between phone/b2g-desktop?

Finally, if we do want the fixes here (again, I'm not clear we do), then I may be wrong in comment 22 about registering for http-on-modify-request if PREF_OVERRIDES_ENABLED=true.  It turns out we always set "general.useragent.site_specific_overrides=true" for some reason (perhaps a bad reason?  It seems kludgy :):

  http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/init/all.js#27
  
Assuming there's good cause for that, we should do as Dao suggests and only init UserAgentOverrides in B2G code, not to base it on on the pref.  Alternately, we could only register the http-on-modify observer if we find we actually have overrides to set (i.e. if 'gOverrides' is not empty).  AFAICT we're only adding entries via ua-override-prefs.js which only exists in the gaia repo.  Perhaps that's the best solution.

Comment 25

[Josh Matthews [:jdm]]

The bug is that the DOM property navigator.userAgent is content processes is not returning the overridden UA.

Comment 26

[Jason Duell]

Attachment: stack of crash when accessing test page: http://www.joshmatthews.net/ua.html

Thanks for clarification on bug jdm :)

So I just tried to test navigator.userAgent access in the child with the patches here applied, using jdm's test web page that just does "alert(navigator.userAgent)"

  http://www.joshmatthews.net/ua.html

It fails with the attached stack trace--looks like it tries to process the alert and then dies during reflow, for reasons that I'm guessing are unrelated to this bug, but just in case...   This was with a unagi debug build built off the b2g18 tree.   

I can't test on desktop because the UA overrides aren't working there--filed bug 825713 for that.

I'm moving on to other bugs for now...

Comment 27

[Josh Matthews [:jdm]]

I'm looking into running the init code in the child processes.

Comment 28

[Josh Matthews [:jdm]]

Attachment: Ensure the UserAgentOverride is initialized just-in-time in content processes.

It's not pretty, but trying to load a content script to call UserAgentOverride.init for every single new process that is launched appears to be a losing game, as I can't figure out how to do it.

Comment 29

[Dão Gottwald [:dao]]

Comment on attachment 697157 [details] [diff] [review]
Ensure the UserAgentOverride is initialized just-in-time in content processes.

This seems more opaque and therefore in a way even worse than attachment 696363 [details] [diff] [review]. A combination of the two approaches might be ok, but explicitly initializing UserAgentOverrides in b2g code would still be preferable.

(In reply to Josh Matthews [:jdm] from comment #28)
> It's not pretty, but trying to load a content script to call
> UserAgentOverride.init for every single new process that is launched appears
> to be a losing game, as I can't figure out how to do it.

Who could help with this?

Comment 30

[Josh Matthews [:jdm]]

Attachment: Ensure the UserAgentOverride is initialized just-in-time in content processes.

A much more attractive possibility.

Comment 31

[Andrew Overholt [:overholt]]

Are you going to take this, Josh?

Comment 32

[Josh Matthews [:jdm]]

Yep.

Comment 33

[Dão Gottwald [:dao]]

Comment on attachment 697466 [details] [diff] [review]
Ensure the UserAgentOverride is initialized just-in-time in content processes.

Before reviewing this, I'd like to have some clarification on the feasibility of loading a b2g content script to initialize UserAgentOverrides.

Comment 34

[Josh Matthews [:jdm]]

Attachment: Initialize the user agent overrides in content process frames.

Here's the other option. This is the same way the keyboard app injects its tentacles into all apps.

Comment 35

[Dão Gottwald [:dao]]

Comment on attachment 698644 [details] [diff] [review]
Initialize the user agent overrides in content process frames.

>--- a/netwerk/protocol/http/UserAgentOverrides.jsm
>+++ b/netwerk/protocol/http/UserAgentOverrides.jsm

>+    Services.obs.addObserver(remote_frame_shown, "remote-browser-frame-shown", false);

>+const kFrameScript = "chrome://global/content/UAO_child.js";
>+function remote_frame_shown(aSubject, aTopic, aData) {
>+  let frameLoader = aSubject.QueryInterface(Ci.nsIFrameLoader);
>+  let mm = frameLoader.messageManager;
>+
>+  try {
>+    mm.loadFrameScript(kFrameScript, true);
>+  } catch (e) {
>+    dump('Error loading ' + kFrameScript + ' as frame script: ' + e + '\n');
>+  }
>+}

Put this and UAO_child.js somewhere in /b2g (/b2g/components/ProcessGlobal.js would be my first bet) -- it doesn't belong in here.

Comment 36

[Josh Matthews [:jdm]]

Attachment: Initialize the user agent overrides in content process frames.

Given that Dao already approved the changes Nikhil made to UAO.jsm, I think only Vivien needs to sign off on the ProcessGlobal changes now.

Comment 37

[Dão Gottwald [:dao]]

Comment on attachment 698722 [details] [diff] [review]
Initialize the user agent overrides in content process frames.

This looks way cleaner to me, thanks. And yes, the remaining UserAgentOverrides.jsm changes are still fine.

Comment 38

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 698722 [details] [diff] [review]
Initialize the user agent overrides in content process frames.

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

::: b2g/chrome/jar.mn
@@ +40,5 @@
>    content/images/mute-hdpi.png         (content/images/mute-hdpi.png)
>    content/images/throbber.png          (content/images/throbber.png)
>    content/images/error.png             (content/images/error.png)
> +
> +  content/UAO_child.js                 (content/UAO_child.js)

nit: let's declare it closer to the other JS files

Comment 39

[Josh Matthews [:jdm]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/134ed7c6275c
https://hg.mozilla.org/mozilla-central/rev/3966def5b219

Comment 40

[Lawrence Mandel [:lmandel] (use needinfo)]

I think we need to apply this fix to Firefox 19 (Aurora) as well to ensure that it makes it into B2G v1.1.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

(In reply to Lawrence Mandel [:lmandel] from comment #40)
> I think we need to apply this fix to Firefox 19 (Aurora) as well to ensure
> that it makes it into B2G v1.1.

I sure hope not, because the workweek rules were that all Gecko patches were only landing on inbound and b2g18.

Comment 42

[Lawrence Mandel [:lmandel] (use needinfo)]

We should go with your understanding based on the work week rules. I'll follow-up to ensure that I'm clear on our path forward.