Last Comment Bug 657472 - 'Tune' the time to wait before displaying the update been downloaded / restart notification and provide ability to override in the update xml
: 'Tune' the time to wait before displaying the update been downloaded / restar...
Status: VERIFIED FIXED
: qawanted
Product: Toolkit
Classification: Components
Component: Application Update (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
Depends on: 659425
Blocks: 706798
  Show dependency treegraph
 
Reported: 2011-05-16 14:14 PDT by Robert Strong [:rstrong] (use needinfo to contact me)
Modified: 2011-12-01 04:56 PST (History)
20 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed


Attachments
patch v1 (2.15 KB, patch)
2011-09-29 15:47 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v2 (1.17 KB, patch)
2011-10-01 01:26 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v3 (2.35 KB, patch)
2011-10-04 01:16 PDT, Tim Taubert [:ttaubert]
robert.strong.bugs: review-
Details | Diff | Splinter Review
patch v4 (6.10 KB, patch)
2011-10-05 03:15 PDT, Tim Taubert [:ttaubert]
robert.strong.bugs: review+
bugzilla: approval‑mozilla‑aurora+
bugzilla: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Screenshot (315.91 KB, image/png)
2011-11-03 06:33 PDT, Paul Silaghi, QA [:pauly]
no flags Details
Screenshot2 (251.53 KB, image/png)
2011-11-03 06:34 PDT, Paul Silaghi, QA [:pauly]
no flags Details

Description Robert Strong [:rstrong] (use needinfo to contact me) 2011-05-16 14:14:05 PDT
Per Johnath's and Jay's request I discussed with limi what we should do with the notification that there is an update downloaded / ready to install that occurs 12 hours after an update has been downloaded. Per limi, this notification should just be removed.

Though not part of this bug, if we want to make users restart we should create a new method that can be controlled by the update snippet.
Comment 1 Justin Dolske [:Dolske] 2011-05-16 14:39:34 PDT
Would this block on having that "new method" available? Seems like if we release a security update for a 0-day exploit, we'd want to make sure users update in a timely manner (which, at least in theory, is part of the purpose of this notification).
Comment 2 Robert Strong [:rstrong] (use needinfo to contact me) 2011-05-16 14:44:53 PDT
In discussing this with UX it wouldn't block though I personally think it might. I do hope that we get the design for the new notification system from UX in time for the two to land during the same cycle.
Comment 3 Robert Strong [:rstrong] (use needinfo to contact me) 2011-05-16 14:59:28 PDT
Discussed this with a few people and the consensus is that we should have a new notification system in place. I've discussed this briefly with limi and faaborg so they are aware that we need a UX design for this.

Tim, sorry for the scope creep. For now, a patch to remove the existing notification would help familiarize you with the existing code in app update and specifically the update prompt.
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#2853
Comment 4 Robert Strong [:rstrong] (use needinfo to contact me) 2011-05-24 20:51:51 PDT
Per a meeting today we are going to go with the existing notification, 'tune' when the notification is displayed by default after x amount of time as determined by bug 659425, and provide the ability for the update xml to override the amount of time to wait before displaying the notification.
Comment 5 Asa Dotzler [:asa] 2011-09-28 21:01:02 PDT
We should set this at 24 or 36 hours (I lean towards 36 but if sec team requires 24, I'll go with that) and get it into Beta and Aurora ASAP. We can implement a better system later, but this seems like a big and relatively quick win for 'silent update'. Can you generate patches for trunk, aurora and beta that simply changes the interval from what it is today to 36 hours and after reviews request approval for landing on branches? This really could have made Firefox 7, I suspect, and I don't want us to miss 8 and 9 with the simple pref change.
Comment 6 Tim Taubert [:ttaubert] 2011-09-29 15:47:52 PDT
Created attachment 563578 [details] [diff] [review]
patch v1

This patch changes the time to wait before showing the update prompt to 36 hours.
Comment 7 Robert Strong [:rstrong] (use needinfo to contact me) 2011-09-29 15:52:35 PDT
Comment on attachment 563578 [details] [diff] [review]
patch v1

iirc the security team wanted to go with 24 hours... I'll double check when I have a minute and post what I find in this bug.

Let's not change the default used in app update and instead just change the preference.

Before this goes in I or someone else needs to review / verify there are no harmful side affects from this change.
Comment 8 Robert Strong [:rstrong] (use needinfo to contact me) 2011-09-30 20:55:13 PDT
Just found the email where Lucas agreed to increasing the time to 24 hours.
Comment 9 Tim Taubert [:ttaubert] 2011-10-01 01:26:03 PDT
Created attachment 563944 [details] [diff] [review]
patch v2

This patch changes the value of app.update.promptWaitTime to 24 hrs.
Comment 10 Robert Strong [:rstrong] (use needinfo to contact me) 2011-10-03 14:26:10 PDT
Comment on attachment 563944 [details] [diff] [review]
patch v2

Thanks!
Comment 11 Asa Dotzler [:asa] 2011-10-03 20:16:35 PDT
Robert, I'd like to try to rush this into Aurora, and possibly Beta. It appears "really safe", well understood, and a potentially solid win on silencing updates that could come out in advance of the more difficult work. Do you think that's reasonable? (we could go straight to beta and not do this on aurora or trunk because we don't actually mind of those people get pushed to update sooner since builds are more frequent than the interval.)
Comment 12 Robert Strong [:rstrong] (use needinfo to contact me) 2011-10-03 20:31:50 PDT
I'm fine with this going to Aurora and Beta and don't think we want this on nightly.
Comment 13 Robert Strong [:rstrong] (use needinfo to contact me) 2011-10-03 20:40:49 PDT
Comment on attachment 563944 [details] [diff] [review]
patch v2

Tim, since we don't want this everywhere can you move the pref into the individual browser/branding preferences? Removing the approval since this is likely not wanted on trunk.
Comment 14 Tim Taubert [:ttaubert] 2011-10-04 01:16:25 PDT
Created attachment 564478 [details] [diff] [review]
patch v3

(In reply to Robert Strong [:rstrong] (do not email) from comment #13)
> Tim, since we don't want this everywhere can you move the pref into the
> individual browser/branding preferences? Removing the approval since this is
> likely not wanted on trunk.

Yeah, that totally makes sense to not have it in nightly. I added the pref changes to the brandings "aurora" and "official". I don't know exactly what the "unofficial" branding is used for but I figured we might not want to touch that.
Comment 15 Robert Strong [:rstrong] (use needinfo to contact me) 2011-10-04 11:56:43 PDT
Comment on attachment 564478 [details] [diff] [review]
patch v3

Looks good but please add it to the following as well:
browser/branding/unofficial/pref/firefox-branding.js

though we don't use unofficial in any of our builds there is talk of making that the default for personal builds and I'd prefer it having the larger value of 24 hours.

The pref needs to be removed from:
browser/app/profile/firefox.js

The original value needs to be added to:
browser/branding/nightly/pref/firefox-branding.js

I *think* it would be a good thing to have a smaller value than 12 hours for nightly but I would prefer for that to be done in another bug if we decide we want to do that.

Should be a simple r+ with the above and thanks!
Comment 16 Tim Taubert [:ttaubert] 2011-10-05 03:15:19 PDT
Created attachment 564777 [details] [diff] [review]
patch v4
Comment 17 Robert Strong [:rstrong] (use needinfo to contact me) 2011-10-05 11:25:59 PDT
Comment on attachment 564777 [details] [diff] [review]
patch v4

Thanks Tim!
Comment 18 Asa Dotzler [:asa] 2011-10-05 11:35:32 PDT
Comment on attachment 564777 [details] [diff] [review]
patch v4

We want to get this into Beta [8] so when they're updating from 8 to 9 they have this improved experience. We'll also want to get this into the next Beta [9].

I'm not sure if this is the right time increment for Aurora and I'm sure it's not right for m-c builds.
Comment 19 Robert Strong [:rstrong] (use needinfo to contact me) 2011-10-05 11:48:47 PDT
(In reply to Asa Dotzler [:asa] from comment #18)
> Comment on attachment 564777 [details] [diff] [review] [diff] [details] [review]
> patch v4
> 
> We want to get this into Beta [8] so when they're updating from 8 to 9 they
> have this improved experience. We'll also want to get this into the next
> Beta [9].
> 
> I'm not sure if this is the right time increment for Aurora and I'm sure
> it's not right for m-c builds.
It is the same as it currently is on m-c and if this is to be changed on m-c it will be done in another bug. If you want to evaluate a different value for aurora it can be done in that bug as well or a different bug. I think the value is fine for aurora but if you prefer we can put it back to 12 hours in this bug.
Comment 20 Johnathan Nightingale [:johnath] 2011-10-07 12:39:19 PDT
Comment on attachment 564777 [details] [diff] [review]
patch v4

Discussed with Robert and Asa. Please land it across the board
Comment 21 christian 2011-10-07 12:41:12 PDT
We'll want extra QA attention on this, adding QA wanted.
Comment 23 Rob Campbell [:rc] (:robcee) 2011-10-11 12:57:36 PDT
https://hg.mozilla.org/mozilla-central/rev/fb18dd9ee477
Comment 24 Kelley Cook 2011-10-14 05:59:33 PDT
This was put on beta, so shouldn't the target milestone be "mozilla8" ?
Comment 25 christian 2011-10-18 12:59:58 PDT
Nope, it landed on central so it is mozilla10. I set the status flags for the branches appropriately.
Comment 26 Vlad [QA] 2011-10-19 08:15:10 PDT
Hi guys. How can I test this? The patch will land on beta as well?
thanks
Comment 27 Robert Strong [:rstrong] (use needinfo to contact me) 2011-10-19 11:32:37 PDT
You can download an update, close the about dialog without restarting, and leave Firefox open for 24 hours to see if it brings up the update window asking to restart.
Comment 28 Vlad [QA] 2011-10-28 05:42:21 PDT
Hi Robert.
I have a question, what if I don't go to Help/About page and simply let the Firefox open, when will the download of the new version start ? In Tools/Options/Advanced is checked "Automatically download and install the update", so I guess it should automatically start

thanks
Comment 29 Robert Strong [:rstrong] (use needinfo to contact me) 2011-10-31 10:49:19 PDT
I'm fairly certain that should be enough.
Comment 30 Paul Silaghi, QA [:pauly] 2011-11-03 06:33:06 PDT
Created attachment 571623 [details]
Screenshot

I've verified this bug on Aurora 9.0a2 (2011-10-10) and Beta 8.0b3 on WinXP and Ubuntu executing following steps:
1. Install Firefox
2. Launch Firefox on a clean profile
3. Go to Help/About Firefox
4. After the new version is downloaded, simply close the About dialog and leave Firefox running for at least 24 hours.

Expected result:
After 24 hours a notification to restart Firefox in order to apply the update should be displayed

Actual results:
An extension update notification is displayed (please see screenshots attached). No notification to restart Firefox and apply the update occurs.
Comment 31 Paul Silaghi, QA [:pauly] 2011-11-03 06:34:55 PDT
Created attachment 571625 [details]
Screenshot2
Comment 32 Robert Strong [:rstrong] (use needinfo to contact me) 2011-11-03 14:18:59 PDT
I'll test and get back with the results tomorrow.
Comment 33 Robert Strong [:rstrong] (use needinfo to contact me) 2011-11-03 14:54:13 PDT
The problem is the notification is only applicable to background downloads.

The simplest way to test would be to launch a version of Firefox that has an update available with a new profile and then wait over 24 hours (25 should be enough).
Comment 34 Lawrence Mandel [:lmandel] (use needinfo) 2011-11-03 16:07:08 PDT
Rob, is there a count down timer or can we do some trickery like set our clock ahead 25 hours after the download completes?
Comment 35 Alex Keybl [:akeybl] 2011-11-03 16:26:58 PDT
(In reply to Robert Strong [:rstrong] (do not email) from comment #32)
> I'll test and get back with the results tomorrow.

Is there any way to speed this test up? The sooner we know the state of this bug, the better. Can we change the system clock or fast forward the VM 25 hours?
Comment 36 Alex Keybl [:akeybl] 2011-11-03 17:54:50 PDT
(In reply to Lawrence Mandel [:lmandel] from comment #34)
> Rob, is there a count down timer or can we do some trickery like set our
> clock ahead 25 hours after the download completes?

Whoops my previous comment was obviously redundant. Let me just add that there's a desire to accelerate the testing in case the proposed plan doesn't succeed for some reason (and a re-test is necessary), or we need to re-spin for any regressions found here.
Comment 37 Robert Strong [:rstrong] (use needinfo to contact me) 2011-11-03 20:26:03 PDT
(In reply to Lawrence Mandel [:lmandel] from comment #34)
> Rob, is there a count down timer or can we do some trickery like set our
> clock ahead 25 hours after the download completes?
There is no way to do that that I know of and if there were it would negate the manual test since it wouldn't be the same as waiting 24 hours.

Besides the automated tests already in place I have verified the notification worked properly using a 1 hour notification and I am waiting on the 24 hour period to complete. I am quite certain everything is fine and the main reason I asked for the 24 hour period is that if we are going to perform a manual test it really should test the actual settings used by the client which we have not been good at in the past.
Comment 38 Robert Strong [:rstrong] (use needinfo to contact me) 2011-11-05 22:52:16 PDT
I've verified that the ui is displayed properly after 24 hours as well.

Note You need to log in before you can comment on or make changes to this bug.