1137681 - Add a way to emulate the user agent per tab

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Tim Nguyen :ntim]

The prototype in bug 1028905 is quite hacky and relies on some global browser preferences (and doesn't work on remote targets).
We need an actor that will let us spoof user agents.

Comment 1

[Tim Nguyen :ntim]

Possibly useful link : https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/UserAgentOverrides.jsm

Comment 2

[Jan Keromnes [:janx]]

Note: As discussed on IRC, I think it's not yet possible to spoof UserAgent strings for a devtools target only.

The way popular Firefox addons do it is by setting the "general.useragent.override" pref, as you did in your prototype, which changes the UA for the whole browser, or to set prefs like "general.useragent.override.mozilla.org" to overrite UAs on a per-domain basis.

An actor to change the global UA pref would be good for now, but eventually it would be great to spoof UA for only one document.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Jan Keromnes [:janx] from comment #2)
> Note: As discussed on IRC, I think it's not yet possible to spoof UserAgent
> strings for a devtools target only.
> 
> The way popular Firefox addons do it is by setting the
> "general.useragent.override" pref, as you did in your prototype, which
> changes the UA for the whole browser, or to set prefs like
> "general.useragent.override.mozilla.org" to overrite UAs on a per-domain
> basis.
> 
> An actor to change the global UA pref would be good for now, but eventually
> it would be great to spoof UA for only one document.

In the end, per document will likely require platform changes to allow such a setting.  Maybe someone like bz or others can tell us how difficult such a change would be.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I did some more investigation after writing comment 3.  There are several components that implement today's behavior of "custom UA per domain":

1. There is site-specific UA service that is consulted[1] (for chrome callers only, I suppose this would need to change?)
2. This component asks[2] UserAgentOverrides if there is an override, and only caches a mapping per host
3. UserAgentOverrides[3] stores the active overrides per domain

So, on the surface at least, it seems like the main change is improving these components to be window / tab specific, instead of operating on a whole domain.

[1]: https://hg.mozilla.org/mozilla-central/annotate/ba44099cbd07/dom/base/Navigator.cpp#l2734
[2]: https://hg.mozilla.org/mozilla-central/annotate/ba44099cbd07/dom/base/SiteSpecificUserAgent.js#l40
[3]: https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/UserAgentOverrides.jsm

Comment 5

[Boris Zbarsky [:bzbarsky]]

So there are two things that matter for "spoofing the user agent".  One is what navigator.userAgent returns.  The other is what UA strings are sent on the wire.

For the former, comment 4 describes the data flow.  Note that SiteSpecificUserAgent.js is passed the URI of the page _and_ the window.  So it could in fact do window-specific or tab-specific bits there if it wanted to.  We'd need to change Navigator::GetUserAgent to ignore the UA override pref if in this devtools spoofing mode, I guess.

For the latter, that's handled entirely in UserAgentOverrides.jsm.  See the HTTP_on_modify_request function there.  That would need to be changed to examine the window in addition to the channel URI.  Or something.  It's not clear to me which process this stuff runs in in the e10s world and therefore whether it actually has access to a window when we're doing e10s.

Comment 6

[Tim Nguyen :ntim]

Attachment: WIP platform support

Just attaching here so I don't lose it.

Comment 7

[Tim Nguyen :ntim]

Attachment: Patch v0.1

This should work thanks to :bz's help. Still needs tests though

Comment 8

[Tim Nguyen :ntim]

Attachment: Patch v0.2

This feels ready for review. Added a mochitest for the functionality. The necko part remains untested though.

Comment 9

[Tim Nguyen :ntim]

Attachment: Patch v0.2

removes some unwanted changes from the previous patch

Comment 10

[Boris Zbarsky [:bzbarsky]]

Tim, are you still looking for a review?

Comment 11

[Tim Nguyen :ntim]

(In reply to Boris Zbarsky [:bz] from comment #10)
> Tim, are you still looking for a review?

Well, it's not going to be really useful because most of the patch will become obsolete once I use the docshell to store the custom UA. I guess you could give me coding style feedback though.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Oh, right.  I can wait for the docshell version.  :)

Comment 13

[Tim Nguyen :ntim]

Attachment: Docshell WIP

Comment 14

[Tim Nguyen :ntim]

Comment on attachment 8636616 [details] [diff] [review]
Docshell WIP

This works, except you need to reload the page before the UA applies. I can't figure out how to clear the UA cache from the docshell, if you could help me out with that, that would be awesome.

Also, the test needs to be updated, but I just want to make sure you're OK with the main code first.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8636616 [details] [diff] [review]
Docshell WIP

The simplest way to clear the cached value on navigator is probably something like this:

  nsRefPtr<nsGlobalWindow> win = mScriptGlobal ?
    mScriptGlobal->GetCurrentInnerWindowInternal() : nullptr;
  if (win) {
    ErrorResult ignored;
    Navigator* navigator = win->GetNavigator(ignored);
    ignored.SuppressException();
    if (navigator) {
      // Call a method to clear the cache here; in the end you want to end up
      // calling NavigatorBinding::ClearCachedUserAgentValue, but that's
      // probably better done from inside Navigator itself.
    }
  }

Past that:

1)  You need to change the iid of the docshell interface.
2)  Null-checking mWindow should happen _before_ you start calling methods on
    it.
3)  Don't name function locals with names starting with 'm'.  That's for member
    variables.

Comment 16

[Tim Nguyen :ntim]

Attachment: Docshell patch v2

The part I use to transmit the UA to existing child frames causes a browser crash. I honestly have no idea why, but I guess it's something very dumb, since I copy-pasted thoughtlessly most of the code. Boris, Can you take a look ? Thanks ! :)

Comment 17

[Tim Nguyen :ntim]

Attachment: Docshell patch v2.1

I forgot to hg add the test in my previous patch

Comment 18

[Tim Nguyen :ntim]

Attachment: Docshell patch v3

This fully works !

Comment 19

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f108dba999af

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8687542 [details] [diff] [review]
Docshell patch v3

>+  RefPtr<nsDocShell> docShell = this;

Why do you need this variable?  You could just do:

  GetChildCount(&childCount);

and inside the loop:

  GetChildAt(i, getter_AddRefs(childItem));

But what you really want to do is this:

  uint32_t childCount = mChildList.Length();
  for (int32_t i = 0; i < childCount; ++i) {
    nsCOMPtr<nsIDocShell> childShell = do_QueryInterface(ChildAt(i));
    if (childShell) {
      childShell->SetCustomUserAgent(aCustomUserAgent);
    }
  }

instead of using the XPCOM getters for child stuff.

>+++ b/docshell/base/nsIDocShell.idl

You need to update the iid.  I mentioned this the last time I was looking at this patch.

>+var test = Task.async(function*() {
>+  waitForExplicitFinish();

Shouldn't waitForExplicitFinish() be outside the async section?

>+  is(frameWin.navigator.userAgent, "foo", "The UA should passed on to frames.");

"should be passed on".

>+  is(newFrameWin.navigator.userAgent, "foo", "New UA should persist across reloads");

This isn't testing anything, since reload() is async and hasn't happened yet.

>+    }, true);

Are you sure you want a _capturing_ listener, not a bubbling one?  If so, please at least document why.

>+    nsIDocShell* mDocShell = mWindow->GetDocShell();

Again, please don't name local variables with names starting with "m".

>+      nsIDocument* doc = mWindow->GetExtantDoc();

What's the point of doing that if we have a custom user agent?

Seems to me like the logic here should go like this:

  if (docShell) {
    // Try to get custom user agent, return if found.
    // Now get the doc and its principal URI for the GetUserAgent call.
  }

A question: normal user-agent overrides don't affect chrome callers.  Should this custom user agent thing do so?

>+  void ClearUserAgentCache();

Document, please.

This setup doesn't propagate the useragent override to workers, so navigator.userAgent in a worker will ignore it (it's calling the 4-arg version of Navigator::GetUserAgent directly).  Is that intentional?  If not, you should probably push your code down into said 4-arg version... and add something that would clear the cache on the worker.  You may be able to model it after how ClearCachedLanguagesValue() calls are done.

I assume you've checked that the UserAgentOverrides.jsm changes work in both e10s and non-e10s mode?

r=me with the above fixed.

Comment 21

[Tim Nguyen :ntim]

Attachment: Docshell patch v4

(In reply to Boris Zbarsky [:bz] from comment #20)
> Comment on attachment 8687542 [details] [diff] [review]
> Docshell patch v3
> 
> >+  RefPtr<nsDocShell> docShell = this;
> 
> Why do you need this variable?  You could just do:
> 
>   GetChildCount(&childCount);
> 
> and inside the loop:
> 
>   GetChildAt(i, getter_AddRefs(childItem));
> 
> But what you really want to do is this:
> 
>   uint32_t childCount = mChildList.Length();
>   for (int32_t i = 0; i < childCount; ++i) {
>     nsCOMPtr<nsIDocShell> childShell = do_QueryInterface(ChildAt(i));
>     if (childShell) {
>       childShell->SetCustomUserAgent(aCustomUserAgent);
>     }
>   }
> 
> instead of using the XPCOM getters for child stuff.
Done, thanks for the advice !

> >+++ b/docshell/base/nsIDocShell.idl
> 
> You need to update the iid.  I mentioned this the last time I was looking at
> this patch.
Done.

> >+var test = Task.async(function*() {
> >+  waitForExplicitFinish();
> 
> Shouldn't waitForExplicitFinish() be outside the async section?
Yep, fixed.

> >+  is(frameWin.navigator.userAgent, "foo", "The UA should passed on to frames.");
> 
> "should be passed on".
Done.

> >+  is(newFrameWin.navigator.userAgent, "foo", "New UA should persist across reloads");
> 
> This isn't testing anything, since reload() is async and hasn't happened yet.
Fixed.

> >+    }, true);
> 
> Are you sure you want a _capturing_ listener, not a bubbling one?  If so,
> please at least document why.
It seems that we need the capturing event listener (I didn't write the test structure). Not sure why it's needed. I don't think it's worth writing a comment, since a lot of other tests use the same pattern as well, without explanation. But if you insist, I can ask why it's needed and add a comment.

> >+    nsIDocShell* mDocShell = mWindow->GetDocShell();
> 
> Again, please don't name local variables with names starting with "m".
Sorry, missed that from the previous review. Fixed.

> >+      nsIDocument* doc = mWindow->GetExtantDoc();
> 
> What's the point of doing that if we have a custom user agent?
>
> Seems to me like the logic here should go like this:
> 
>   if (docShell) {
>     // Try to get custom user agent, return if found.
>     // Now get the doc and its principal URI for the GetUserAgent call.
>   }
> 
> A question: normal user-agent overrides don't affect chrome callers.  Should
> this custom user agent thing do so?
Yes, it should, AFAIK, it doesn't heavily break the browser doing so (most if not all of the platform checks are done using AppConstants.jsm).

> >+  void ClearUserAgentCache();
> 
> Document, please.
Fixed.

> This setup doesn't propagate the useragent override to workers, so
> navigator.userAgent in a worker will ignore it (it's calling the 4-arg
> version of Navigator::GetUserAgent directly).  Is that intentional?  If not,
> you should probably push your code down into said 4-arg version... and add
> something that would clear the cache on the worker.  You may be able to
> model it after how ClearCachedLanguagesValue() calls are done.
I've moved the logic into the 4-arg version. I've filed bug 1226946 for the worker changes.

> I assume you've checked that the UserAgentOverrides.jsm changes work in both
> e10s and non-e10s mode?
Turns out they don't work at all. Will post a network patch with the needed changes.

Comment 22

[Tim Nguyen :ntim]

Attachment: Network patch WIP

Non working WIP of the network changes.

Comment 23

[Tim Nguyen :ntim]

Attachment: Network patch v2

:bz, Are you ok with this approach ?

Comment 24

[Jason Duell]

Comment on attachment 8693800 [details] [diff] [review]
Network patch v2

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

bz: so I told Tim to just add the SetDocshellUserAgentOverride() calls to {nsHttpChannel|HttpChannelChild}::AsyncOpen, because using UserAgentOverrides.jsm seemed iffy for a couple reasons: 1) the performance issues we hit in bug 896114 (though those seem to have been mitigated in bug 896382: but I don't know how much).  2) we currently don't use UserAgentOverrides.jsm in Firefox desktop any more (bug 896114), though maybe it wouldn't be a big deal to turn it back on?  3) We currently don't support any of the on-XXX-request notifications in the child, so to make UserAgentOverrides.jsm work we would have needed to turn them on (possibly not a big deal, but I remember bsmith advocating heavily for not turning them on for security reasons I can't recall now).

This approach seemed the fastest way to get this working.  On the down side, if there happens to be a site-specific UA override, it will stomp over our devtools override with this approach (but there seem to be very very few per-site overrides, and none on desktop).

I'm ok with the .jsm approach if you think this one isn't good enough.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8693800 [details] [diff] [review]
Network patch v2

I'm sorry for the lag here...

>+            nsAutoCString utf8CustomUserAgent;
>+            CopyUTF16toUTF8(customUserAgent, utf8CustomUserAgent);

This is probably more concise as:

  NS_ConvertUTF16toUTF8 utf8CustomUserAgent(customUserAgent);

and in general this method could perhaps use more early returns than deep nesting.  

The general approach seems fine to me.  Perhaps add a comment about how site-specific overrides might override this, since it's not obvious from the code.

Comment 26

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cf718b32f70

Comment 27

[Tim Nguyen :ntim]

Attachment: Part 2 - Network changes

Thanks for the reviews !

Try push in previous comment.

Comment 28

[Tim Nguyen :ntim]

Attachment: Part 1: Docshell changes (v5)

Fixed build bustage, it's weird that it was only a warning locally though.

Comment 29

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f493c7c6afa

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/720b3adb1df5
https://hg.mozilla.org/integration/fx-team/rev/7a9e01e904b0

Comment 31

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/3cd5acd350fb

Comment 32

[Carsten Book [:Tomcat]]

backed this out since this seems have caused https://treeherder.mozilla.org/logviewer.html#?job_id=6025874&repo=fx-team

Comment 33

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/596f292fe07f

Comment 34

[Tim Nguyen :ntim]

(In reply to Carsten Book [:Tomcat] from comment #32)
> backed this out since this seems have caused
> https://treeherder.mozilla.org/logviewer.html#?job_id=6025874&repo=fx-team

Working on a test fix.

Comment 35

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdc2066d3f38

Comment 36

[Tim Nguyen :ntim]

Attachment: Part 1 - Docshell changes (v6)

Fixes test by moving back the Navigator.cpp logic into the 1 arg function.

Mochitest-e10s try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9994f757bcb4

Complete try push (only for diligence, the one in comment 29 should be enough): https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdc2066d3f38

Comment 37

[Boris Zbarsky [:bzbarsky]]

> Fixes test by moving back the Navigator.cpp logic into the 1 arg function.

Why?  Is aWindow null in the worker case in the 4-arg version?

Comment 38

[Tim Nguyen :ntim]

(In reply to Boris Zbarsky [:bz] from comment #37)
> > Fixes test by moving back the Navigator.cpp logic into the 1 arg function.
> 
> Why?  Is aWindow null in the worker case in the 4-arg version?

We discussed about this on IRC, I'll take a look in bug 1229864.

Comment 39

[Boris Zbarsky [:bzbarsky]]

> We discussed about this on IRC, I'll take a look in bug 1229864.

I would really prefer we understand what's going on here before landing more code...

Comment 40

[Tim Nguyen :ntim]

:bz gave me a green light via IRC to land this patch. 
The DOM window isn't being passed as mentioned in bug 1229864.

Comment 41

[Carsten Book [:Tomcat]]

part 2 failed to apply:

adding 1137681 to series file
renamed 1137681 -> ua-network-override.patch
applying ua-network-override.patch
patching file netwerk/protocol/http/HttpBaseChannel.cpp
Hunk #7 FAILED at 3163
1 out of 7 hunks FAILED -- saving rejects to file netwerk/protocol/http/HttpBaseChannel.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh ua-network-override.patch

Comment 42

[Tim Nguyen :ntim]

Attachment: Part 2 - Network changes

Turns out only a small whitespace change failed to apply.

Comment 43

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e2708b9fdadc
https://hg.mozilla.org/integration/fx-team/rev/a144e5190618

Comment 44

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures in https://treeherder.mozilla.org/logviewer.html#?job_id=6136954&repo=fx-team

Comment 45

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/1720c214ebeb
https://hg.mozilla.org/integration/fx-team/rev/64fc74a36a16

Comment 46

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=28ae87980a02

Comment 47

[Tim Nguyen :ntim]

Attachment: Part 1 - Docshell changes (v7)

The test didn't execute the load event listener as the async task wasn't written properly, I should have used yield to wait for the load event. Also switched to add_task() as :pbro suggested.

Mochitest-e10s-bc: https://treeherder.mozilla.org/#/jobs?repo=try&revision=28ae87980a02

Comment 48

[Tim Nguyen :ntim]

Complete try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e9212f4d223

Comment 49

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1cdb9a03dac2
https://hg.mozilla.org/integration/fx-team/rev/4747283cf161

Comment 50

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1cdb9a03dac2
https://hg.mozilla.org/mozilla-central/rev/4747283cf161