534090 - do not use background notification for major updates (was PMU 3.0->3.5 major update has been really poor)

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect, P1)

Description

[Samuel Sidler (old account; do not CC)]

Our prompted major update (PMU) isn't getting good uptake. The last one we had also didn't do as well as in the 2.0.0.x -> 3.0.x days. We attributed this to the timing of that update, but in this case, we've only see about 1m people accept the major update, well below expectations and well below what has happened in every other major update (usually it's as low as 10%).

This bug is to track the work we're doing to figure out what's going on and to track any work we need to do to fix problems found.

I'm nominating this for blocking across branches because if this is a UI issue we caused, it needs to block our next maintenance releases (to fix 3.0.x -> 3.5.x and 3.5.x -> 3.6.x) and, if it's a more critical change, might need to block 3.6 (like, if there are string changes). That's a long shot (and unlikely) but I like to be safe rather than sorry.

(And I'll own this for now.)

Comment 1

[Samuel Sidler (old account; do not CC)]

Ken got the following result from his survey:

  * Hello, I didn't want to upgrade from Firefox 3.0.15 to 3.5.5. However, when
    I clicked on my Firefox desktop icon, it updated itself to 3.5.5 without my
    consent. I didn't click on anything else. It updated on start-up. What
    happened?  Has something changed with the Firefox update process?  This was
    pretty annoying, and now I'm nervous about trying to switch back to 3.0.15.
    This probably isn't the right method to ask this question, but I thought it
    was worth a try.

Ken also saw weirdness himself on an old computer at home... He received a small notification instead of the expected window appearing. Al saw the same thing. Clicking the notification (in the bottom right on Windows) opened the expected window, but this is a change from 2.0.0.x -> 3.0.x. This could explain some of the uptake slowness and we should revert that change ASAP.

Comment 2

[Al Billings [:abillings - ex-MoCo]]

Attachment: Firefox 3 MU Prompt example

One possibility for the cause of the issue is that when a Major Update is available to Firefox 3.0.x users, the prompt that they see uses the same mechanism as the "download complete" prompts: a slider at the lower right that pops up for a couple of seconds and then self-dismisses. There is no other notification. If the user clicks on the slider, then the full Major Update dialog (with the buttons to update now, later, etc.) appears. If they don't click on the slider, then the user doesn't seem another prompt until the update interval passes and this prompt is simply another slider.

Comment 3

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

Bug 391598 was where we re-worked this pre-3.0 and introduced the unobtrusive notification.

Comment 4

[Al Billings [:abillings - ex-MoCo]]

My testing was after setting app.update.interval to "60" and app.update.timer to "1000". I saw no big MU dialog unless I clicked on the little slider.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

That's ... alarming.

The desired behaviour, ftr, is:

// minor update

Downloads automatically, once the download is complete and the update is available, we use the same download notification service as is used for file downloads to indicate that an update is available and will be applied on restart. If the browser is not restarted within 12 hours, then we get more aggressive and pop up a wizard page after a few minutes without keyboard activity.

// major update

The minute we get a major update snippet, we wait for a few minutes without keyboard activity and pop the major update dialog wizard with the billboard advertising the new version and asking the user if they want to update.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Gavin's telling me that the desired behaviour as described in comment 5 is never what was implemented, which is simply shocking to me.

Comment 7

[Samuel Sidler (old account; do not CC)]

We should back out bug 391598 then (at least the part that made this unobtrusive). (Who did that ui-review anyway? ;))

We can revisit how we do this in the 3.7-timeframe to make it less obtrusive without killing our uptake...

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

And if that's the case, then what's happening is that users who do not leave their browser open for 12 hours (ie: they close it once a day) will never see the major update dialog, only a little notification slider, since major updates aren't automatically applied (as minor updates are). 

The good news is that fix would be pretty easy, which would be to remove the "background" parameter when we get a major update.

Comment 9

[Al Billings [:abillings - ex-MoCo]]

I changed the setting app.update.promptWaitTime (which I just found out about) from "43200" to "120" (seconds) and I got the big MU dialog, unprompted, after two minutes.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Looks like that was the case; reviewing bug 39158, it looks like I had been assuming that we were only talking about minor updates, though I never made that explicitly clear. I further expected that QA was testing automatic delivery, and it turns out that they weren't (but I bet they will from now on!)

We need to fix this immediately on all branches. Added bonus: we're about to goose our Firefox 3.5 numbers!

Comment 11

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

(In reply to comment #7)
> We should back out bug 391598 then (at least the part that made this
> unobtrusive).

No, that'd be far too much code churn (and pretty risky since we've made quite a few changes on top of it).

I think what we actually want is to change the call to showUpdateDownloaded() in Downloader's onStopRequest() to only pass "true" for "background" if the update is not a major update.

Comment 12

[Mike Connor [:mconnor]]

Attachment: untested

Did I mention: untested?

Comment 13

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

Looks like the same root cause as the cause of bug 462568. I'll take a look at mconnor's approach tomorrow.

Comment 14

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

I went through the different scenarios this morning and though this is better than current behavior I believe a bit more should be done here since a minor update that has incompatible add-ons will also exhibit this same behavior.

Comment 15

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

I highly suspect the issue that bug 391598 was trying to fix at least in part was due to app update not being able to tell if an add-on was in fact incompatible with the next release which I fixed after taking over app update in the 3.5 cycle. As of 3.5 the only times we will ever show a prompt are:
1. major update
2. minor update with the warn me if this will disable any add-ons checkbox checked.
3. All updates with ask me what I want to do checked.

By making the behavior from bug 391598 only apply to major updates the other two cases will still not receive notification of the update.

Since as of 3.5 the default behavior for minor updates we now only notify when an enabled / compatible add-on will be disabled I think we should go with the same behavior for notification of minor updates in that the UI would be displayed on idle since it is now the exception instead of the rule.

Comment 16

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

Attachment: alternative patch

This is untested but it will soon be.

I prefer taking this approach for the reasons stated in comment #15... comments for and against are welcome.

Comment 17

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

Also note that Bug 391598 was a spinoff of Bug 334767 which is about "don't show restart now or later dialog after automatic background download" and this patch doesn't affect that behavior whatsoever... it only affects notification of an update available per the conditions stated in comment #15.

Comment 18

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

Attachment: patch rev1

Comment 20

[Dave Townsend [:mossop]]

With this as it is if the user has been idle for longer than IDLE_TIME when the update is detected then we will display both the alert notification and open the UI immediately. This doesn't seem ideal.

Comment 21

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

Agreed that showing both the toast notification and the prompt UI isn't ideal but that seems rather minor especially since this should be a minimal due to it being for 1.9.0.17. Having said that, the additional complexity added to _showUnobtrusiveUI to prevent this isn't all that much and should be safe.

Comment 22

[Dave Townsend [:mossop]]

Comment on attachment 417173 [details] [diff] [review]
patch rev1

This is ok from a code standpoint so long as beltzner is ok with that double notification behaviour.

Comment 23

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

Attachment: patch rev2

Comment 24

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

Comment on attachment 417194 [details] [diff] [review]
patch rev2

Hey beltzner, running this by you to make sure it is ok with you. This makes it so:
if not idle - an update alert notification will  be shown and then the UI will be shown on idle.
if idle - the UI will be shown on idle.

The reason for choosing this behavior is outlined in comment #15 in that this also affects minor updates under certain circumstances.

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 417194 [details] [diff] [review]
patch rev2

Yup, this seems right. For minor updates where everything will go smoothly (no add-on compatibility issues) then show notification, don't show UI for 12 hours on the expectation that the user will restart within that frame, and after 12 hours show the UI.

For major updates or minor updates with complications, wait for first available idle and then show the UI.

uir=beltzner

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Rob, do we have automated tests or would it require a Litmus/Mozmill test?

Comment 27

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

No automated tests as of yet but I'll try to add a mochitest for this next week. Adding this to Litmus/Mozmill would be a good thing as well.

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

QA (in the form of Al) informed me that we don't currently test the automatic delivery of Major Update offers, only manual "Check for Update..." testing. We definitely need a litmus/mozmill test that sets the timeout values to an appropriate level for testing, and make sure we include it in every Major Update QA test plan.

Comment 29

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

btw: we have some testing in xpcshell for the notification path without displaying the ui but it can't cover everything. Previously we've had issues with update snippets which can't be covered by automated testing since they are generated per release etc. so no matter what is done to automate this I believe we'll always need litmus/mozmill to test this. Having said that, I have a couple of ideas for adding tests for this specific issue that I'll try to implement this week.

I'll land this after the electrolysis landing / closure is over on Monday.

Comment 30

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

(In reply to comment #3)
> Bug 391598 was where we re-worked this pre-3.0 and introduced the unobtrusive
> notification.

Do we know what release of Firefox first had this regression?

Comment 31

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

Per bug 462568 comment #7 "3.0b2 was the first milestone with this behavior."

Comment 32

[Al Billings [:abillings - ex-MoCo]]

To be clear, it isn't a regression.

Comment 33

[Mike Beltzner [:beltzner, not reading bugmail]]

To be clearer, it is a regression. It's a change in behaviour that regressed the functionality of the feature. The fact that the regression occurred because neither the reviewer nor the ui-reviewer nor QA caught the fact that it changed the beahviour in unwanted and unintended ways doesn't make it any less of a regression.

Comment 34

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

(In reply to comment #29)
>...
> I'll land this after the electrolysis landing / closure is over on Monday.
Tree is still closed so this probably won't land until Tuesday.

Comment 35

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

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/54b46c4ec441

Comment 36

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

pushed to mozilla-1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/fed4bcabc62a

Comment 37

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

Comment on attachment 417194 [details] [diff] [review]
patch rev2

Requesting branch approvals

Comment 38

[Daniel Veditz [:dveditz]]

Comment on attachment 417194 [details] [diff] [review]
patch rev2

Approved for 1.9.1.7 and 1.9.0.17, a=dveditz for release-drivers

Comment 39

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

Pushed to mozilla-1.9.1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/52052a4ad13b

Checked in to the CVS HEAD for 1.9.0.17
Checking in mozilla/toolkit/mozapps/update/src/nsUpdateService.js.in;
/cvsroot/mozilla/toolkit/mozapps/update/src/nsUpdateService.js.in,v  <--  nsUpda
teService.js.in
new revision: 1.156; previous revision: 1.155
done

Comment 40

[Chris AtLee [:catlee]]

Does this need to go on the relbranches for 3.5.6 and 3.0.16 as well?

Comment 41

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

My understanding was this was to land for 3.5.7 and 3.0.17 only.

Comment 42

[Chris AtLee [:catlee]]

Yeah, and I thought we were doing the 3.5.7 and 3.0.17 releases from the previous relbranch.

Comment 43

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

That I hadn't heard.

Comment 44

[Al Billings [:abillings - ex-MoCo]]

That is my understanding as well.

Comment 45

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

Thanks, I emailed dveditz for confirmation

Comment 46

[Daniel Veditz [:dveditz]]

Comment on attachment 417194 [details] [diff] [review]
patch rev2

The previous .7/.17 releases were renamed .8/.18, approved for the NEW 1.9.1.7 and 1.9.0.17, a=dveditz for release-drivers

This means checking in on the 1.9.1.6/1.9.0.16 relbranches.

hg(191): GECKO1916_20091130_RELBRANCH 
cvs(190): GECKO190_20091130_RELBRANCH

Comment 47

[Daniel Veditz [:dveditz]]

Please change status1.9.1 from .8-fixed to .7-fixed when it's landed on the relbranch. QA will verify on both before adding the verified keyword.

For the 1.9.0 tree please add the fixed1.9.0.17 keyword when you land, and leave the fixed1.9.0.18 keyword there as well.

Comment 48

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

Pushed to mozilla-1.9.1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ecdc4b1972f3

Moved tip back to default by fixing "No newline at end of file" for nsUpdateService.js.in
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8d42d253857f

Checked in to the CVS GECKO190_20091130_RELBRANCH for 1.9.0.17
Checking in mozilla/toolkit/mozapps/update/src/nsUpdateService.js.in;
/cvsroot/mozilla/toolkit/mozapps/update/src/nsUpdateService.js.in,v  <--  nsUpda
teService.js.in
new revision: 1.155.6.1; previous revision: 1.155
done

Comment 49

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

(In reply to comment #48)
> Pushed to mozilla-1.9.1
> http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ecdc4b1972f3
should be pushed to mozilla-1.9.1 branch GECKO1916_20091130_RELBRANCH

Comment 50

[juan becerra [:juanb]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.17) Gecko/2009122116 Firefox/3.0.17 (.NET CLR 3.5.30729)

I edited app.update.interval, app.update.timer to 60 and 1000 respectively. The notification came up after 60 seconds while not-idle. The dialog window comes after a minute of idle.

However, when I keep working on the app, and letting the notifications go by, and then I let it idle for a minute I get more than one MU UI dialog http://grab.by/1m3Z

Could someone else help test this? I'm afraid messing with the app.update preferences could be causing this, but it could still be a problem with the app.

Comment 51

[juan becerra [:juanb]]

I forgot to clear the "verified1.9.0.17" keyword before I investigated further and posted the above comment, so changing back to "fixed1.9.0.17"

Comment 52

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

(In reply to comment #50)
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.17) Gecko/2009122116
> Firefox/3.0.17 (.NET CLR 3.5.30729)
> 
> I edited app.update.interval, app.update.timer to 60 and 1000 respectively. The
> notification came up after 60 seconds while not-idle. The dialog window comes
> after a minute of idle.
> 
> However, when I keep working on the app, and letting the notifications go by,
> and then I let it idle for a minute I get more than one MU UI dialog
> http://grab.by/1m3Z
> 
> Could someone else help test this? I'm afraid messing with the app.update
> preferences could be causing this, but it could still be a problem with the
> app.
I've investigated that and it is primarily due to the extremely short interval being used to test. The fix for this will be somewhat invasive and not something I want to try to fix for branches since the vast majority of users will never see this.

Comment 53

[Mike Beltzner [:beltzner, not reading bugmail]]

Rob, is there any way that a user would end up seeing multiple update offers?

Comment 54

[Mike Beltzner [:beltzner, not reading bugmail]]

I'm guessing if the user left their browser open for more than a day (default app.update.interval), and didn't attend to the first major update notice, they'd get multiple major update notices, right? 

I'm pretty sure that's OK, though it would be nice to get a followup bug filed.

Comment 55

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

I added code for 3.5 and above that should (I don't recall if I verified that it does) prevent multiple prompts from happening but when the timer is set to such a low number multiple windows are opened before the window is created so it is unable to detect that a window already exists.

With 3.0 for each 24 hours after the first prompt is displayed a second prompt will be displayed. I am fairly certain that this is the case without this patch as well.

I'll file a followup

Comment 56

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

(In reply to comment #55)
>...
> I'll file a followup
meant to say I'll verify and file a followup if necessary

Comment 57

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #27)
> No automated tests as of yet but I'll try to add a mochitest for this next
> week. Adding this to Litmus/Mozmill would be a good thing as well.

Alright. Al, can you update the MU subgroup on the 3.5 branch in Litmus to reflect this bug? Once the tests are ready I can implement the Mozmill tests.

Comment 58

[Al Billings [:abillings - ex-MoCo]]

I've verified this again (as did Juan, really) for 1.9.0.17. The bug that Juan saw is actually worse in that if you get more than one MU dialog up at the same time, the subsequent dialogs are blank (with no MU information, unlike what Juan saw) and none of the buttons work. The only way to dismiss them is to click on the "X" for close window. 

That said, users shouldn't normally see multiples. I ran this by Beltzner and Dan Veditz on the phone.

Comment 59

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

I just checked 3.6 which uses the same code as 3.5 and as long as the timers aren't set to fire at a ridiculously fast rate multiple prompts are not shown due to the code I added for 3.5 to prevent this. I'll file a followup bug to enforce a reasonable minimum which will likely be around 60 seconds.

Comment 60

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

btw: I'll also check 3.0.x as time permits

Comment 61

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

I looked at enforcing reasonable minimums and though it is easy to do so it then makes the timer manager xpcshell tests take quite a bit longer. At least for the time being I would prefer it if QA uses the following values when testing app update.

The idle time must be less than the interval
user_pref("app.update.idletime", 1);

checks for updates every 30 seconds
user_pref("app.update.interval", 30);

timer fires every 30 seconds
user_pref("app.update.timer", 30000);

Comment 62

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

(In reply to comment #60)
> btw: I'll also check 3.0.x as time permits
I just checked the behavior using
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.0.18pre) Gecko/2010010605 GranParadiso/3.0.18pre

with the above preferences and it does the right thing in that multiple update prompts are not shown.

So, the bug described in comment #58 is caused by setting the timer to fire multiple times before the update check has completed and the ui is shown... I believe the value used by QA for app.update.timer is 60 which makes the timer fire every 60 milliseconds.

I'll look at ways to safeguard against this but I think the priority of guarding against this is a really low priority.

Comment 63

[Jean-Marie COUPRIE]

Sorry to comment so late but I think that there are problems that you have not seen in the discussion of this bug. I have just discovered this bug reading what is new in 3.5.7.

1)I am a Security minded personal user : I run in non-admin account for my normal daily and web surfing activity. According the security sites advices, I divide by 4 to 5 the probability that malwares install with this. But with a non admin account an update cannot be installed and I have an error message at the beginning of the Firefox session. See my Bug 500303.

2) I have 3 profiles with different add-ons installed (add-ons are installed in the profile and not common to all of them). The update program can easily check which add-ons are in the active profile and if they are compatible. But how can it do the same in the 2 inactive profiles ?

3) I think that the automatic update should be cut in 2 parts :
a) download of the update even with a limited account and test if account has administrator rights or right to write in the case of limited user installed Firefox. If not, send a message to the user asking to go to administrator account without being connected to the net.
b) installation only if the previous test is OK.
b)install only

Comment 64

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

(In reply to comment #63)
I'll followup in bug 500303

Comment 65

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

Filed bug 540356 with a patch to enforce a sane minimum for app.update.timer.