885641 - Force an update check if the build is older than some reasonable value

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Doug Turner (:dougt)]

Firefox has a problem with adoption of new versions. [1]  There are probably a series of issues.  One simple thing we can do to resolve many of these things is to force a version check if the installation time is older than some period of time (say 6 weeks).

So, what I propose is that during delayedStartup we check the installation time via Services.appinfo.appBuildID (which we already to in other places), and compare it with 'todays' date.  If the delta is more than some value, we call updater.backgroundChecker.checkForUpdates().

[1] http://arstechnica.com/information-technology/2013/06/internet-explorer-10-takes-chunks-out-of-ie9-windows-8-closes-on-vista/

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

hey rob, please take a look and let me know what you think of the direction.

Comment 2

[Robert Strong (they/them - no direct email)]

Comment on attachment 765783 [details] [diff] [review]
patch v.1

I think you can get away with just calling notify and just let app update do a background check.

const Ci = Components.interfaces; const Cc = Components.classes; 
Cc["@mozilla.org/updates/timer-manager;1"].getService(Ci.nsIUpdateTimerManager).QueryInterface(Ci.nsITimerCallback).notify(null);

Should simplify things quite a bit

Comment 3

[Robert Strong (they/them - no direct email)]

You might want to run this by Asa as well... he might have some ideas like doing this even when updates are disabled.

Comment 4

[Robert Strong (they/them - no direct email)]

It will be interesting to see if this changes things. :)

Comment 5

[Robert Strong (they/them - no direct email)]

(In reply to Robert Strong [:rstrong] (do not email) from comment #2)
> Comment on attachment 765783 [details] [diff] [review]
> patch v.1
> 
> I think you can get away with just calling notify and just let app update do
> a background check.
> 
> const Ci = Components.interfaces; const Cc = Components.classes; 
> Cc["@mozilla.org/updates/timer-manager;1"].getService(Ci.
> nsIUpdateTimerManager).QueryInterface(Ci.nsITimerCallback).notify(null);
> 
> Should simplify things quite a bit
Ignore that pseudo code and use this instead

const Ci = Components.interfaces; const Cc = Components.classes; Cc["@mozilla.org/updates/update-service;1"].getService(Ci.nsIApplicationUpdateService).QueryInterface(Ci.nsITimerCallback).notify(null);

Comment 6

[Doug Turner (:dougt)]

it 'works'... :)

how do I test this out for realz?

Comment 7

[Doug Turner (:dougt)]

Attachment: patch v.2

we probably should move the "app.update.checkInstallTime.days" pref from where it is, into the branding prefs.  And have nightly/aurora check once a day, and the others like every 1-2 weeks?

Comment 8

[Robert Strong (they/them - no direct email)]

There are tons of tests for the notify method already so you could focus on the browser specific portion though exactly how I'm not sure. If you want end to end testing you will need to jump through a bunch of hoops to do so similar to what I had to do for app update here
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/test/chrome/

and here
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/test/unit/

there is no simple way but you might be able to fake the app update parts if you can get that code to run in an xpcshell test and create a mock nsIApplicationUpdateService.

Comment 9

[Robert Strong (they/them - no direct email)]

Comment on attachment 765790 [details]
patch v.2

Overall this looks fine.

Using the buildID could be a minefield since it has changed in the past. Please run that part by someone on the build side or possibly bsmedberg.

The system time can change and might lead to some weird results. Not sure of a better way to deal with that. :(

I'm a tad buried and with the app update part being minimal please ask Gavin if he or someone else on his team to review the remainder. I'll try to help with the tests when you get something together.

Comment 10

[Doug Turner (:dougt)]

> Using the buildID could be a minefield since it has changed in the past. Please run that part by someone on the build side or possibly bsmedberg.

Asking releng

> The system time can change and might lead to some weird results. Not sure of a better way to deal with that. :(

Yeah, two cases -- either we test more (who cares), or we are in the same state as today (crappy, but who cares).

Since we're talking about days/weeks, i don't think the accuracy matters that much.

> I'll try to help with the tests when you get something together.

Instead of tests, can we just collect telemetry to prove that things are working?  I am not sure the test value here (if we aren't testing production, it kind of doesn't matter really).

Comment 11

[(dormant account)]

As far as telemetry goes ideally* we'd report a boolean histogram.

something like 
ANCIENT_BUILD_TRIED_TO_UPDATE...record 1 if update was successful, 0 if no update was found.

This gives you 3 pieces of info: boolean flags + whether this check triggered. 

* this might be tricky

Comment 12

[Robert Strong (they/them - no direct email)]

(In reply to Doug Turner (:dougt) from comment #10)
> > Using the buildID could be a minefield since it has changed in the past. Please run that part by someone on the build side or possibly bsmedberg.
> 
> Asking releng
> 
> > The system time can change and might lead to some weird results. Not sure of a better way to deal with that. :(
> 
> Yeah, two cases -- either we test more (who cares), or we are in the same
> state as today (crappy, but who cares).
> 
> Since we're talking about days/weeks, i don't think the accuracy matters
> that much.
You could update the timestamp pref when it is greater or lesser than the interval to mitigate this if it occurs.

> > I'll try to help with the tests when you get something together.
> 
> Instead of tests, can we just collect telemetry to prove that things are
> working?  I am not sure the test value here (if we aren't testing
> production, it kind of doesn't matter really).
I don't know and I haven't used telemetry in this manner for tests or Firefox tests as in this case.

The value would be testing the untested parts of this new code which would be a good thing since a build ID change could muck things up. The app update call is already extremely well tested.

Comment 13

[Justin Wood (:Callek)]

(In reply to Doug Turner (:dougt) from comment #10)
> > Using the buildID could be a minefield since it has changed in the past. Please run that part by someone on the build side or possibly bsmedberg.
> 
> Asking releng

Currently the buildid is ever increasing and based on the date. While it has changed in the past there are still lots of things that rely on it, so adding an extra thing is (imo) not a bad idea, since we'll have to be careful if we ever do change it anyway.

We have no plans to change its format/generation at this time.

> > The system time can change and might lead to some weird results. Not sure of a better way to deal with that. :(
> 
> Yeah, two cases -- either we test more (who cares), or we are in the same
> state as today (crappy, but who cares).
> 
> Since we're talking about days/weeks, i don't think the accuracy matters
> that much.

System time can of course be wildly out of date, but I agree that extra checks make sense. --- we could even go one further and do the check automatically if the time is *less* than our parsed buildid by a day or more.

---

As discussed on IRC a good threshold is probably 6-7 weeks for our release audience, since the actual buildid can be a week before our release date, and we generally start throttling back updates shortly after release while we ensure stability.

I also suggested that we do different thresholds for each train, with my specific suggestion being:

nightly 48-72 hours, aurora 1 week, beta 2 weeks, release 6-7 weeks.

Comment 14

[Chris AtLee [:catlee]]

The buildid has always been time based, and I don't see that changing any time soon. We have added more precision to it at various points though.

Can we add a check that the buildid passed into the build system matches the format expected by this code? Then at least we catch the problem early if we end up changing the buildid later.

Comment 15

[Doug Turner (:dougt)]

> Can we add a check that the buildid passed into the build system matches the format expected by this code?

I think we just need to define that buildid is always exactly this date string.  I am not sure where'd we test this.  I suppose we can add some sanity tests.  If we screw this date up, things like the about: dialog will be busted (which is tested via litmus).  The about: dialog also uses this identifier to display the date of the build.


I'll move the prefs to the right branding location as described in comment #13.

I'll also see if we can get some moar telemetry so that we aren't stumbling in the dark.

Comment 16

[Doug Turner (:dougt)]

Attachment: patch v.3

Comment 17

[Doug Turner (:dougt)]

Comment on attachment 766240 [details] [diff] [review]
patch v.3

taras, can you double check the telemetry here?  nothing fancy, basically want to know:  when we get a update due to an old buildid, does it pass or fail.

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 766240 [details] [diff] [review]
patch v.3

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -1272,16 +1272,62 @@ var gBrowserInit = {
>       gMetroPrefs.pushDesktopControlledPrefToMetro(prefName);
>       Services.prefs.addObserver(prefName, gMetroPrefs, false);
>     }, this);
> #endif
> #endif
> 
>     Services.obs.notifyObservers(window, "browser-delayed-startup-finished", "");
>     setTimeout(function () { BrowserChromeTest.markAsReady(); }, 0);
>+
>+    // check for update if our build is old
>+    if (gPrefService.getBoolPref("app.update.enabled") &&
>+        gPrefService.getBoolPref("app.update.checkInstallTime")) {
>+
>+        TelemetryTimestamps.add("checkingForUpdateBasedOnInstallTime");
>+

This code runs in every window you open, but you only want it after startup. This means you should put it somewhere in nsBrowserGlue.js, maybe _onFirstWindowLoaded.

Also, indent with two spaces.

Comment 19

[Doug Turner (:dougt)]

dao, thanks...

rstrong/taras, continue reviewing.  I'll move and incorporate your changes.

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Justin Wood (:Callek) from comment #13)
> As discussed on IRC a good threshold is probably 6-7 weeks for our release
> audience, since the actual buildid can be a week before our release date,
> and we generally start throttling back updates shortly after release while
> we ensure stability.

I think that in certain situations we also increased the 6 weeks period to a 7 weeks period, near end of the year I think that happened at least twice in the last 5 years.  If you add that to the additional week you said about, the count will be at 8 weeks.

Comment 21

[Doug Turner (:dougt)]

Mak, not sure about that.  We know we have an update problem (see comment 0).  If we use 8 weeks, that means these users that aren't getting the update from the normal means will not see an update for 2 weeks after we ship (in the case where we ship every 6 weeks). The patch collects.  If we see it is a problem we can adjust.

Comment 22

[Doug Turner (:dougt)]

The patch collects... telemetry.

Comment 23

[(dormant account)]

Comment on attachment 766240 [details] [diff] [review]
patch v.3

+
+        TelemetryTimestamps.add("checkingForUpdateBasedOnInstallTime");
+

take that out. UPDATE_OLD_BINARY_UPDATED will be lazily instantiated giving you the same effect(assuming one of onCheckComplete or onError always runs). The histogram will not be present in the ping.

Make sure that you intend 
+                let foundUpdate = updateCount > 0;
and 
+             Services.telemetry.getHistogramById("UPDATE_OLD_BINARY_UPDATED").add(false);

to be indistinguishable(otherwise use a 3 value enum histogram and do Services.telemetry.getHistogramById("UPDATE_OLD_BINARY_UPDATED").add(foundUpdate+1);)

Comment 24

[Robert Strong (they/them - no direct email)]

(In reply to Doug Turner (:dougt) from comment #21)
> Mak, not sure about that.  We know we have an update problem (see comment
> 0).
Note: that article refers to "Firefox had stronger growth" which has to do with market share and not an update problem. Can you provide the text in the article that refers to an update problem?

Comment 25

[Robert Strong (they/them - no direct email)]

(In reply to Taras Glek (:taras) from comment #23)
> Comment on attachment 766240 [details] [diff] [review]
> patch v.3
> 
> +
> +        TelemetryTimestamps.add("checkingForUpdateBasedOnInstallTime");
> +
> 
> take that out. UPDATE_OLD_BINARY_UPDATED will be lazily instantiated giving
> you the same effect(assuming one of onCheckComplete or onError always runs).
> The histogram will not be present in the ping.
> 
> Make sure that you intend 
> +                let foundUpdate = updateCount > 0;
> and 
> +            
> Services.telemetry.getHistogramById("UPDATE_OLD_BINARY_UPDATED").add(false);
> 
> to be indistinguishable(otherwise use a 3 value enum histogram and do
> Services.telemetry.getHistogramById("UPDATE_OLD_BINARY_UPDATED").
> add(foundUpdate+1);)
Please use a 3 value enum

Comment 26

[Doug Turner (:dougt)]

hey rob, john can get you all of the data you need about this problem.  Any other changes you'd like to see other than the 3 value enum?

Comment 27

[Doug Turner (:dougt)]

Attachment: patch v.4

Comment 28

[Robert Strong (they/them - no direct email)]

(In reply to Doug Turner (:dougt) from comment #26)
> hey rob, john can get you all of the data you need about this problem.
Not sure if we can... just pointing out that the article doesn't describe the problem as stated in comment #21. Often people jump to the conclusion there is a problem with updates when there isn't (e.g. update rage as described at the previous summit in San Jose where the next week the data self-corrected without anyone finding out what actually happened, the article talking about growth declining and assuming that means there is an update problem when growth isn't about updates, etc.).

>  Any
> other changes you'd like to see other than the 3 value enum?
Yes, I was just commenting on that because Taras left it as optional and I think it should be a 3 value enum.

Comment 29

[(dormant account)]

By the way, if system time can get us into trouble here- Should we use a timestamp from ADU/FHR server response for safety?

Comment 30

[Robert Strong (they/them - no direct email)]

Comment on attachment 766356 [details] [diff] [review]
patch v.4

>+            onCheckComplete: function onCheckComplete(request, updates, updateCount) {
>+                let foundUpdate = updateCount > 0;
>+                Services.telemetry.getHistogramById("UPDATE_OLD_BINARY_UPDATED").add(foundUpdate);
It is important to note for telemetry that the user can still cancel the update if they have to opt-in for the update. This can happen if they have incompatible add-ons with the newer version which has kept people on older builds (e.g. when google toolbar went away, system provided add-ons such as integration with a password manager, antivirus software add-ons, etc.).

>+        let buildDate = new Date(buildID.slice(0,4),     // year
>+                                 buildID.slice(4,6) - 1, // months are zero-based.
>+                                 buildID.slice(6,8),     // day
>+                                 buildID.slice(8,10),    // hour
>+                                 buildID.slice(10,12),   // min
>+                                 buildID.slice(12,14))   // ms
Personally I'd just pad this out since you are only interested in the day but I'm ok with this.

>+        if (buildDate + acceptableAge < today) {
>+            let updater = Cc["@mozilla.org/updates/update-service;1"].getService(Ci.nsIApplicationUpdateService)
nit: keep it under 80 here and elsewhere... end with a ;

>+            updater.backgroundChecker.checkForUpdates(new OldBuildUpdateCheckListener(updater), true); 
nit: get rid of EOL whitespace

Calling back to the aus backgound checker hasn't been done before so I am going to evaluate that before finishing the review.

As shown by dao's drive by, it would be a good thing to have dao, gavin, or someone else on gavin's team to review this as well as previously noted.

Comment 31

[Doug Turner (:dougt)]

> By the way, if system time can get us into trouble here- Should we use a timestamp from ADU/FHR server response for safety?

What's the worst thing that will happen?  An extra http request for old builds at startup?  Not sure its worth the complexity at all.

Comment 32

[Doug Turner (:dougt)]

> As shown by dao's drive by, it would be a good thing to have dao, gavin, or someone else on gavin's team to review this as well as previously noted.

Yup.  After you're good with it, I'll get someone from gavin's team to give a blessing.

Comment 33

[Robert Strong (they/them - no direct email)]

(In reply to Doug Turner (:dougt) from comment #32)
> > As shown by dao's drive by, it would be a good thing to have dao, gavin, or someone else on gavin's team to review this as well as previously noted.
> 
> Yup.  After you're good with it, I'll get someone from gavin's team to give
> a blessing.
You might want to do this prior to my finishing the review. I will be offline for the remainder of today and won't likely get to evaluating the remainder until tomorrow.

Comment 34

[Doug Turner (:dougt)]

> It is important to note for telemetry that the user can still cancel the update if they have to opt-in for the update. 

I think that is a different bug.  Maybe bug 885824?

Comment 35

[Doug Turner (:dougt)]

dao, since you just touched it :)  mind reviewing this?

Also, nsBrowserGlue.js has many lines that use > 80chars.  I am going to stick with what I got for now because it looks a lot nicer.

Comment 36

[Jared Wein [:jaws] (please needinfo? me)]

Would it be simpler if we just made app.update.interval decrease as the build gets older?

Comment 37

[Doug Turner (:dougt)]

jared, I think you're basically right.  We are sort of doing what you suggested... just more steeply than what you're suggesting. :)

The patch basically keeps the app.update.interval constant, then it drops it to "every damn time you startup".  The patch is kind bigger than what you probably are suggesting, but I wanted to collect some telemetry about the ping.

In any case, if you want to write a patch that gets us to the place where we check every time we startup (because there probably is an update for us), I am happy to let you drive! :)

Comment 38

[John Schoenick [:johns]]

I don't have any special metrics outside of the public browser stats, but just looking at the version rundowns in this article makes it pretty clear to me that there's an issue:

Firefox version uptake: http://arstechnica.com/wp-content/uploads/2013/06/firefox-versions-2013-05-640x480.png
Chrome: http://arstechnica.com/wp-content/uploads/2013/06/chrome-versions-2013-05-640x480.png

Both browsers have some percentage of constantly out of date users, but FF has 20-30% that are 2+ revisions old. Even ignoring versions prior to 4, we have twice the out of date users of chrome.

Comment 39

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

(In reply to Doug Turner (:dougt) from comment #21)
> The patch collects telemetry. If we see it is a problem we can adjust.

I wonder whether the set of people having update problems who've also enabled telemetry is large enough that the telemetry data will be useful.

Comment 40

[Doug Turner (:dougt)]

> I wonder whether the set of people having update problems who've also enabled telemetry is large enough that the telemetry data will be useful.

Something we'll have to think about when looking at the data?  Want to review the patch, or let Dao worry about it?

Comment 41

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

I was just voicing a concern, it wasn't meant to be interpreted as stop energy.

The location of the code seems fine, though it would probably be nicer to factor it out into a separate helper function (_checkForOldBuildUpdates) for clarity.

Comment 42

[Robert Strong (they/them - no direct email)]

(In reply to John Schoenick [:johns] from comment #38)
> I don't have any special metrics outside of the public browser stats, but
> just looking at the version rundowns in this article makes it pretty clear
> to me that there's an issue:
> 
> Firefox version uptake:
> http://arstechnica.com/wp-content/uploads/2013/06/firefox-versions-2013-05-
> 640x480.png
> Chrome:
> http://arstechnica.com/wp-content/uploads/2013/06/chrome-versions-2013-05-
> 640x480.png
> 
> Both browsers have some percentage of constantly out of date users, but FF
> has 20-30% that are 2+ revisions old. Even ignoring versions prior to 4, we
> have twice the out of date users of chrome.
Note that Firefox by default has its preferences set to notify users when add-ons are incompatible, Firefox provides UI to disable updates and to notify of updates giving the user the option to decline the update, and we don't download updates in a separate process outside of the browser (this was done this way to limit security exploit vectors and resource / time constraints). As long as all of these things are different I would expect there to be a substantial difference between chrome's update rate and Firefox's update rate. I am not saying there aren't things we can improve though I do suspect that these things (especially the first two) account for the majority of the differences.

Comment 43

[Robert Strong (they/them - no direct email)]

(In reply to Doug Turner (:dougt) from comment #40)
> > I wonder whether the set of people having update problems who've also enabled telemetry is large enough that the telemetry data will be useful.
> 
> Something we'll have to think about when looking at the data?  Want to
> review the patch, or let Dao worry about it?
Hey Doug, I very much agree with Gavin on this and have been thinking about what can be done to provide this while making the call from Firefox into app update a hand off similar to what your original patch using notify did. I should have something along those lines sometime in the next 24 hours.

Comment 44

[Robert Strong (they/them - no direct email)]

(In reply to Jared Wein [:jaws] from comment #36)
> Would it be simpler if we just made app.update.interval decrease as the
> build gets older?
I think this is a good thing but it would also be a good thing to decrease the app.update.timerFirstInterval pref and possibly app.update.timerMinimumDelay in Firefox as well though in a separate bug.

Comment 45

[Doug Turner (:dougt)]

per comment #43.  rob, let me know where I can help.

Comment 46

[Robert Strong (they/them - no direct email)]

(In reply to Doug Turner (:dougt) from comment #45)
> per comment #43.  rob, let me know where I can help.
Thanks Doug, you can help by taking this bug and adding the call I provide in bug 886545.

Comment 47

[Doug Turner (:dougt)]

:P

Comment 48

[Dão Gottwald [:dao]]

Comment on attachment 766356 [details] [diff] [review]
patch v.4

Sounds like there's a new patch coming, canceling review request.

Comment 49

[(dormant account)]

(In reply to Robert Strong [:rstrong] (do not email) from comment #42)
> Note that Firefox by default has its preferences set to notify users when
> add-ons are incompatible, Firefox provides UI to disable updates and to
> notify of updates giving the user the option to decline the update, and we
> don't download updates in a separate process outside of the browser (this
> was done this way to limit security exploit vectors and resource / time
> constraints). As long as all of these things are different I would expect
> there to be a substantial difference between chrome's update rate and
> Firefox's update rate. I am not saying there aren't things we can improve
> though I do suspect that these things (especially the first two) account for
> the majority of the differences.

Learning this made me sad. Do we track the incidence of update opt-out due to such UI via telemetry?

Comment 50

[Doug Turner (:dougt)]

rob, Anything I can do to help move this forward?

Comment 51

[Robert Strong (they/them - no direct email)]

Comment on attachment 766356 [details] [diff] [review]
patch v.4

Not at the moment... other work got in the way bu I am still working on support for this in bug 886545

Comment 52

[Doug Turner (:dougt)]

see comment #49

Comment 53

[Robert Strong (they/them - no direct email)]

Sorry, I missed that and responded to the needinfo from comment #50.

(In reply to Taras Glek (:taras) from comment #49)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #42)
> > Note that Firefox by default has its preferences set to notify users when
> > add-ons are incompatible, Firefox provides UI to disable updates and to
> > notify of updates giving the user the option to decline the update, and we
> > don't download updates in a separate process outside of the browser (this
> > was done this way to limit security exploit vectors and resource / time
> > constraints). As long as all of these things are different I would expect
> > there to be a substantial difference between chrome's update rate and
> > Firefox's update rate. I am not saying there aren't things we can improve
> > though I do suspect that these things (especially the first two) account for
> > the majority of the differences.
> 
> Learning this made me sad. Do we track the incidence of update opt-out due
> to such UI via telemetry?
I've added telemetry for this in bug 886545 (one of the reasons I didn't want to go with the current patch in this bug). Asa has also brought up the possibility of removing the majority of these differences though it is by no means a sure thing.

Comment 54

[Robert Strong (they/them - no direct email)]

Pushed bug 886545 to mozilla-inbound. If it sticks the call to check fo a background update is
const Ci = Components.interfaces;
const Cc = Components.classes;
Cc["@mozilla.org/updates/update-service;1"].getService(Ci.nsIApplicationUpdateService).checkForBackgroundUpdates();

The new telemetry histograms can be seen in the patch in bug 886545

Comment 55

[Robert Strong (they/them - no direct email)]

Note: I changed the component to General since this in now way touches the installer code.

Comment 56

[Robert Strong (they/them - no direct email)]

(In reply to Robert Strong [:rstrong] (do not email) from comment #54)
> Pushed bug 886545 to mozilla-inbound. If it sticks the call to check fo a
> background update is
> const Ci = Components.interfaces;
> const Cc = Components.classes;
> Cc["@mozilla.org/updates/update-service;1"].getService(Ci.
> nsIApplicationUpdateService).checkForBackgroundUpdates();
> 
> The new telemetry histograms can be seen in the patch in bug 886545
FYI: app update support for this bug landed on m-c on 2013-07-12.

Comment 57

[Robert Strong (they/them - no direct email)]

Some recent data points regarding updating

I deliberately stop <browser> from updating to the latest version
Firefox: 15%
IE: 18%
Chrome: 12%

Note: Among Firefox users the top cited reasons for not updating were…
- No benefit to upgrading (41%)
- Issues with add-ons and extensions (38%)

The <browser> updates can sometimes cause me problems
Firefox: 24%
IE: 30%
Chrome: 23%

Note: Among Firefox users the top cited reason for problems was 'Issues with add-ons and extensions'

I highly suspect that the biggest increase in updates we can get would be to force users to update like chrome does but that needs to be considered very carefully since that will disable or break user add-ons.

Comment 58

[Doug Turner (:dougt)]

Attachment: patch v.5

Comment 59

[Robert Strong (they/them - no direct email)]

Comment on attachment 777499 [details] [diff] [review]
patch v.5

The app.update.checkInstallTime preference is Firefox specific and not used by app update so use a different prefix for the pref. Perhaps browser.update.checkInstallTime

I've already reviewed the rest previously except for:
1. the intervals you have chosen which though I am ok with someone else should r+ those additions.
2. The call to checkForBackgroundUpdates which I am fine with

Comment 60

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

Comment on attachment 777499 [details] [diff] [review]
patch v.5

Aurora should probably use the same value as Nightly/Unofficial, since it's possible for it to update as frequently AIUI (it builds nightly if there are checkins).

I haven't looked into what checkForBackgroundUpdates does or whether it's reasonable to call it here, but I expect rstrong covered that.

Comment 61

[Dão Gottwald [:dao]]

Comment on attachment 777499 [details] [diff] [review]
patch v.5

>--- a/browser/components/nsBrowserGlue.js
>+++ b/browser/components/nsBrowserGlue.js
>@@ -591,6 +591,28 @@ BrowserGlue.prototype = {
>     }
> #endif
> 
>+     // check for update if our build is old
>+     if (Services.prefs.getBoolPref("app.update.enabled") &&
>+       Services.prefs.getBoolPref("app.update.checkInstallTime")) {
>+
>+       let buildID = Services.appinfo.appBuildID;
>+       let today = new Date().getTime();
>+       let buildDate = new Date(buildID.slice(0,4),     // year
>+                                buildID.slice(4,6) - 1, // months are zero-based.
>+                                buildID.slice(6,8),     // day
>+                                buildID.slice(8,10),    // hour
>+                                buildID.slice(10,12),   // min
>+                                buildID.slice(12,14))   // ms
>+           .getTime();
>+
>+       const millisecondsIn24Hours = 86400000;
>+       let acceptableAge = Services.prefs.getIntPref("app.update.checkInstallTime.days") * millisecondsIn24Hours;
>+
>+       if (buildDate + acceptableAge < today) {
>+         Cc["@mozilla.org/updates/update-service;1"].getService(Ci.nsIApplicationUpdateService).checkForBackgroundUpdates();
>+       }
>+     }
>+
>     this._trackSlowStartup();
> 
>     // Offer to reset a user's profile if it hasn't been used for 60 days.

Indentation is off for your whole block. Also, can you follow the recommendation from the the second part of comment 41?

Comment 62

[Nick Thomas [:nthomas] (UTC+12)]

We'll need to set app.update.checkInstallTime.days longer than 42 on the official branding (release channel). Those builds are created about a week before the release day, there's typically a week-long soft launch where background updates checks aren't given an update, then a week of solid updating before hitting the long tail. A value of 63 days would typically have about 60% the previous version updated.

Comment 63

[Doug Turner (:dougt)]

Attachment: patch to checkin

Comment 64

[Nick Thomas [:nthomas] (UTC+12)]

Some questions about this change. IIRC we know that the average Firefox session can be pretty short (a few mins) so presumably there will be lots of app starts doing update checks once builds get old. So
* do we know if users will be prompted to update a lot ? Does it matter if they have incompatible add-ons ?
* for IT's sake, are there any estimates how many extra requests the update server would need to serve ? Or add-on compatibility checks served out of AMO ? I'd like  to make sure we're not building ourselves a DDOS, and/or big increases in d/l traffic if people are stuck in failing update loops (download-fail-download-fail-repeat)

Perhaps it would also be good to append a startup=1 param (or similar) on the update query so that we can track how many of these queries we're getting, given low opt-in on telemetry, and possibly handle them differently on the update server.

Comment 65

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/1db61d5c5510

Comment 66

[Doug Turner (:dougt)]

nthomas: file follow ups

Comment 67

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1db61d5c5510