819548 - [OTA Update] No progress UI when update is resuming the update

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System, defect, P1)

Description

[Tony Chung [:tchung]]

Attachment: screenshot 1

My update froze, so i restarted my device.  fortunately, i can tell that the update is resuming via logcat.

however, there's no indication in the UI that update is resuming.  instead, you get a "Checking for updates.." logo, and nothing in notifications appear.

Logcat of resuming download:
12-07 10:54:41.400: E/GeckoConsole(108): AUS:SVC Downloader:onProgress - progress: 9500702/48119660
12-07 10:54:41.981: I/Gecko(108): *** AUS:SVC Downloader:onProgress - progress: 9511694/48119660
12-07 10:54:41.981: E/GeckoConsole(108): AUS:SVC Downloader:onProgress - progress: 9511694/48119660
12-07 10:54:42.581: I/Gecko(108): *** AUS:SVC Downloader:onProgress - progress: 9529556/48119660
12-07 10:54:42.581: E/GeckoConsole(108): AUS:SVC Downloader:onProgress - progress: 9529556/48119660
12-07 10:54:43.212: I/Gecko(108): *** AUS:SVC Downloader:onProgress - progress: 9550166/48119660
12-07 10:54:43.212: E/GeckoConsole(108): AUS:SVC Downloader:onProgress - progress: 9550166/48119660
12-07 10:54:50.529: I/Gecko(108): *** AUS:SVC Downloader:onProgress - progress: 9561158/48119660
12-07 10:54:50.529: E/GeckoConsole(108): AUS:SVC Downloader:onProgress - progress: 9561158/48119660
12-07 10:55:25.653: I/Gecko(108): *** AUS:SVC Downloader:onProgress - progress: 9562532/48119660
12-07 10:55:25.653: E/GeckoConsole(108): AUS:SVC Downloader:onProgress - progress: 9562532/48119660
12-07 10:55:26.294: I/Gecko(108): *** AUS:SVC Downloader:onProgress - progress: 9563906/48119660
12-07 10:55:26.294: E/GeckoConsole(108): AUS:SVC Downloader:onProgress - progress: 9563906/48119660


See screenshots, no progress UI.

REpro:
1) install 12-3 beta build 
2) check for updates , and find a newer build available.
3) apply the update
4) if the update stalls, reboot the phone
5) after reconnecting, verify the update is still resuming (via logcat), but nothing on the UI indicates it so.


Expected:
- UI shows update resumes

Actual:
- update is still in progress (via logcat) ,but UI says checking... and the notification is empty.

Comment 1

[Tony Chung [:tchung]]

Attachment: screenshot 2

Comment 2

[Joe Cheng [:jcheng] (please needinfo)]

BB+, P3 - OTA is an imporant feature. It needs to be stable.

Comment 3

[Marshall Culpepper [:marshall_law]]

This sounds eerily similar to Bug 821523. Tony can you check that out and mark this as a duplicate if it matches your testcase?

Comment 4

[Etienne Segonzac (:etienne)]

(In reply to Marshall Culpepper [:marshall_law] from comment #3)
> This sounds eerily similar to Bug 821523. Tony can you check that out and
> mark this as a duplicate if it matches your testcase?

Did some cleanup.

Bug 821523 refers to a piece of UI in the Settings app (updated by gecko.updateStatus mozSetting), this bug refers to a piece of UI in the System app (the fake notification in the tray updated through mozChromeEvents).

Comment 5

[Josh Carpenter [:jcarpenter]]

Per the thread with Julie in bug 821523, I agree that if the device restarts we _must_ prompt the user to resume, and never resume, given the cost of bandwidth in the target markets.

Comment 6

[Josh Carpenter [:jcarpenter]]

Per the thread with Julie in bug 821523, I agree that if the device restarts we must prompt the user to manually resume the interrupted update. Automatically resuming is never acceptable, owing to target market costs of bandwidth.

Comment 7

[Julien Wajsberg [:julienw]]

So in Bug 821523 comment 6 I said that we need some platform work, and the Gaia part will follow without a change.

Agreed ?

Comment 8

[Julien Wajsberg [:julienw]]

I'll try do produce a first patch.

Comment 9

[Julien Wajsberg [:julienw]]

Attachment: patch v1

First try.

The only thing I'm not ok with this is that I think we have to force an update to get the notification, or wait for whatever delay is configured in the settings.

Any idea on how a notification could be shown as soon as gaia starts up ?

Comment 10

[Julien Wajsberg [:julienw]]

Comment on attachment 694925 [details] [diff] [review]
patch v1

Ok, I have some idea and I'll try them later today (I'm traveling today, will work on this after). So removing the review request.

Comment 11

[Julien Wajsberg [:julienw]]

Attachment: patch v2

requesting review from Dave as he's working on this stuff for Bug 785124.

After trying various things and with limited success, I decided to keep it simple, so it's my first patch except I added the function names in the LOG.

I think that we should have a patch in Gaia so that Gaia's Update Manager would know that an update (either system or app) was not finished, and would check for an update asap after the startup.

Comment 12

[Julien Wajsberg [:julienw]]

What I tried instead of only setting um.activeUpdate to null:
* in UpdateService, call |this._displayPrompt|
* in B2G's UpdatePrompt, in the method "showUpdateAvailable", instead of calling "downloadUpdate" when sending an event fails (which it does at startup), I wanted to set the setting "gaia.system.checkForUpdates" to true, so that it forces an update check as soon as gaia is ready.

I stopped here because I realized that Gecko doesn't use SettingsManager for now, and I thought that Gaia could manage to keep the information that the update hasn't finished.

Comment 13

[Dave Hylands [:dhylands]]

Comment on attachment 695326 [details] [diff] [review]
patch v2

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

I'm not sure that I'm the best person to do this review, as I'm not fully aware of of the consequeces of even this simple change.

::: toolkit/mozapps/update/nsUpdateService.js
@@ +1616,3 @@
>      if (status == STATE_DOWNLOADING) {
>        LOG("UpdateService:_postUpdateProcessing - patch found in downloading " +
>            "state");

Patches should have 8 lines of context (so use the -U8 option if you're using git to create the patch).

@@ +1616,5 @@
>      if (status == STATE_DOWNLOADING) {
>        LOG("UpdateService:_postUpdateProcessing - patch found in downloading " +
>            "state");
> +#ifdef MOZ_WIDGET_GONK
> +      LOG("UpdateService:_postUpdateProcessing - We're in Gonk, returning.");

To me, the really important thing that you haven't mentioned is that we need to wait for user input before resuming the download.

I would fix the LOG statement and the comment below. It's not that b2g is not ready yet, it's that we want explicit user interaction before continuing.

@@ +1619,5 @@
> +#ifdef MOZ_WIDGET_GONK
> +      LOG("UpdateService:_postUpdateProcessing - We're in Gonk, returning.");
> +      // we can't call _selectAndInstallUpdate yet because b2g is not ready yet
> +      // and that would trigger the download.
> +      um.activeUpdate = null;

Should we also call cleanupActiveUpdate?

@@ +1620,5 @@
> +      LOG("UpdateService:_postUpdateProcessing - We're in Gonk, returning.");
> +      // we can't call _selectAndInstallUpdate yet because b2g is not ready yet
> +      // and that would trigger the download.
> +      um.activeUpdate = null;
> +      return;

Since you're doing a return here, doesn't that imply that the rest of the code (between the #endif and the end of the  if statement) will never be executed?

Presumably, then you should remove this return, #else the rest of the if statement and use the return at the end of the #if statement to make it a little more obvious what's going on.

Comment 14

[Julien Wajsberg [:julienw]]

(In reply to Dave Hylands [:dhylands] from comment #13)
> Comment on attachment 695326 [details] [diff] [review]
> patch v2
> 
> Review of attachment 695326 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure that I'm the best person to do this review, as I'm not fully
> aware of of the consequeces of even this simple change.

Who do you think would be a good person ? :marshall ?

> ::: toolkit/mozapps/update/nsUpdateService.js
> @@ +1616,3 @@
> >      if (status == STATE_DOWNLOADING) {
> >        LOG("UpdateService:_postUpdateProcessing - patch found in downloading " +
> >            "state");
> 
> Patches should have 8 lines of context (so use the -U8 option if you're
> using git to create the patch).

Ok. I used |hg diff| but I will find a way :)

> 
> @@ +1616,5 @@
> >      if (status == STATE_DOWNLOADING) {
> >        LOG("UpdateService:_postUpdateProcessing - patch found in downloading " +
> >            "state");
> > +#ifdef MOZ_WIDGET_GONK
> > +      LOG("UpdateService:_postUpdateProcessing - We're in Gonk, returning.");
> 
> To me, the really important thing that you haven't mentioned is that we need
> to wait for user input before resuming the download.
> 
> I would fix the LOG statement and the comment below. It's not that b2g is
> not ready yet, it's that we want explicit user interaction before continuing.

Ok.

> @@ +1619,5 @@
> > +#ifdef MOZ_WIDGET_GONK
> > +      LOG("UpdateService:_postUpdateProcessing - We're in Gonk, returning.");
> > +      // we can't call _selectAndInstallUpdate yet because b2g is not ready yet
> > +      // and that would trigger the download.
> > +      um.activeUpdate = null;
> 
> Should we also call cleanupActiveUpdate?

Nope because that would prevent us from resuming the download.
Calling cleanupActiveUpdate works but it makes the download restarting from the start.

> @@ +1620,5 @@
> > +      LOG("UpdateService:_postUpdateProcessing - We're in Gonk, returning.");
> > +      // we can't call _selectAndInstallUpdate yet because b2g is not ready yet
> > +      // and that would trigger the download.
> > +      um.activeUpdate = null;
> > +      return;
> 
> Since you're doing a return here, doesn't that imply that the rest of the
> code (between the #endif and the end of the  if statement) will never be
> executed?
> 
> Presumably, then you should remove this return, #else the rest of the if
> statement and use the return at the end of the #if statement to make it a
> little more obvious what's going on.

Good idea, thanks.

Comment 15

[Dave Hylands [:dhylands]]

(In reply to Julien Wajsberg [:julienw] from comment #14)
> (In reply to Dave Hylands [:dhylands] from comment #13)
> > Comment on attachment 695326 [details] [diff] [review]
> > patch v2
> > 
> > Review of attachment 695326 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I'm not sure that I'm the best person to do this review, as I'm not fully
> > aware of of the consequeces of even this simple change.
> 
> Who do you think would be a good person ? :marshall ?

marshall would be a good place to start.

If push comes to shove, I can probably do it since it appears to be a B2G only patch.

> > ::: toolkit/mozapps/update/nsUpdateService.js
> > @@ +1616,3 @@
> > >      if (status == STATE_DOWNLOADING) {
> > >        LOG("UpdateService:_postUpdateProcessing - patch found in downloading " +
> > >            "state");
> > 
> > Patches should have 8 lines of context (so use the -U8 option if you're
> > using git to create the patch).
> 
> Ok. I used |hg diff| but I will find a way :)

See: https://developer.mozilla.org/en-US/docs/Installing_Mercurial#Configuration

The unified = 8 line in the [diff] section will tell diff to provide 8 lines of context.

Comment 16

[Julien Wajsberg [:julienw]]

Attachment: patch v3

Addressed comments from :dhyllands.

Thanks !

Comment 18

[Marshall Culpepper [:marshall_law]]

(In reply to Julien Wajsberg [:julienw] from comment #12)
> * in B2G's UpdatePrompt, in the method "showUpdateAvailable", instead of
> calling "downloadUpdate" when sending an event fails (which it does at
> startup), I wanted to set the setting "gaia.system.checkForUpdates" to true,
> so that it forces an update check as soon as gaia is ready.
> 
> I stopped here because I realized that Gecko doesn't use SettingsManager for
> now, and I thought that Gaia could manage to keep the information that the
> update hasn't finished.

We don't use the settings manager, but we do use the settings service for "gecko.updateStatus". See here:
https://mxr.mozilla.org/mozilla-central/source/b2g/components/UpdatePrompt.js#176

Comment 19

[Marshall Culpepper [:marshall_law]]

Comment on attachment 695779 [details] [diff] [review]
patch v3

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

This code probably works for stopping the automatic resume, but how is Gaia going to manage to prompt to resume the download? Do you have a gaia patch in progress for that? r- for now until we can make sure the path from Gecko -> Gaia will prompt correctly for download resuming.

On a side note: I can see this prompt-for-every-resume being incredibly annoying for a spotty network connection. Do we have any kind of plan to allow these resumes to be auto-accepted?

::: toolkit/mozapps/update/nsUpdateService.js
@@ +1622,5 @@
> +          "without doing anything.");
> +      // we can't call _selectAndInstallUpdate yet because b2g is not ready yet
> +      // and that would trigger the download whereas we want the user consent
> +      // before downloading anything
> +      um.activeUpdate = null;

One way we might let Gaia know of the update resume is to reuse nsIUpdatePrompt.showUpdateAvailable here..

Comment 20

[Julien Wajsberg [:julienw]]

(In reply to Marshall Culpepper [:marshall_law] from comment #19)
> Comment on attachment 695779 [details] [diff] [review]
> patch v3
> 
> Review of attachment 695779 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This code probably works for stopping the automatic resume, but how is Gaia
> going to manage to prompt to resume the download? Do you have a gaia patch
> in progress for that? r- for now until we can make sure the path from Gecko
> -> Gaia will prompt correctly for download resuming.

This actually works when the user checks for update again, or at the next automatic check (that is daily by default).

I assumed it was enough for v1.

I can work on something Gaia-side if you think that's necessary. I could try to hack something around the settings service but I'd prefer to do this right and keep this in Gaia.

> On a side note: I can see this prompt-for-every-resume being incredibly
> annoying for a spotty network connection. Do we have any kind of plan to
> allow these resumes to be auto-accepted?

I agree but I guess this is not for v1...


> ::: toolkit/mozapps/update/nsUpdateService.js
> @@ +1622,5 @@
> > +          "without doing anything.");
> > +      // we can't call _selectAndInstallUpdate yet because b2g is not ready yet
> > +      // and that would trigger the download whereas we want the user consent
> > +      // before downloading anything
> > +      um.activeUpdate = null;
> 
> One way we might let Gaia know of the update resume is to reuse
> nsIUpdatePrompt.showUpdateAvailable here..

No we can't because Gaia is not ready yet, and then won't get the event, and |sendUpdateEvent| would return false, and the download would be triggered. That's actually what's happening in the current code and what we want to avoid.

Comment 21

[Julien Wajsberg [:julienw]]

Marking blocking bug 813770 which basically does what I was doing here.

Therefore in this patch I'll now concentrate on getting an UI ASAP.

After a night of thinking ;) I think using the settings service is not so hacky and I'll try something around this today.

Comment 22

[Dave Hylands [:dhylands]]

Yeah - I think that if we set the setting thhat "Force Update Now" sets once we detect an interrupted download, then that would cause the update available notification to occur fairly soon after booting.

Comment 23

[Julien Wajsberg [:julienw]]

Attachment: patch v4

New proposition. This should be applied after fix for Bug 813770. Sorry for the space changes, my vim configuration changes this automatically and I don't know how to remove these from the diff with |hg export|.

I tried the cases of rebooting during download and during uncompressing. By rebooting, I mean pulling the battery, in order to fail in a dirty way.

I'm still concerned about the last following case :
- the user gets the system update notification in Gaia, but he doesn't apply it and reboots the phone
-> the notification is not shown unless he force checks or wait for whatever delay is configured (default is 1 day)

I don't know how this could be handled in a cleanly way, maybe this should be handled in another bug.

Comment 24

[Dave Hylands [:dhylands]]

Comment on attachment 697993 [details] [diff] [review]
patch v4

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

Yeah - that should work.

Comment 25

[Dave Hylands [:dhylands]]

> Created attachment 697993 [details] [diff] [review]
> patch v4
> 
> New proposition. This should be applied after fix for Bug 813770. Sorry for
> the space changes, my vim configuration changes this automatically and I
> don't know how to remove these from the diff with |hg export|.
> 
> I tried the cases of rebooting during download and during uncompressing. By
> rebooting, I mean pulling the battery, in order to fail in a dirty way.
> 
> I'm still concerned about the last following case :
> - the user gets the system update notification in Gaia, but he doesn't apply
> it and reboots the phone
> -> the notification is not shown unless he force checks or wait for whatever
> delay is configured (default is 1 day)

We could have the code which posts an update notification set a setting, which gets cleared once the user starts the download.

Then the startup code could check that settings and set the same forceCheck flag (in the same place we check the interrupted download).

If the phone doesn't reboot, then the extra setting doesn't do anything, but if it reboots, then its the same thing as the user choosing checkForUpdates if they've been notitifed of an update.

Comment 26

[Julien Wajsberg [:julienw]]

Yep this should work, and I'd say that this could even not be b2g-specific.

Let's open a new bug for this though.

Comment 27

[Julien Wajsberg [:julienw]]

Filed Bug 827090

Comment 28

[Julien Wajsberg [:julienw]]

Comment on attachment 697993 [details] [diff] [review]
patch v4

we don't want to do this because we want to keep this worst case scenario handling.

Comment 29

[Julien Wajsberg [:julienw]]

spoke with Marshall and we'll handle this in gaia instead.

Comment 30

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Is there still something specific we want QA to do, re: qawanted added at 12/20?

Comment 31

[Julien Wajsberg [:julienw]]

nope thanks (and sorry)

Comment 32

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

No problem, just wanted to make sure we weren't leaving something on the floor. Thanks for removing!

Comment 33

[Julien Wajsberg [:julienw]]

New thinking :
We'll set a bool value (maybe with asyncStorage if this happens to be already in the system app) when we first get the system update notification, we'll remove it when we apply the update notification, and at init we'll automatically trigger a "check for update" if we happen to have this bool value set to true.

This would fix Bug 827090 in the process too.

I think michal will try to work on this today.

Comment 34

[Anthony Ricaud (:rik)]

Taking this as I'm starving for bugs. I might pair with Michal.

Comment 35

[Anthony Ricaud (:rik)]

Clarifying a bit the STR for this bug:
1) Flash with a "user" build (VARIANT=user in .userconfig)
2) check for updates , and find a newer build available.
3) click in notifications and apply the update
4) When rebooting, the update will continue to download
5) after reconnecting, verify the update is still resuming (via logcat), but nothing on the UI indicates it so.

If on step 4, you cleanly reboot via the UI, you get bug 827896 which is different.

Comment 36

[Julien Wajsberg [:julienw]]

bug 827896 is different but could be fixed here ;)

Comment 37

[Anthony Ricaud (:rik)]

Attachment: Review pointer to https://github.com/mozilla-b2g/gaia/pull/7504

Comment 38

[Anthony Ricaud (:rik)]

Forgot to mention that we paired with Michal to fix this.

Comment 39

[Julien Wajsberg [:julienw]]

Comment on attachment 700446 [details] [diff] [review]
Review pointer to https://github.com/mozilla-b2g/gaia/pull/7504

comments on the pull request

Comment 40

[Anthony Ricaud (:rik)]

Comment on attachment 700446 [details] [diff] [review]
Review pointer to https://github.com/mozilla-b2g/gaia/pull/7504

We adressed most of the comments of the first review. Asking Etienne for a new review.

Comment 41

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

This one is ready to land.

Comment 42

[Etienne Segonzac (:etienne)]

Comment on attachment 700446 [details] [diff] [review]
Review pointer to https://github.com/mozilla-b2g/gaia/pull/7504

Epic follow up Rick :)

Comment 43

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/commit/030bd8c5f9a28ffd21058607ce3d603d4b2166aa

Comment 44

[Lukas Blakk [:lsblakk] use ?needinfo]

Landed on mozilla-b2g18/gaia master prior to the 1/25 branching to mozilla-b2g18_v1_0_0/v1.0.0, updating status-b2g-v1.0.0 to fixed.

Comment 45

[Cody Roesch [:croesch]]

Unagi Build ID: 20130313070202
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/e74dafa6b2d9
Gaia: b34e726147f8e671ad8c538b50900ccfbffcb084
Kernel: Dec 5th

This bug has been verified fixed. There is no longer hidden update activity after an OTA stall and a phone reboot.

The update availability will be checked once the phone is booted and ready and an update will be shown as available in notificaitons for the user to choose what to do.