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.)
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.
Created attachment 417007 [details] 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.
Bug 391598 was where we re-worked this pre-3.0 and introduced the unobtrusive notification.
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.
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.
Gavin's telling me that the desired behaviour as described in comment 5 is never what was implemented, which is simply shocking to me.
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...
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.
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.
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!
(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.
Looks like the same root cause as the cause of bug 462568. I'll take a look at mconnor's approach tomorrow.
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.
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.
Created attachment 417163 [details] [diff] [review] 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.
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.
Created attachment 417173 [details] [diff] [review] patch rev1
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.
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 188.8.131.52. Having said that, the additional complexity added to _showUnobtrusiveUI to prevent this isn't all that much and should be safe.
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 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 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
Rob, do we have automated tests or would it require a Litmus/Mozmill test?
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.
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.
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.
(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?
Per bug 462568 comment #7 "3.0b2 was the first milestone with this behavior."
To be clear, it isn't a regression.
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.
(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.
Pushed to mozilla-central http://hg.mozilla.org/mozilla-central/rev/54b46c4ec441
pushed to mozilla-1.9.2 http://hg.mozilla.org/releases/mozilla-1.9.2/rev/fed4bcabc62a
Comment on attachment 417194 [details] [diff] [review] patch rev2 Requesting branch approvals
Comment on attachment 417194 [details] [diff] [review] patch rev2 Approved for 184.108.40.206 and 220.127.116.11, a=dveditz for release-drivers
Pushed to mozilla-1.9.1 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/52052a4ad13b Checked in to the CVS HEAD for 18.104.22.168 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
Does this need to go on the relbranches for 3.5.6 and 3.0.16 as well?
My understanding was this was to land for 3.5.7 and 3.0.17 only.
Yeah, and I thought we were doing the 3.5.7 and 3.0.17 releases from the previous relbranch.
That I hadn't heard.
That is my understanding as well.
Thanks, I emailed dveditz for confirmation
Comment on attachment 417194 [details] [diff] [review] patch rev2 The previous .7/.17 releases were renamed .8/.18, approved for the NEW 22.214.171.124 and 126.96.36.199, a=dveditz for release-drivers This means checking in on the 188.8.131.52/184.108.40.206 relbranches. hg(191): GECKO1916_20091130_RELBRANCH cvs(190): GECKO190_20091130_RELBRANCH
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 fixed220.127.116.11 keyword when you land, and leave the fixed18.104.22.168 keyword there as well.
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 22.214.171.124 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: 126.96.36.199; previous revision: 1.155 done
(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
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:188.8.131.52) 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 forgot to clear the "verified184.108.40.206" keyword before I investigated further and posted the above comment, so changing back to "fixed220.127.116.11"
(In reply to comment #50) > Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:18.104.22.168) 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.
Rob, is there any way that a user would end up seeing multiple update offers?
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.
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
(In reply to comment #55) >... > I'll file a followup meant to say I'll verify and file a followup if necessary
(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.
I've verified this again (as did Juan, really) for 22.214.171.124. 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.
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.
btw: I'll also check 3.0.x as time permits
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);
(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:126.96.36.199pre) 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.
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
Filed bug 540356 with a patch to enforce a sane minimum for app.update.timer.