1127448 - Add youtube.com UA string override that includes Firefox build number for Windows Beta builds (only)

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P1)

Description

[Chris Peterson [:cpeterson]]

We'd like YouTube to track metrics for specific beta builds to test our video fixes. Unfortunately, their metrics system seems to be limited to server-side
User-Agent header filtering so we would like to add youtube.com UA string override that includes Firefox beta build version.

We will want to uplift this patch to Beta 36.

Mike Taylor replied:

Firefox for Android also has an override mechanism that they mainly use for "Desktop mode" (which spoofs a Linux desktop UA) [1], using the same plumbing that B2G uses [2].

[1] <http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3045>
[2] <http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/UserAgentOverrides.jsm>

Comment 1

[Chris Peterson [:cpeterson]]

[Tracking Requested - why for this release]:

We will want this UA override in Beta 36 because YouTube is collecting their MSE playback metrics for Beta 36 users.

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Ralph - do you have time to look into this?

Comment 3

[Ralph Giles (:rillian)]

I'm off tomorrow. ni me again if you want me to look monday morning.

Comment 4

[Chris Peterson [:cpeterson]]

Richard: what is the YouTube domain name that Firefox should whitelist for our User-Agent string override? We don't want to send this nonstandard UA string to more servers than necessary. :)

To limit scope, this UA override should be #ifdef'd only for Beta channel (not Nightly or Aurora) on Windows.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

MSE won't ship in 36. Tracking for 37.

Comment 6

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Raplh - do you have time to look into this?

Comment 7

[Ralph Giles (:rillian)]

Sure.

Comment 8

[Ralph Giles (:rillian)]

Attachment: Append the build id to the ua string for youtube

This seems to work. Cargo-culted from the t.co override in fennec's desktop mode support. Guessing on the reviewer too. :)

Does this need privacy review? It leaks higher resolution version information than our normal user agent string to youtube, and to any observers when that domain is visited without TLS. That's not ideal, but I don't think it has a serious impact on user privacy.

We missed 36 for this feature, but would like to uplift to 37.

Comment 9

[Chris Peterson [:cpeterson]]

Comment on attachment 8564379 [details] [diff] [review]
Append the build id to the ua string for youtube

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

Since we are limiting the build id to YouTube sites and Firefox's pre-release channels, I'm not too concerned about browser fingerprinting. :)

::: browser/base/content/browser.js
@@ +7932,5 @@
> +// Modify the user agent string for specific domains
> +// to route debug information through their logging.
> +var DebugUserAgent = {
> +  DEBUG_UA: null,
> +  DOMAINS: ['youtube.com', 'www.youtube.com'],

We also want to whitelist 'youtube-nocookie.com', which some third-party sites use to embed YouTube videos. Bug 1119463 has more info.

@@ +7935,5 @@
> +  DEBUG_UA: null,
> +  DOMAINS: ['youtube.com', 'www.youtube.com'],
> +
> +  init: function() {
> +    UserAgentOverrides.addComplexOverride(this.onRequest.bind(this));

Does UserAgentOverrides handle both http and https URLs? YouTube uses both.

@@ +7942,5 @@
> +    this.DEBUG_UA = ua + " Build/" + gAppInfo.appBuildID;
> +  },
> +
> +  onRequest: function(channel, defaultUA) {
> +    if (this.DOMAINS.includes(channel.URI.host)) {

We only want to use to DEBUG_UA in the Nightly, Aurora, and Beta channels. In face, can we hide all this DebugUserAgent code from the Release channel behind an #ifdef?

Comment 10

[Ralph Giles (:rillian)]

(In reply to Chris Peterson [:cpeterson] from comment #9)
> Comment on attachment 8564379 [details] [diff] [review]
> Append the build id to the ua string for youtube
> 
> Review of attachment 8564379 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Since we are limiting the build id to YouTube sites and Firefox's
> pre-release channels, I'm not too concerned about browser fingerprinting. :)
> 
> ::: browser/base/content/browser.js
> @@ +7932,5 @@
> > +// Modify the user agent string for specific domains
> > +// to route debug information through their logging.
> > +var DebugUserAgent = {
> > +  DEBUG_UA: null,
> > +  DOMAINS: ['youtube.com', 'www.youtube.com'],
> 
> We also want to whitelist 'youtube-nocookie.com', which some third-party
> sites use to embed YouTube videos. Bug 1119463 has more info.

Added locally. Are the primary sites enough? Youtube uses a lot of subsidiary domains for page resources like images, trackers and so on.

> Does UserAgentOverrides handle both http and https URLs? YouTube uses both.

Yes, it overrides both protocols. I tested by visiting youtube and looking at the request headers in the network tab of the developer tools.

> We only want to use to DEBUG_UA in the Nightly, Aurora, and Beta channels.
> In face, can we hide all this DebugUserAgent code from the Release channel
> behind an #ifdef?

I couldn't figure out how to limit it to pre-release channels. There's an isReleaseBuild, but I assume that's 'true' on beta as well as release. It occurs to me now I could use the update channel either as js gAppInfo.defaultUpdateChannel or cpp MOZ_UPDATE_CHANNEL. Let me experiment.

Comment 11

[Ralph Giles (:rillian)]

Attachment: Append the build id to the ua string for youtube v2

Added youtube-nocookie, #ifdef protection for release channel.

Verified on nightly. Will try beta next.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Yeah, I can review this - but I want to make sure gavin is in the loop on this. We're cool with this, right gavin?

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8564411 [details] [diff] [review]
Append the build id to the ua string for youtube v2

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

::: browser/base/content/browser.js
@@ +922,5 @@
>      BrowserOnClick.init();
>      DevEdition.init();
>  
> +#if defined(MOZ_UPDATE_CHANNEL) && MOZ_UPDATE_CHANNEL != release
> +    UserAgentOverrides.init();

This also needs to be uninitted, it seems.

@@ +7946,5 @@
> +    'www.youtube-nocookie.com',
> +  ],
> +
> +  init: function() {
> +    UserAgentOverrides.addComplexOverride(this.onRequest.bind(this));

Because this is in browser.js, this is going to run once per window... I'm not entirely sure how UserAgentOverrides work, but somehow I think you probably want this to only run once. If that's the case, you want it in nsBrowserGlue.js instead.

Comment 14

[Justin Dolske [:Dolske]]

UA overrides were originally disabled in bug 896114 for performance reasons.

How long do we need to send this extra data to YouTube? Is this just for Beta-37, and in ~6 weeks it will be removed?

Comment 15

[Chris Peterson [:cpeterson]]

cpearce: will want need this YouTube UA override after we ship MSE? Is there an MSE #define we can use to #ifdef this override only on platforms with MSE?

It would also be convenient if the MSE #define was something that will be deleted after MSE is enabled on all platforms, so whoever is deleting that MSE #define remembers to delete this UA override. :)

Comment 16

[Ralph Giles (:rillian)]

(In reply to Chris Peterson [:cpeterson] from comment #15)
> Is there
> an MSE #define we can use to #ifdef this override only on platforms with MSE?

There's an about:config pref we could check.

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

Can't we just check to see if the MediaSource object is constructable, as its visibility is controlled in WebIDL to restrict it to platforms where MSE is available.

We can do that by seeing if (typeof MediaSource == "function"). The JS wizards here may know a better way to do that.

Comment 18

[Ralph Giles (:rillian)]

(window.MediaSource && typeof window.MediaSource == 'function') maybe?

Assuming window is available and we run before content has a chance to, that should work. And as long as we don't mind requiring a restart after flipping the pref.

Comment 19

[Ralph Giles (:rillian)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #13)
 
> > +#if defined(MOZ_UPDATE_CHANNEL) && MOZ_UPDATE_CHANNEL != release
> > +    UserAgentOverrides.init();
> 
> This also needs to be uninitted, it seems.

Can you offer some guidance on where? nsBrowserGlue has _onQuitApplicationGranted but I don't see an obvious shutdown method in browser.js. The fennec version leaves it active, I guess relying on gc?

> Because this is in browser.js, this is going to run once per window... I'm
> not entirely sure how UserAgentOverrides work, but somehow I think you
> probably want this to only run once. If that's the case, you want it in
> nsBrowserGlue.js instead.

It doesn't appear to work when called from nsBrowserGlue.js. Wes, any insight into this? Does it depend on machinery which isn't available outside a window?

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Ralph Giles (:rillian) from comment #18)
> And as long as we don't mind requiring a restart after flipping
> the pref.

Based on my testing, we require a restart when flipping the pref for WebIDL to notice anyway.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

(In reply to Ralph Giles (:rillian) from comment #19)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #13)
>  
> > > +#if defined(MOZ_UPDATE_CHANNEL) && MOZ_UPDATE_CHANNEL != release
> > > +    UserAgentOverrides.init();
> > 
> > This also needs to be uninitted, it seems.
> 
> Can you offer some guidance on where? nsBrowserGlue has
> _onQuitApplicationGranted but I don't see an obvious shutdown method in
> browser.js. The fennec version leaves it active, I guess relying on gc?

I can offer some help tomorrow, considering this seems to be kind of time-sensitive. I'm mconley, and operate on ET, if you want to ping me in IRC.

At the very least, I'll apply the patch and see what needs to be done to make it operate in nsBrowserGlue.js (or at the very least, find out why it can't work there).


(In reply to Justin Dolske [:Dolske] from comment #14)
> UA overrides were originally disabled in bug 896114 for performance reasons.
> 
> How long do we need to send this extra data to YouTube? Is this just for
> Beta-37, and in ~6 weeks it will be removed?

I am also very interested in the answers to these questions.

Clearing the needinfo from gavin's plate - if dolske is aware of this stuff and it passes his OK, I think that's good enough for me.

Comment 22

[Ralph Giles (:rillian)]

Attachment: Append the build id to the ua string for youtube v3

Ok, thanks for taking a look. I'm on Canada/Pacific if you need to find me on irc.

Here's a slightly updated patch which checks for MSE support, like :cpearce suggested. Still in browser.js.

Comment 23

[Ralph Giles (:rillian)]

Attachment: Append buildid in nsBrowserGlue.js v3

Port of the v3 patch to nsBrowserGlue.js. This version calls UserAgentOverrides.uninit(), but didn't work in my testing.

Comment 24

[Chris Peterson [:cpeterson]]

(In reply to Justin Dolske [:Dolske] from comment #14)
> How long do we need to send this extra data to YouTube? Is this just for
> Beta-37, and in ~6 weeks it will be removed?

We would like this extra build id until we ship MSE. YouTube will use it to track improvements, from beta to beta, of our MSE video metrics compared to Flash video. We can probably remove this UA override after we ship MSE on Windows (hopefully in 37).

Comment 25

[Wesley Johnston (:wesj)]

(In reply to Ralph Giles (:rillian) from comment #19)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #13)
>  
> > > +#if defined(MOZ_UPDATE_CHANNEL) && MOZ_UPDATE_CHANNEL != release
> > > +    UserAgentOverrides.init();
> > 
> > This also needs to be uninitted, it seems.
> 
> Can you offer some guidance on where? nsBrowserGlue has
> _onQuitApplicationGranted but I don't see an obvious shutdown method in
> browser.js. The fennec version leaves it active, I guess relying on gc?
> 
> > Because this is in browser.js, this is going to run once per window... I'm
> > not entirely sure how UserAgentOverrides work, but somehow I think you
> > probably want this to only run once. If that's the case, you want it in
> > nsBrowserGlue.js instead.
> 
> It doesn't appear to work when called from nsBrowserGlue.js. Wes, any
> insight into this? Does it depend on machinery which isn't available outside
> a window?

Ours doesn't really need to be uninited. It always runs, and because of how we shut down, its unlikely it would ever be called. We also only ever have one window. I don't see any reason you'd need a window though.

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8565212 [details] [diff] [review]
Append buildid in nsBrowserGlue.js v3

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

::: browser/components/nsBrowserGlue.js
@@ +119,5 @@
>                                    "resource://gre/modules/UpdateChannel.jsm");
>  #endif
>  
> +
> +#if defined(MOZ_UPDATE_CHANNEL) && MOZ_UPDATE_CHANNEL != release

We should file a follow-up bug to get this stuff removed when it's no longer necessary.

@@ +2869,5 @@
> +    let ua = Cc["@mozilla.org/network/protocol;1?name=http"]
> +               .getService(Ci.nsIHttpProtocolHandler).userAgent;
> +    this.DEBUG_UA = ua + " Build/" + gAppInfo.appBuildID;
> +    // Only run if the MediaSource Extension API is available.
> +    this.enabled = window.MediaSource && typeof window.MediaSource == 'function'

Ah, this bit isn't going to work in the scope of nsBrowserGlue.js, as there is no "window" in this context.

Is there some other way of detecting the MSE API? Perhaps an about:config pref?

Comment 27

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

Tracking all MSE P1 bugs for Firefox 37.

Comment 28

[Ralph Giles (:rillian)]

Attachment: Append buildid in nsBrowserGlue.js v4

Yes, we can check a pref instead. I've made that change, but I'm still not seeing this code work from nsBrowserGlue.js.

Filed bug 1133862 for the removal.

Comment 29

[Ralph Giles (:rillian)]

Attachment: Append the build id to the ua string for youtube v5

nsBrowserGlue.js doesn't have a gAppInfo and I missed the error message in the log noise. With a local copy it works on nightly. Tested against youtube with and without https, with and without media.mediasource.enabled true.

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8565550 [details] [diff] [review]
Append the build id to the ua string for youtube v5

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

Just a few nits and suggestions. Overall, I'm OK with this, assuming:

1) You've found a way of testing to ensure that this is not being included with release builds.
2) You get a green try push running at least mochitest-bc for Windows, OS X and Linux
3) You ensure that the user agent is indeed getting modified for those domains, and only those domains.

::: browser/components/nsBrowserGlue.js
@@ +2870,5 @@
> +                  .getService(Ci.nsIXULAppInfo)
> +                  .QueryInterface(Ci.nsIXULRuntime);
> +    let ua = Cc["@mozilla.org/network/protocol;1?name=http"]
> +                .getService(Ci.nsIHttpProtocolHandler).userAgent;
> +    this.DEBUG_UA = ua + " Build/" + info.appBuildID;

You can just use Services.appinfo.appBuildID;

@@ +2872,5 @@
> +    let ua = Cc["@mozilla.org/network/protocol;1?name=http"]
> +                .getService(Ci.nsIHttpProtocolHandler).userAgent;
> +    this.DEBUG_UA = ua + " Build/" + info.appBuildID;
> +    // Only run if the MediaSource Extension API is available.
> +    this.enabled = Services.prefs.getBoolPref("media.mediasource.enabled");

Might as well just check that media.mediasource.enabled is true as the first thing that init does, and bail out early if it's false.

That way, we avoid doing any work unless media.mediasource.enabled is true.

It'll also mean we won't need this.enabled, since the override won't be registered unless media.mediasource.enabled is true.

Comment 31

[Ralph Giles (:rillian)]

Attachment: Append the build id to the ua string for youtube v6

Great suggestions, thanks. Applied. Carrying forward r=mconley.

Comment 32

[Ralph Giles (:rillian)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #30)

> 1) You've found a way of testing to ensure that this is not being included
> with release builds.

Working on this. Since this is time-sensitive, would you be ok with landing on m-c and requesting 37 uplift in parallel with this testing? I take the extra fingerprinting pretty seriously, and we'll need to verify this a couple of times as the code rides to release. In the meantime we can see if this is working for youtube.

> 2) You get a green try push running at least mochitest-bc for Windows, OS X
> and Linux

Pushed to try on top of aurora in https://treeherder.mozilla.org/#/jobs?repo=try&revision=d65c01a7463c

> 3) You ensure that the user agent is indeed getting modified for those
> domains, and only those domains.

Yes. Verified manually.

Comment 33

[Mike Conley (:mconley) (:⚙️)]

(In reply to Ralph Giles (:rillian) from comment #32)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #30)
> 
> > 1) You've found a way of testing to ensure that this is not being included
> > with release builds.
> 
> Working on this. Since this is time-sensitive, would you be ok with landing
> on m-c and requesting 37 uplift in parallel with this testing? I take the
> extra fingerprinting pretty seriously, and we'll need to verify this a
> couple of times as the code rides to release. In the meantime we can see if
> this is working for youtube.
> 

I'm personally fine with that, yes.

Comment 34

[Ralph Giles (:rillian)]

Thanks!

Comment 35

[Ralph Giles (:rillian)]

Comment on attachment 8565604 [details] [diff] [review]
Append the build id to the ua string for youtube v6

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: We want this to coordinate issues with youtube as we fix remaining issues on this feature, especially with our beta population.
[Describe test coverage new/current, TreeHerder]: Local testing. Presuming green on try.
[Risks and why]: Risk should be minimal. This adds the build id to the user agent string when visiting youtube with MSE enabled. Unlikely failure modes would be leaking the build id to other domains and performance issues with the override.
[String/UUID change made/needed]: None.

Comment 36

[Justin Dolske [:Dolske]]

(In reply to Chris Peterson [:cpeterson] from comment #24)
> (In reply to Justin Dolske [:Dolske] from comment #14)
> > How long do we need to send this extra data to YouTube? Is this just for
> > Beta-37, and in ~6 weeks it will be removed?
> 
> We would like this extra build id until we ship MSE. YouTube will use it to
> track improvements, from beta to beta, of our MSE video metrics compared to
> Flash video. We can probably remove this UA override after we ship MSE on
> Windows (hopefully in 37).

Can we be a little firmer in that plan? That we _will_ remove it when MSE ships, which currently planned for 37 but that it might(?) slip to 38? Would also be good to make sure YouTube knows this is a very temporary thing.

I think this would also merit a dev.platform post, since this is temporarily reintroducing (1) a known performance regression and (2) a feature that we've previously WONTFIX'd for desktop. Seems fair given the schedule pressure, but folks should know what's happening and changes involving the UA have tended to be a pit of despair.

Comment 37

[Ralph Giles (:rillian)]

(In reply to Justin Dolske [:Dolske] from comment #36)

> Can we be a little firmer in that plan? That we _will_ remove it when MSE
> ships, which currently planned for 37 but that it might(?) slip to 38? Would
> also be good to make sure YouTube knows this is a very temporary thing.

I don't think this is up to me, but I don't see why we'd want to keep it after we've stopped with the beta feature landings.

> I think this would also merit a dev.platform post, since this is temporarily
> reintroducing (1) a known performance regression and (2) a feature that
> we've previously WONTFIX'd for desktop. Seems fair given the schedule
> pressure, but folks should know what's happening and changes involving the
> UA have tended to be a pit of despair.

https://groups.google.com/forum/#!topic/mozilla.dev.platform/AHCvcOV9KKw

Comment 38

[Ralph Giles (:rillian)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/982b7c688f3f

Comment 39

[Chris Peterson [:cpeterson]]

(In reply to Justin Dolske [:Dolske] from comment #36)
> (In reply to Chris Peterson [:cpeterson] from comment #24)
> > We would like this extra build id until we ship MSE. YouTube will use it to
> > track improvements, from beta to beta, of our MSE video metrics compared to
> > Flash video. We can probably remove this UA override after we ship MSE on
> > Windows (hopefully in 37).
> 
> Can we be a little firmer in that plan? That we _will_ remove it when MSE
> ships, which currently planned for 37 but that it might(?) slip to 38? Would
> also be good to make sure YouTube knows this is a very temporary thing.

Fair enough. We will remove it when MSE ships (on Windows Vista+) in 37 (or possibly 38). MSE for Linux, OS X, and Windows XP are scheduled for a later release, but the performance measurements for Windows Vista+ should be useful for all platforms.

YouTube knows this is a temporary thing. This is a test feature we suggested.

Comment 40

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8565604 [details] [diff] [review]
Append the build id to the ua string for youtube v6

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js

>+var DebugUserAgent = {

>+  onRequest: function(channel, defaultUA) {
>+    if (this.DOMAINS.includes(channel.URI.host)) {

Array.prototype.includes is Nightly-only (bug 1070767), so this won't work on the other branches.

Comment 41

[Ralph Giles (:rillian)]

Attachment: Use portable indexOf instead of Array.includes.

Aha. Thank you.

Follow-up patch to use Array.indexOf instead of includes. I couldn't get 'key in array' to work, which is why I used .includes() in the first place.

Comment 42

[Ralph Giles (:rillian)]

Comment on attachment 8565604 [details] [diff] [review]
Append the build id to the ua string for youtube v6

Whichever of you gets to it first.

Comment 43

[Ralph Giles (:rillian)]

Comment on attachment 8565741 [details] [diff] [review]
Use portable indexOf instead of Array.includes.

Thanks for the quick review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/84fff2da5d7e

Comment 44

[Robert Kaiser]

Doesn't it need Gerv, i.e. the user agent module owner, to actually sign off on UA changes?

Comment 45

[Gervase Markham [:gerv]]

While I'd prefer us to use a different header for cleanliness, I can imagine that perhaps their server-side processing is based around UA. So if it's a short-term thing for two specific sites on non-release channels, I have no problem with it. If any of those conditions were changed, I might.

Gerv

Comment 46

[Ralph Giles (:rillian)]

Thanks Gerv. Things are as you imagine. As I understand it, their filtering for both page content and logging is based on the UA, which is how we arrived at this bug.

Comment 47

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

Comment on attachment 8565604 [details] [diff] [review]
Append the build id to the ua string for youtube v6

I've made a note to review whether we can remove this override before 37 ships. In this case we will likely remove this override in Beta 7. Aurora+

Comment 48

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4fd8b4229765
https://hg.mozilla.org/releases/mozilla-aurora/rev/7dcad4766947

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/982b7c688f3f
https://hg.mozilla.org/mozilla-central/rev/84fff2da5d7e