897221 - dynamically updatable UA override mechanism

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Browser, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

To help with site evangelism we want to be able to update the UA overrides without pushing a gaia update.

Comment 1

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

From a user experience perspective, a dynamic UA override list is highly preferable as this will allow us to respond to continually changing Web content.

As some partners have specific sites that they would like to include on the whitelist but that may not be required by all partners, we should discuss whether we want a single list or a mechanism that supports both Mozilla and partner lists.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Notes: https://etherpad.mozilla.org/dynamic-ua-overrides

I have some WIP patches that I will put up soon.

Comment 3

[Dão Gottwald [:dao]]

(In reply to Jim Chen [:jchen :nchen] from comment #2)
> Notes: https://etherpad.mozilla.org/dynamic-ua-overrides

It seems like that proposal could make it easier for partners to add device-specific cruft the UA string for an unbound amount of sites. Why are we allowing them to change the default device UA in the first place while keeping the Firefox OS brand, thus requiring this "carrot to keep from having to change the default device UA"?

Comment 4

[Gervase Markham [:gerv]]

I'm pretty convinced that all UA overrides should come through us. We should not let partners mess with this; it's a really complex area, and the result would be a compatibility nightmare - and lots of uncertain web developers who curse Firefox OS.

Gerv

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Yes, I think we've concluded that Mozilla will maintain the only UA overrides list, and partners have to come to us with their requirements. Essentially this feature keeps the status quo on B2G; it just making it easier for us to add to the list and also remove items from the list once evangelism succeeds.

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add UserAgentUpdates.jsm (v1)

WIP

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add support for preset overrides in UserAgentOverrides.jsm (v1)

WIP. This is the mechanism to load updates into the override module.

Comment 8

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

(In reply to Dão Gottwald [:dao] from comment #3)
> It seems like that proposal could make it easier for partners to add
> device-specific cruft the UA string for an unbound amount of sites. Why are
> we allowing them to change the default device UA in the first place while
> keeping the Firefox OS brand, thus requiring this "carrot to keep from
> having to change the default device UA"?

The Firefox OS UA has been held as a terminal acceptance (TA) certification blocker by some of our partners. Translation: the inclusion of a device/model id in the UA is a stop ship issue. There are real use cases that still require device/model id in the UA today. (See CPU/Memory Determination on https://wiki.mozilla.org/UA/UseCases.) We need a solution in place that allows us to move past TA without modifying the default device UA. An UA override mechanism that provides modifiable, partner specific overrides is a short term stop gap while we figure out a long term approach for this issue.

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add support for preset overrides in UserAgentOverrides.jsm (v2)

This patch adds a 'setPresetOverrides' method to UserAgentOverrides. When looking for UA overrides, the preset overrides are searched, after searching the regular pref overrides.

I also adjusted the algorithm for matching overrides. Instead of iterating through the overrides and checking each one against the host string, this algorithm iterates through possible host strings and check each one against the overrides. I think this way is more efficient because we only use map lookups which are very fast.

Another benefit of this approach is that subdomain overrides always have priority. For example, if we are looking up 'foo.bar.com' with two overrides, 'foo.bar.com' and 'bar.com', the old algorithm can choose either one based on their order in the Map. However, the new algorithm will always choose 'foo.bar.com'. This patch adds a test for this behavior to the test in bug 901085.

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Support JS objects and avoid negative delay in nsUpdateTimerManager (v1)

When we have a callback in nsUpdateTimerManager, I think it makes sense to check for callback.notify instead of (callback instanceof nsITimerCallback). This way a regular JS object can be used as callback.

This patch also fixes a bug where _timer.delay can be set to a negative value. For example, this can happen when setting a new interval of 1 minute, but we're already more than 1 minute into the current, longer interval.

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add UserAgentUpdates.jsm (v2)

Do you think you can review this, Dão?

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add test for UserAgentUpdates.jsm (v1)

Test for the UserAgentUpdates module

Comment 13

[Andreas Gal :gal]

What happens if this list keeps growing and becomes very large?

Comment 14

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Andreas Gal :gal from comment #13)
> What happens if this list keeps growing and becomes very large?

Hopefully it won't come to that :)  We update the whole list at a time, so we can take out entries depending site evangelism. And if you're thinking about performance, this should handle hundreds of entries fine (the current b2g list has ~180 entries).

Comment 15

[[:fabrice] Fabrice Desré]

Comment on attachment 788336 [details] [diff] [review]
Add support for preset overrides in UserAgentOverrides.jsm (v2)

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

r=me with nits addressed.

::: netwerk/protocol/http/UserAgentOverrides.jsm
@@ +51,5 @@
>    },
>  
> +  setPresetOverrides: function uao_setPresetOverrides(overrides) {
> +    gOverrideForHostCache.clear();
> +    overrides && (overrides.get = function (key) {

nit: function(key)

@@ +84,3 @@
>        }
> +      return userAgent;
> +    };

nit: add blank line

Comment 16

[[:fabrice] Fabrice Desré]

Comment on attachment 788348 [details] [diff] [review]
Add UserAgentUpdates.jsm (v2)

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

r- because you need at least to fix the file loading/writing.

I guess we also need a followup to load this jsm and call its init() function.

::: netwerk/protocol/http/UserAgentUpdates.jsm
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = [ "UserAgentUpdates" ];

nit: we usually don't add spaces around "Symbol"

@@ +15,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(
> +    this, "UpdateChannel", "resource://gre/modules/UpdateChannel.jsm");

nit: add a blank line

@@ +20,5 @@
> +XPCOMUtils.defineLazyModuleGetter(
> +    this, "UserAgentOverrides", "resource://gre/modules/UserAgentOverrides.jsm");
> +
> +XPCOMUtils.defineLazyServiceGetter(
> +    this, "gUpdateTimer", "@mozilla.org/updates/timer-manager;1", Ci.nsIUpdateTimerManager);

Nit: add a blank line

@@ +45,5 @@
> +
> +var gInitialized = false;
> +
> +this.UserAgentUpdates = {
> +    init: function uau_init() {

Here and everywhere else: there is no need anymore to name functions. The js engine is smart enough now to print messages with enough context.

@@ +46,5 @@
> +var gInitialized = false;
> +
> +this.UserAgentUpdates = {
> +    init: function uau_init() {
> +        if (gInitialized)

here and in other places too: use {} even for single line ifs

@@ +83,5 @@
> +            this._loadFromFile(FileUtils.getFile(
> +                    dirs.shift(), [FILE_UPDATES], true /* followLink */),
> +                /* success */ this._applyUpdate.bind(this),
> +                /* error */ loadUpdate.bind(this));
> +        }).call(this);

you could also do dirs.forEach(..., this);

@@ +108,5 @@
> +                // An error reading the file, or an error parsing the contents.
> +                stream.close(); // close() is idempotent
> +                error();
> +            }
> +        }).bind(this));

Please use OS.File instead of NetUtil.

@@ +118,5 @@
> +            .createInstance(Ci.nsIScriptableUnicodeConverter);
> +        converter.charset = 'UTF-8';
> +        let istream = converter.convertToInputStream(JSON.stringify(update));
> +        NetUtil.asyncCopy(istream, ostream,
> +            function (status) FileUtils.closeSafeFileOutputStream(ostream));

Here also.

@@ +125,5 @@
> +    _getPref: function uau_getPref(name, def) {
> +        try {
> +            switch (typeof def) {
> +            case 'number': return Services.prefs.getIntPref(name);
> +            case 'boolean': return Services.prefs.getBoolPref(name);

nit: the indentation is off.

@@ +159,5 @@
> +    },
> +
> +    _fetchUpdate: function uau_fetchUpdate(url, success, error) {
> +        let request = Cc['@mozilla.org/xmlextras/xmlhttprequest;1']
> +            .createInstance(Ci.nsIXMLHttpRequest);

nit: align .createInstance with ["@mozilla..."] (and use double quotes everywhere, there is a mix of single and double in this file)

@@ +190,5 @@
> +    _scheduleUpdate: function uau_scheduleUpdate(retry) {
> +        let interval = this._getPref(PREF_UPDATES_INTERVAL, 604800 /* 1 week */);
> +        retry && (interval = this._getPref(PREF_UPDATES_RETRY, interval));
> +
> +        gUpdateTimer.registerTimer(TIMER_ID, this, Math.max(1, interval));

1 seems way too low has a min interval... I would set it to one day.

Comment 17

[[:fabrice] Fabrice Desré]

Comment on attachment 788350 [details] [diff] [review]
Add test for UserAgentUpdates.jsm (v1)

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

r=me (and now I see why you set a 1s min interval in the other patch...) Maybe we need a "testing mode" pref for that.

Comment 18

[Dão Gottwald [:dao]]

(In reply to Fabrice Desré [:fabrice] from comment #16)
> Comment on attachment 788348 [details] [diff] [review]
> Add UserAgentUpdates.jsm (v2)
> 
> Review of attachment 788348 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- because you need at least to fix the file loading/writing.
> 
> I guess we also need a followup to load this jsm and call its init()
> function.

I think it might make sense to reverse the dependency and let UserAgentOverrides init and call UserAgentUpdates, and thereby avoid exposing the setPresetOverrides method that I find somewhat weird (partly because the name is vague).

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 788348 [details] [diff] [review]
Add UserAgentUpdates.jsm (v2)

another nit: indentation should be two spaces

Comment 20

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Initialize UserAgentUpdates.jsm inside UserAgentOverrides (v1)

(In reply to Fabrice Desré [:fabrice] from comment #15)
> Comment on attachment 788336 [details] [diff] [review]
> Add support for preset overrides in UserAgentOverrides.jsm (v2)
> 
> > +    overrides && (overrides.get = function (key) {
> 
> nit: function(key)

Done

> > +      return userAgent;
> > +    };
> 
> nit: add blank line

Done

(In reply to Dão Gottwald [:dao] from comment #18)
> (In reply to Fabrice Desré [:fabrice] from comment #16)
> > Comment on attachment 788348 [details] [diff] [review]
> > Add UserAgentUpdates.jsm (v2)
> > 
> 
> I think it might make sense to reverse the dependency and let
> UserAgentOverrides init and call UserAgentUpdates, and thereby avoid
> exposing the setPresetOverrides method that I find somewhat weird (partly
> because the name is vague).

That sounds like a good idea. I changed UserAgentOverrides to call UserAgentUpdates.init() and pass it a callback. I renamed the variable in UserAgentOverrides to 'gUpdatedOverrides'.

Re-asking for review because of this change.

Comment 21

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add UserAgentUpdates.jsm (v3)

(In reply to Fabrice Desré [:fabrice] from comment #16)
> Comment on attachment 788348 [details] [diff] [review]
> Add UserAgentUpdates.jsm (v2)
> 
> I guess we also need a followup to load this jsm and call its init()
> function.

Done in the other new patch.

> > +this.EXPORTED_SYMBOLS = [ "UserAgentUpdates" ];
> 
> nit: we usually don't add spaces around "Symbol"

Fixed

> > +XPCOMUtils.defineLazyModuleGetter(
> > +    this, "UpdateChannel", "resource://gre/modules/UpdateChannel.jsm");
> 
> nit: add a blank line

Done

> > +this.UserAgentUpdates = {
> > +    init: function uau_init() {
> 
> Here and everywhere else: there is no need anymore to name functions. The js
> engine is smart enough now to print messages with enough context.

Removed unnecessary function names.

> > +    init: function uau_init() {
> > +        if (gInitialized)
> 
> here and in other places too: use {} even for single line ifs

Fixed

> > +            }
> > +        }).bind(this));
> 
> Please use OS.File instead of NetUtil.

Changed to OS.File. I'm not very familiar with OS.File and Promise so please check my code.

> > +        NetUtil.asyncCopy(istream, ostream,
> > +            function (status) FileUtils.closeSafeFileOutputStream(ostream));
> 
> Here also.

Changed to OS.File.

> > +            case 'number': return Services.prefs.getIntPref(name);
> > +            case 'boolean': return Services.prefs.getBoolPref(name);
> 
> nit: the indentation is off.

Fixed

> > +        let request = Cc['@mozilla.org/xmlextras/xmlhttprequest;1']
> > +            .createInstance(Ci.nsIXMLHttpRequest);
> 
> nit: align .createInstance with ["@mozilla..."] (and use double quotes
> everywhere, there is a mix of single and double in this file)

Done

> > +        gUpdateTimer.registerTimer(TIMER_ID, this, Math.max(1, interval));
> 
> 1 seems way too low has a min interval... I would set it to one day.

A separate "testing mode" seems like too much trouble for this one change. The pref should never be set to anything close to 1 outside of the test. It'd be a bug if it is.

Comment 22

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add test for UserAgentUpdates.jsm (v1.1)

Fixed the test to use OS.File also. Carried over r+.

Comment 23

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Schedule in main process and notify content processes of updates for B2G (v1)

This patch is for B2G support. It restricts scheduling for updates to the main process. It also notifies content processes when an update occurs (this is necessary for overriding navigator.userAgent). The patch notifies content processes by changing a pref in the main process and observing the pref change in the content processes. However, I'm not sure how content process notification can be tested in our Mochitest :(

Comment 24

[[:fabrice] Fabrice Desré]

(In reply to Jim Chen [:jchen :nchen] from comment #23)
> Created attachment 797475 [details] [diff] [review]
> Schedule in main process and notify content processes of updates for B2G (v1)
> 
> This patch is for B2G support. It restricts scheduling for updates to the
> main process. It also notifies content processes when an update occurs (this
> is necessary for overriding navigator.userAgent). The patch notifies content
> processes by changing a pref in the main process and observing the pref
> change in the content processes. However, I'm not sure how content process
> notification can be tested in our Mochitest :(

Are you sure that the content process notification works at all? I would be (pleasantly) surprised if we carried pref changes over IPC.

Comment 25

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Fabrice Desré [:fabrice] from comment #24)
> (In reply to Jim Chen [:jchen :nchen] from comment #23)
> > Created attachment 797475 [details] [diff] [review]
> > Schedule in main process and notify content processes of updates for B2G (v1)
> > 
> > This patch is for B2G support. It restricts scheduling for updates to the
> > main process. It also notifies content processes when an update occurs (this
> > is necessary for overriding navigator.userAgent). The patch notifies content
> > processes by changing a pref in the main process and observing the pref
> > change in the content processes. However, I'm not sure how content process
> > notification can be tested in our Mochitest :(
> 
> Are you sure that the content process notification works at all? I would be
> (pleasantly) surprised if we carried pref changes over IPC.

I have not tested it (I'm going to try it now). I saw some IPC code [1] that forwarded pref changes, but I could be reading the code wrong.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#1659

Comment 26

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Schedule in main process and notify content processes of updates for B2G (v2)

(In reply to Jim Chen [:jchen :nchen] from comment #25)
> (In reply to Fabrice Desré [:fabrice] from comment #24)
> > 
> > Are you sure that the content process notification works at all? I would be
> > (pleasantly) surprised if we carried pref changes over IPC.
> 
> I have not tested it (I'm going to try it now). I saw some IPC code [1] that
> forwarded pref changes, but I could be reading the code wrong.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#1659

I tested on B2G, and content processes do receive pref changes. However, I found that the profile directory on B2G does not have the right permissions for content processes to read, so I switched to the "PrefD" directory, which is "/data/local" on B2G and the same as "ProfD" on other platforms. The saved file is set to be world-readable. I think this is acceptable security-wise because the file only contains data from a public server.

Comment 27

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Include nsIUpdateTimerManager in Fennec and B2G (v1)

In Fennec and B2G, we are not including update.xpt, which has the definition for nsIUpdateTimerManager. The interface is not actually a part of the updater, so it shouldn't be ifdef'ed to MOZ_UPDATER.

Comment 28

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59214bd5b894
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0938d3591a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8711da4abee
https://hg.mozilla.org/integration/mozilla-inbound/rev/88fd66b4944c
https://hg.mozilla.org/integration/mozilla-inbound/rev/a085fcd7822c
https://hg.mozilla.org/integration/mozilla-inbound/rev/44c54f58ce7d

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out 88fd66b4944 for frequent armv6 timeouts in:

http://hg.mozilla.org/integration/mozilla-inbound/rev/9e90a0bf43fc

Comment 30

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/59214bd5b894
https://hg.mozilla.org/mozilla-central/rev/d0938d3591a9
https://hg.mozilla.org/mozilla-central/rev/a8711da4abee
https://hg.mozilla.org/mozilla-central/rev/a085fcd7822c
https://hg.mozilla.org/mozilla-central/rev/44c54f58ce7d

Comment 31

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/88fd66b4944c

Comment 32

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add test for UserAgentUpdates.jsm (v2)

Turns out the UserAgentUpdates object needed a QueryInterface method. Still not sure why the problem only showed up on ARMv6...

Comment 33

[[:fabrice] Fabrice Desré]

Comment on attachment 811307 [details] [diff] [review]
Add test for UserAgentUpdates.jsm (v2)

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

::: netwerk/protocol/http/UserAgentUpdates.jsm
@@ +234,5 @@
> +        aIID.equals(Ci.nsISupports)) {
> +      return this;
> +    }
> +    throw Components.results.NS_NOINTERFACE;
> +  },

Any reason for not using XPCOMUtils.generateQI([...]) ?

Comment 34

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add test for UserAgentUpdates.jsm (v3)

(In reply to Fabrice Desré [:fabrice] from comment #33)
> Comment on attachment 811307 [details] [diff] [review]
> Add test for UserAgentUpdates.jsm (v2)
> 
> Review of attachment 811307 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/UserAgentUpdates.jsm
> @@ +234,5 @@
> > +        aIID.equals(Ci.nsISupports)) {
> > +      return this;
> > +    }
> > +    throw Components.results.NS_NOINTERFACE;
> > +  },
> 
> Any reason for not using XPCOMUtils.generateQI([...]) ?

Ah I wasn't aware of it. This should be better. Thanks!

Comment 35

[Andreas Gal :gal]

I would like to see a follow-up bug with a better scaling approach. UA issues will be with us for a while. Even if we focus on the top N sites in top K locales, we can easily grow a 50k list just to address the most urgent UA issues.

Scaling issues:
- we don't want to keep this in memory all the time (database in indexeddb or even better a mmapped sorted list?)
- we don't want to re-download the whole list all the time (we should factor out the download code into a generic incremental update-capable download path for lists like this and others)
- we don't want to slow down network requests with the lookup (maybe add a bloom filter that helps us preselect where to check for overrides at all)
- some devices don't care about UAs for some locales, so how about a per-TLD list that is downloaded the first time you access that TLD?

We don't need to address all of this at once, but lets have a clear plan.

Comment 36

[Andreas Gal :gal]

Last bit I forgot: bonus points if we can come up with a way to detect the top N sites. This will need some solid privacy reviewing etc but it would definitely help maintaining the UA override list. If we know the top N sites people go to in a locale, we can do some analysis and see if we get the right mobile content with the FFOS vs WebKit UA and then focus evangelism and overrides as needed.

Comment 37

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

(In reply to Andreas Gal :gal from comment #36)
> Last bit I forgot: bonus points if we can come up with a way to detect the
> top N sites. This will need some solid privacy reviewing etc but it would
> definitely help maintaining the UA override list. If we know the top N sites
> people go to in a locale, we can do some analysis and see if we get the
> right mobile content with the FFOS vs WebKit UA and then focus evangelism
> and overrides as needed.

We can review your suggestion with privacy but based on my work on Telemetry I think sending specific URLs/Domains is a non starter. (Still, worth having a discussion.)

Comment 38

[(inactive) Jim Chen [:jchen] [:darchons]]

Re-landed test at
https://hg.mozilla.org/integration/mozilla-inbound/rev/922eb17b8a44

Going to file follow-up bugs soon.

Comment 39

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/922eb17b8a44

Comment 40

[Karl Dubost💡 :karlcow]

Attachment: dynamic-ua-overrides-latest.txt

Adding the etherpad text export of Goals and constraints that it doesn't get lost in the future.

Comment 41

[Karl Dubost💡 :karlcow]

We have a pretty bad issue in Bug 1132734. 
The updates on the UA override list are not propagated to the UA override server.