1092152 - When updating an app, if it fails (network goes down) sometimes the app gets into a un-updatable state

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Antonio Manuel Amaya Calvo (:amac)]

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

Expected:
  The update is applied correctly.

Actual: 
  Sometimes (It might depend on the exact moment when the download is stopped) the update cannot be applied. If the app is uninstalled, it cannot be installed again.

The uninstallable state can be fixed by:

  1. Uninstalling the app
  2. Executing 
adb shell rm -r /data/local/tmp/webapps

Comment 1

[Antonio Manuel Amaya Calvo (:amac)]

[Blocking Requested - why for this release]:
This is failing on 2.0, and it's a problem that can/will happen for users and that for most of them leave the device on a state where some apps cannot be used anymore.

Comment 2

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Patch V1. Don't calculate the downloaded file hash if we didn't download it

Comment 3

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 8515111 [details] [diff] [review]
Patch V1. Don't calculate the downloaded file hash if we didn't download it

Now this was an interesting bug... I don't know how or why, sometimes when we're downloading a update if I remove the network I don't get a NETWORK_ERROR. Instead the file appears to be still downloading, but stuck at 0 bytes. If I cancel the download then (can't do anything else really) it seems we have updated the ETAG but nothing else (maybe the package was downloaded but not applied?). 

So when I try to resume the download, the server answers with a 304 (cause the etag is the same). And so we don't download anything but try to hash the (not downloaded) file. Which fails and gives and exception. And at that point, you can kiss that application goodbye forever until you do a hard reset (or do some file-level modifications).

In any case, the fix is simple, just don't try hashing the file if we got a 304. I've tested this and this allows me to use/and update again an app that was borked.

Test run is at: 
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bf57bc219eb7

Comment 4

[Antonio Manuel Amaya Calvo (:amac)]

Oh, I also have the b2g32 patch ready (and tested). Will upload once this one is reviewed.

Comment 5

[Wayne Chang [:wchang]]

This should block as it presents a very broken experience to end users.

Antonio, can you please request appropriate uplift approvals for your patches?

Thanks

Comment 6

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: *Incorrect* B2G 32 patch, this is the M-C patch

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: Some times apps will become unlaunchable/unupdatable/uninstallable when there's a network error. That state is non transient and requires a hard reset to get out of.
Testing completed: Locally
Risk to taking this patch (and alternatives if risky): Very low, it just alters the order of a couple of validations
String or UUID changes made by this patch: None

r=fabrice
This is the equivalent patch for B2G32 (the Webapps.jsm was moved and it's missing one line that's on Mozilla Central).

Comment 7

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 8515111 [details] [diff] [review]
Patch V1. Don't calculate the downloaded file hash if we didn't download it

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 #): 

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Some times apps will become unlaunchable/unupdatable/uninstallable when there's a network error. That state is non transient and requires a hard reset to get out of.
Testing completed: Locally
Risk to taking this patch (and alternatives if risky): Very low, it just alters the order of a couple of validations
String or UUID changes made by this patch: None

Comment 8

[Carsten Book [:Tomcat]]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #3)
> Comment on attachment 8515111 [details] [diff] [review]
> Patch V1. Don't calculate the downloaded file hash if we didn't download it
> 
> Now this was an interesting bug... I don't know how or why, sometimes when
> we're downloading a update if I remove the network I don't get a
> NETWORK_ERROR. Instead the file appears to be still downloading, but stuck
> at 0 bytes. If I cancel the download then (can't do anything else really) it
> seems we have updated the ETAG but nothing else (maybe the package was
> downloaded but not applied?). 
> 
> So when I try to resume the download, the server answers with a 304 (cause
> the etag is the same). And so we don't download anything but try to hash the
> (not downloaded) file. Which fails and gives and exception. And at that
> point, you can kiss that application goodbye forever until you do a hard
> reset (or do some file-level modifications).
> 
> In any case, the fix is simple, just don't try hashing the file if we got a
> 304. I've tested this and this allows me to use/and update again an app that
> was borked.
> 
> Test run is at: 
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bf57bc219eb7

Hi Anthonia, this test run shows a lot of test failures like in the web-platform tests. Is this fixed/related to this change ?

Comment 9

[Antonio Manuel Amaya Calvo (:amac)]

I saw that but don't think it's related with the patch. The error seems to be related with launching the tests, and the only change this patch has is changing the order of a couple of checks when installing apps. Oh, and it's Antonio ;)

Comment 10

[Antonio Manuel Amaya Calvo (:amac)]

Re-requesting checkin. I didn't notice it was erased. As I said, the errors don't seem related at all with the patch changes.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #9)
> I saw that but don't think it's related with the patch. The error seems to
> be related with launching the tests, and the only change this patch has is
> changing the order of a couple of checks when installing apps. Oh, and it's
> Antonio ;)

I assume you pushed on top of b2g34? That'd be an expected result then :)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a24fb43555a8

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/1b076c5a760c

Your b2g32 patch doesn't apply...

Comment 15

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: B2G32 patch. The correct one this time...

r=fabrice

This is the correct B2G32 patch... I uploaded the same patch twice the other morning.

Dunno if I should ask for approval again, not obsoleting the other one still because of that.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8516482 [details] [diff] [review]
*Incorrect* B2G 32 patch, this is the M-C patch

Marking a patch obsolete doesn't clear the flag. But it does make the attachments easier to follow.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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