Tim Nguyen :ntim Assignee
	Description • 9 years ago

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.

	Tim Nguyen :ntim Assignee
	Comment 1 • 9 years ago

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

	Jan Keromnes [:janx]
	Comment 2 • 9 years ago

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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 3 • 9 years ago

(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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 4 • 9 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 9 years ago

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.

	Tim Nguyen :ntim Assignee
	Comment 6 • 9 years ago

Just attaching here so I don't lose it.

	Tim Nguyen :ntim Assignee
	Comment 7 • 9 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 8 • 9 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 9 • 9 years ago

removes some unwanted changes from the previous patch

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 9 years ago

Tim, are you still looking for a review?

	Tim Nguyen :ntim Assignee
	Comment 11 • 9 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 9 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 14 • 9 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 9 years ago

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.

	Tim Nguyen :ntim Assignee
	Comment 17 • 9 years ago

I forgot to hg add the test in my previous patch

	Tim Nguyen :ntim Assignee
	Comment 18 • 9 years ago

This fully works !

	Tim Nguyen :ntim Assignee
	Comment 19 • 9 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 9 years ago

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.

	Tim Nguyen :ntim Assignee
	Comment 21 • 9 years ago

(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.

	Tim Nguyen :ntim Assignee
	Comment 22 • 9 years ago

Non working WIP of the network changes.

	Tim Nguyen :ntim Assignee
	Comment 23 • 9 years ago

:bz, Are you ok with this approach ?

	Jason Duell
	Comment 24 • 9 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 9 years ago

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.

	Tim Nguyen :ntim Assignee
	Comment 26 • 9 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 27 • 9 years ago

Thanks for the reviews !

Try push in previous comment.

	Tim Nguyen :ntim Assignee
	Comment 28 • 9 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 29 • 9 years ago

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

	Pulsebot
	Comment 30 • 9 years ago

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

	Pulsebot
	Comment 31 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 32 • 9 years ago

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

	Pulsebot
	Comment 33 • 9 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 34 • 9 years ago

(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.

	Tim Nguyen :ntim Assignee
	Comment 35 • 9 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 36 • 9 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 37 • 9 years ago

> 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?

	Tim Nguyen :ntim Assignee
	Comment 38 • 9 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 39 • 9 years ago

> 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...

	Tim Nguyen :ntim Assignee
	Comment 40 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 41 • 9 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 42 • 9 years ago

Turns out only a small whitespace change failed to apply.

	Pulsebot
	Comment 43 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 44 • 9 years ago

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

	Pulsebot
	Comment 45 • 9 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 46 • 9 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 47 • 9 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 48 • 9 years ago

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

	Pulsebot
	Comment 49 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 50 • 9 years ago
bugherder

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