1096971 - When updating an app, if it fails (network goes down) you need to restart the device in order to use the app

Status: VERIFIED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[mbarone]

Tested on Flame 2.0 and master.

STR
  1. Install an app from a store. The app *must* be privileged and have a specific origin (like app://whatever.com).
  2. Update the app on the store
  3. Check for updates and apply the update.
  4. Before the update is downloaded completely, remove the network from the device (stopping the wifi for example).

  5. At a later point (before it gives the network error), reactivate the network, and cancel the update.
  6. Resume the update

ACTUAL RESULT
The app seems to be installed as expected but when you open the app the message "Required update: There is an update with new features....." is shown. Also the message "There was an error downloading the update" is shown. The user can't use the app until you restart the device; now is when the app works as expected.

EXPECTED RESULT
The app should be updated correctly without restarting the device.

Comment 1

[Antonio Manuel Amaya Calvo (:amac)]

There seem to be several errors here:

1 On one hand, when the app is finally installed, for some reason we're not updating correctly the etag/hash. So when checkForUpdates is called again, a new (bogus) update is offered.

2 Then, when the user tries to download the package, the actual zip is detected as being the same we already have and so we (correctly) send a applied event ([1]). But then in the next line we throw a PACKAGE_UNCHANGED. Which ends sending a downloaderror event at [2]. So we're telling Gaia that the download was a success, and applied, and that it was an error at the same time. Which seems kinda schizophrenic, all things considered.

Now, we could fix this in Gaia by checking if the error was PACKAGE_UNCHANGED, but since we're already saying that the download was a success, I think it would be better to not send the downloaderror when the package is unchanged.

WDTY, Fabrice?

(oh, BTW, this fails also since 2.0 at least).

[1] http://mxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm#3264
[2] http://mxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm#3945

Comment 2

[[:fabrice] Fabrice Desré]

Yep, it looks strange to say "everything work catastrophically"! So yes, we should not send downloaderror in this case.
I'm not sure how we can test that though.

Comment 3

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 1, V1. Don't fire downloaderror when the package wasn't changed on an update

This fixes half of the problem (the easy half): when the package downloaded is the same as the one we had, we won't fire a downloaderror on the content. I also updated the test to fail in case we did.
Test run is at: 
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=41f133ffcd84

Comment 4

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 2, V1. Don't erase the staged attributes if the download is still available

And this fixes the second problem. The problem exists because when a download fails we were deleting the .staged attribute, but the staged-update file still existed (because the download is still available, and it's a transient error supposedly). So when the user retries the download the .staged.etag and .staged.manifestHash are not recalculated, and so even if the update is applied correctly the system still thinks that the remote manifest is different from the local one (and so the update is offered again).

Added a test to check for this error condition also (downloading/applying an update after it failed once).

Comment 5

[Antonio Manuel Amaya Calvo (:amac)]

I forgot, the test run is at: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7418117dcd88

Comment 6

[Maria Angeles Oteo (:oteo)]

[Blocking Requested - why for this release]: Fatal error, we have detected the platform fault with Loop application but it will happen for any application that interrupts his downloading from the Market Place because a network disconnection.

Comment 7

[[:fabrice] Fabrice Desré]

Comment on attachment 8524620 [details] [diff] [review]
Part 2, V1. Don't erase the staged attributes if the download is still available

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

::: dom/apps/tests/file_packaged_app.sjs
@@ +61,5 @@
>      var packageName = "test_packaged_app_" + packageVersion + ".zip";
>  
>      setState("packageName", packageName);
>      var packagePath = "/" + gBasePath + "file_packaged_app.sjs?" +
> +                      (allowCancel?"allowCancel&": "") +

nit: spaces around the operators.

Comment 8

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 2, V2. With nits fixed

r=fabrice

Comment 9

[Antonio Manuel Amaya Calvo (:amac)]

Part 2 touches the same files than part1, and they should be applied in order.

Comment 10

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 8523775 [details] [diff] [review]
Part 1, V1. Don't fire downloaderror when the package wasn't changed on an update

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: When a package update fails for a transient problem (like a network error, network going down, etc) after the network comes up and the package is updated successfully, it'll be unlaunchable until the phone is rebooted. The user doesn't get any indication of this (the UI shown is as if the package wasn't updated successfully)
Testing completed: The patch includes tests, and also manual tests.
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch:none

Comment 11

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 8526898 [details] [diff] [review]
Part 2, V2. With nits fixed

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: When a package update fails for a transient problem (like a network error, network going down, etc) after the network comes up and the package is updated successfully, the package update will *still* be offered (so if the app checks for updates it'll offer it again). This is compounded by the other problem fixed in this patch (the update will be offered even after a reboot).
Testing completed: The patch includes tests, and also manual tests.
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch:none

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/71a0e053308c
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e0063fd3c5e

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/71a0e053308c
https://hg.mozilla.org/mozilla-central/rev/8e0063fd3c5e

Comment 14

[Wesly Huang (EPM)]

[Triage] fixed in m/c already per comment#13, not blocking 2.0 per the abnormal use case and current 2.0 phase.

Comment 15

[bhavana bajaj [:bajaj]]

oteo/:amac, is this 2.0 specific regression or has this been always busted int he past releases as well?

Comment 16

[Antonio Manuel Amaya Calvo (:amac)]

This code was rewritten completely for 2.0. The actual code that I changed didn't exist in 1.4 (it would launch an uncatched exception which would be logged, but that would be transparent for Gaia so for the user there would not be any problems).

Comment 17

[Maria Angeles Oteo (:oteo)]

[Blocking Requested - why for this release]:

Clearing my needinfo, as Antonio has already confirmed that this is a regression and in 1.4 this bug was not happening

Comment 18

[KTucker [:KTucker][Inactive 3/4/2016]]

Yeojin, can you verify that this issue has been fixed?

Comment 19

[mbarone]

Tested with Flame
master (KK 188 based)
Gecko-6d3a321
Gaia-689bdc8

It works well. If the user lost the internet connection and then resume the connection, the update is updated as expected and you don't have to restart the device. When the device finish to update the app, the message "There was an error downloading the update" is displayed. I understand that this is the expected behavior.

Comment 20

[Yeojin Chung [:YeojinC]]

Marking as verified fixed based on Comment 19.

Comment 21

[Wesly Huang (EPM)]

[Triage] per comment#16 we may "treat" this one as a regression however not reported from partner, also fixed already in mc/, thus de-nom..

Comment 22

[Beatriz Rodríguez [:brg]]

[Blocking Requested - why for this release]: this is a blocker and reported by a partner. please approve the uplift to the branch we are about to commercialize.

Comment 23

[Wesly Huang (EPM)]

[Triage]

Thanks Beatriz, I see, so at least we need this in 2.0 as well.

Also per comment#1 & comment#17 update flags, and tag for 2.0.

qawanted for 2.1 and 2.0M.

Comment 24

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: B2G 32 version, folded

This is the B2G32 version of the patch (without backporting the tests, and folded into a single patch).

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Was there a b2g34 approval request coming for this too?

Comment 26

[Antonio Manuel Amaya Calvo (:amac)]

It should be applied to b2g34 also, indeed. I believe the m-c one should apply. Let me know if it doesn't so I can rebase it. Thanks!

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Re-read my comment. You need to *request approval* for each branch.

Comment 28

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 8523775 [details] [diff] [review]
Part 1, V1. Don't fire downloaderror when the package wasn't changed on an update



[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: When a package update fails for a transient problem (like a network error, network going down, etc) after the network comes up and the package is updated successfully, it'll be unlaunchable until the phone is rebooted. The user doesn't get any indication of this (the UI shown is as if the package wasn't updated successfully)
Testing completed: The patch includes tests, and also manual tests.
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch:none

Comment 29

[Antonio Manuel Amaya Calvo (:amac)]

There you go ;)

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/3148f1ae8a27
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/d92a441f3e88

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/a8ebdfd96f95

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/a8ebdfd96f95