1042463 - "System Update Ready" prompt keeps coming back and remind me battery is not 50% charged

Status: RESOLVED FIXED

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

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

STR:

1. Make sure battery level is below 50% (fig will be changed after bug 1034735 lands)
2. Manually check for update
3. Pull down the utility tray, tap, and confirm to download update
4. The phone will now tell you "System Update Ready" "You need at 50% battery level to install the update." -- this is normal
5. Tap OK to dismiss the button and start doing other stuff, e.g. Facebook, e-mail.

Expected:

1. That dialog should not interrupt me until the battery is 50% charged, or it should never interrupt me (this need UX clarification -- needinfo)

Actual:

1. The dialog on step 4 show up again, and again after a few minute, and again ....

[Blocking Requested]: Extremely annoy to user especially for people on the Nightly OTA.

Comment 1

[Stephany Wilkes]

Flagging Jenny. I agree that this is very irritating. Thanks Tim!

Comment 2

[Jenny Lee]

The battery level warning dialog should only appear when user tries to install update at battery level <25% (charging or not) by tapping the "system update ready" notification. It is not supposed to show up if not triggered by user. Tks!

Comment 3

[Gregor Wagner [:gwagner]]

I don't think we ever worked on the relevant code here. Tim, does your team cover this area?

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

That's not ping people responsible of taking care of the same script forever although I could identify Etienne was the original author.

$ g count -- apps/system/js/update_manager.js
    15  Etienne Segonzac
     5  Julien Wajsberg
     4  crh0716
     3  Kevin Grandon
     3  Michal Budzynski
     2  Anthony Ricaud
     2  Fabien Cazenave
     2  Staś Małolepszy
     1  Greg Weng
     1  Francesco Lodolo (:flod)
     1  Ryan VanderMeulen
     1  hanj.kim
     1  Timothy Guan-tin Chien
     1  Zbigniew Braniecki
     1  Arthur Chen
     1  German Toro del Valle

$ g count -- apps/system/js/updatable.js
     9  Etienne Segonzac
     5  Julien Wajsberg
     3  Marshall Culpepper
     3  Alive Kuo
     1  Francesco Lodolo (:flod)
     1  KWierso
     1  Margaret Leibovic
     1  Michal Budzynski
     1  Pavel Ivanov
     1  Alexandre Lissy
     1  crh0716
     1  Alexandre Poirot
     1  Anthony Ricaud
     1  Fabien Cazenave

===

Besides digging into git log, After reading the actual code, I found is that the dialog might be triggered by Gecko, since there is no setTimeout() anywhere here.

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/updatable.js#L199-L206

and it looks like the timer comes from here, which Gecko try to ask user to apply the downloaded update whenever the phone is idle.

http://dxr.mozilla.org/mozilla-central/source/b2g/components/UpdatePrompt.js

Comment 5

[Gregor Wagner [:gwagner]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #4)
> That's not ping people responsible of taking care of the same script forever
> although I could identify Etienne was the original author.
> 

It works for me on 2.0 so I assumed this is a regression and for regression I usually try to ping the owners first. Maybe the interns want to help out here :)

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

If this is a regression, we should find it. Although I admit the STR is painful to do repetitively, so not setting regressionwindow-wanted. 

Maybe ping the authors of bugs listed here?

http://hg.mozilla.org/mozilla-central/filelog/a5a720259d79/b2g/components/UpdatePrompt.js

Comment 7

[:gerard-majax]

This is definitively something that I've saw on a Flame dogfooding 2.0, but the STR were not consistent :(

Comment 8

[:gerard-majax]

According to http://dxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#552 it means you needs to be idle for 600 secs to get the prompt.

The name of the pref is b2g.update.apply-idle-timeout, and the value is in ms. Looking at Gecko, the detection of idling is done in Services.idle (http://dxr.mozilla.org/mozilla-central/source/b2g/components/UpdatePrompt.js#174). Could it be possible that we are facing a regression there?

Tim, do you think you can check your STR regarding those 600 secs of idle? Since you make reference to "a few minutes".

Comment 9

[James Hobin [:hobinjk]]

This bug appears because the "battery below threshold" flow is indistinguishable from the "battery okay, but user requested the update to be later" flow. Both send an 'update-prompt-apply-result' event with detail 'wait', which triggers Gecko-land's waitForIdle method. The easiest fix is changing the detail to disambiguate the waiting and insufficient battery conditions.

Comment 10

[James Hobin [:hobinjk]]

Attachment: system-update-ready.patch

This is the patch for the Gecko side of this issue. This modification is not strictly necessary, but keeps the handling of 'battery-nok' explicit.

Comment 11

[James Hobin [:hobinjk]]

Attachment: Gaia PR

This is the Gaia side of the fix where all of the interesting work occurs.

Comment 12

[Gregor Wagner [:gwagner]]

Is this ready for review?

Comment 13

[James Hobin [:hobinjk]]

(In reply to Gregor Wagner [:gwagner] from comment #12)
> Is this ready for review?

Yes, I believe this is ready for review. Flagging you as the reviewer for now, feel free to forward to the proper person or people.

Comment 14

[Etienne Segonzac (:etienne)]

Comment on attachment 8467257 [details] [review]
Gaia PR

r=me with:
* renaming the "battery-nok" reason to "low-battery", sorry for the gecko/gaia churn
* adding a small gaia-unit test similar to this one [1] to make sure we're actually sending the correct reason in the low battery case

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/unit/updatable_test.js#L708

Comment 15

[jhobin]

(In reply to Etienne Segonzac (:etienne) from comment #14)
> Comment on attachment 8467257 [details] [review]
> Gaia PR
> 
> r=me with:
> * renaming the "battery-nok" reason to "low-battery", sorry for the
> gecko/gaia churn
> * adding a small gaia-unit test similar to this one [1] to make sure we're
> actually sending the correct reason in the low battery case
> 
> [1]
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/unit/
> updatable_test.js#L708

The pull request has been updated with unit test confirming that updatable uses the declineInstallBattery callback with the "low-battery" value.

Comment 16

[jhobin]

Attachment: system-update-ready.patch

Changes Gecko to also use "low-battery"

Obligatory video: http://youtu.be/etEkugzg8zA?t=1m47s

Comment 17

[jhobin]

Try results for Gecko-side patch: https://tbpl.mozilla.org/?tree=Try&rev=031c9a16e548

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/2c2abe6f3ae9

Master: https://github.com/mozilla-b2g/gaia/commit/0368ef53e524dfac5c6d1a12a2e5938e7785ff60

Please make sure your future patches contain proper hg commit information, including author and a useful commit message. Thanks.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2c2abe6f3ae9

Comment 20

[Kevin Grandon :kgrandon]

I'm going to mark this as testsuite+ for now. The amount of mocking we'd have to do in an integration test for this issue would make this no more useful than a unit test.

Comment 21

[Lance（leave from Mozilla）]

Attachment: verify_video.MP4

Hi reporter,
Could you help to check if this is a correct result of my video? After I confirm to download update with battery is 18%, it will be downloaded successfully and directly.
See attachment:verify_video.MP4

Flame 2.1 build:
Gaia-Rev        77c57eb8a985d5cbd34a597fb1b978ba6e205af6
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/f05d0a2d2378
Build-ID        20150120001202
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150120.035022
FW-Date         Tue Jan 20 03:50:33 EST 2015
Bootloader      L1TC000118D0

Comment 22

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Lance,

Sorry for the delay. The video is not the correct duplication of my STR. My STR specifically talk about System update, not app update. Thanks.