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

RESOLVED FIXED in Firefox 20

Status

()

defect
P1
normal
RESOLVED FIXED
7 years ago
4 months ago

People

(Reporter: lmandel, Assigned: jdm)

Tracking

unspecified
B2G C4 (2jan on)
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox19 wontfix, firefox20 fixed, firefox21 fixed, b2g18 fixed)

Details

Attachments

(2 attachments, 6 obsolete attachments)

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.
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.
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.
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
Assignee: nobody → jones.chris.g
blocking-basecamp: ? → +
Keywords: regression
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
Target Milestone: --- → B2G C3 (12dec-1jan)
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
Ha ok, sorry for misunderstanding.

Dao, can you check why the fix in bug 800157 could not work here?
Someone equipped for building and running b2g and gaia needs to check if and when this regressed.
(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.
I didn't even notice this was assigned to me.  I'm not the right owner.
Assignee: jones.chris.g → nobody
nsm, can you take a look?
Assignee: nobody → nsm.nikhil
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?
(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!
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.
Attachment #696363 - Flags: review?(dao)
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
Attachment #696366 - Flags: review?(dao)
Seems like a good idea to set gInitialized only once all initialization is done.
Attachment #696367 - Flags: review?(dao)
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.
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 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 on attachment 696367 [details] [diff] [review]
Patch 3: Move gInitialized to end of init()

add an empty line before "gInitialized = true;"
Attachment #696367 - Flags: review?(dao) → review+
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?
Attachment #696363 - Flags: review?(dao) → review-
>  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.
(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.
> 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.
The bug is that the DOM property navigator.userAgent is content processes is not returning the overridden UA.
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...
I'm looking into running the init code in the child 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.
Attachment #697157 - Flags: review?(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?
Attachment #697157 - Flags: review?(dao) → review-
A much more attractive possibility.
Attachment #697157 - Attachment is obsolete: true
Are you going to take this, Josh?
Yep.
Assignee: nsm.nikhil → josh
Target Milestone: B2G C3 (12dec-1jan) → B2G C4 (2jan on)
Attachment #697466 - Flags: review?(jones.chris.g)
Attachment #697466 - Flags: review?(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.
Attachment #697466 - Flags: review?(jones.chris.g) → review+
Attachment #696366 - Flags: review?(dao)
Summary: Regression: navigator.userAgent is not affected by UA override → navigator.userAgent is not affected by UA override in content processes
Priority: -- → P1
Here's the other option. This is the same way the keyboard app injects its tentacles into all apps.
Attachment #698644 - Flags: review?(dao)
Attachment #696367 - Attachment is obsolete: true
Attachment #696366 - Attachment is obsolete: true
Attachment #696363 - Attachment is obsolete: true
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.
Attachment #698644 - Flags: review?(dao) → review-
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.
Attachment #698722 - Flags: review?(21)
Attachment #698644 - Attachment is obsolete: true
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.
Attachment #698722 - Flags: review+
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
Attachment #698722 - Flags: review?(21) → review+
I think we need to apply this fix to Firefox 19 (Aurora) as well to ensure that it makes it into B2G v1.1.
Attachment #697466 - Attachment is obsolete: true
Attachment #697466 - Flags: review?(dao)
(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.
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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.