838823 - restarting a stopped install of an app fails with NO_DOWNLOAD_AVAILABLE

Status: VERIFIED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Julien Wajsberg [:julienw]]

STR:
* go to http://everlong.org/mozilla/selfupdatinghosted/
* start installing the app
* stop the download from the notification screen
* restart the download from the homescreen

Expected:
* the download restarts

Actual:
* the download does not restart, "download stopped" message is displayed

For hosted+appcache apps this happens whether the download was stopped with a cancel or a network error. However for packaged apps that only happens when the download is stopped with a network error.
 
This was partly caused by the fix to Bug 836909 because hosted+appcache apps that get canceled have downloadAvailable set to false whereas packaged apps that get canceled keep their downloadAvailable to true.

When this is not a cancelation but a network error, then the behavior is different because both hosted+appcache and packaged apps get downloadAvailable = false.

Reading the code, I think that hosted+appcache apps also get downloadAvailable = false when there is an error during update, but I didn't try this.

I can work on this but the only thing that I need to know is : should pending apps have "downloadAvailable == true" when they're canceled ? I'd think that it should be the case.

Nominating because this is a big regression.

Comment 1

[Julien Wajsberg [:julienw]]

Interesting places are :
- setError in AppCacheObserver
- "app.downloadAvailable = false;" in cleanup in downloadPackage (this is from fix to bug 832408)

Comment 2

[Julien Wajsberg [:julienw]]

The alternative is not checking for downloadAvailable if installState is pending, but that would bring us a little more spaghetti..

Comment 3

[Julien Wajsberg [:julienw]]

Attachment: patch v1

I removed the reset of downloadAvailable when we have an error as I believe this is incorrect.

I tested all combinations of these cases with this patch: packaged/hosted+appcache, install/update, cancel/error, restart before reboot/restart after reboot

Everything seems fine. I found other corner bugs in the process (Bug 839071 for example) but this is unrelated to this.

Comment 4

[[:fabrice] Fabrice Desré]

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

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

::: dom/apps/src/Webapps.jsm
@@ -2024,5 @@
>        app.downloading = false;
>  
> -      // To prevent repeated prompts, wait for the next checkForUpdates to
> -      // try a new download.
> -      app.downloadAvailable = false;

This was done for good reasons: gaia was constantly prompting for files that were unavailable. So maybe we need a better heuristic, but not just removing that.

Comment 5

[Julien Wajsberg [:julienw]]

Attachment: patch v2

Now I reset downloadAvailable to false for most errors that a download restart could not fix, while keeping it to true when a network error occurs.

I believe we need a follow up in Gaia to remove/disable the restart icon if downloadAvailable is false after an install error (maybe have another icon, like a big red cross, for this case ?).

For appcache errors we don't know what caused the error so I'm never reseting downloadAvailable.

Comment 6

[Julien Wajsberg [:julienw]]

I tried all use cases (install/update, reboot/no reboot, cancel/network error) of packaged apps with this patch and it works for me. For the most serious errors (like an invalid package), that's where we need a follow up in Homescreen. I'm filing a new bug now.

Comment 7

[Julien Wajsberg [:julienw]]

Filed Bug 839491 for the homescreen.

Comment 8

[Alex Keybl [:akeybl]]

Let's back out bug 836909 instead of taking a forward fix here for v1.0.0 and v1.0.1. Please prepare a patch as soon as possible.

Comment 9

[Julien Wajsberg [:julienw]]

Alex, I may be wrong here, but I thoroughly tested all combinations with this patch, and I am very confident we'll be in a right state after this.

If we back out 836909, the user or even an app can trigger a download that would put this app in a weird state. Also if we back out this we really need to fix Bug 837193 then. I think that's either one or the other.

Comment 10

[Jason Smith [:jsmith]]

(In reply to Julien Wajsberg [:julienw] from comment #9)
> Alex, I may be wrong here, but I thoroughly tested all combinations with
> this patch, and I am very confident we'll be in a right state after this.
> 
> If we back out 836909, the user or even an app can trigger a download that
> would put this app in a weird state. Also if we back out this we really need
> to fix Bug 837193 then. I think that's either one or the other.

I think the reasoning for proposing the backout is since we're dealing with way too many regressions resulting in a regression spin on the gecko portion of apps, resulting in fear in what patches we land in what order. Sadly, this comes at the cost that we have too few tests in the tree. 

Fabrice - What do you think?

The only idea I can think of that we consider landing some of these things safely is land with automated tests. Although we need Dale to land his patch. I'll ping Clint get review on it.

Comment 11

[Jason Smith [:jsmith]]

A different option we could do is grab a set of patches you've got in the queue, get a try build request over to rel eng, and I'll take a look at it.

Comment 12

[[:fabrice] Fabrice Desré]

My 2 cents here:
I'm also worried by the amount of "tiny" patches that only address a single issue and usually lead to further regressions. To be honest, I'm not sure that we're making overall progress there, even if each patch looks good in isolation. Having manual vetting from QA on a custom build with a patch set looks like a good alternative while we are waiting for the tests to land. Jason, I can do a build for you if you have time to test.

More broadly, what we're paying there is the rush to a feature freeze in October, when we had to come up with a new API that added lots of complexity and edge-cases.

Comment 13

[Jason Smith [:jsmith]]

Manual vetting from QA with a custom build with a patch set sounds fine to me. Let me know when you've got the build and I'll take a look.

Comment 14

[Julien Wajsberg [:julienw]]

mmm should we remove the tef+ on this bug, to prevent any accidental landing ?

Comment 15

[Jason Smith [:jsmith]]

Let me ask Ryan directly on this one.

Comment 16

[Jason Smith [:jsmith]]

Ryan mentioned just put wontfix on the two branches for now. When we want to land on those branches, change it back to affected and put checkin-needed again when it's safe.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f301d7eac633

Comment 18

[Jason Smith [:jsmith]]

Using some of the tef+ patches in a custom build, I'm still seeing some odd behavior like bug 838337 happening.

I can technically restart a download, however.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Jason Smith [:jsmith]]

Per testing on Fabrice's custom build, this patch seems safe to land on the higher up branches. Marking checkin-needed.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/db4583506bf6
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/f8a5cf33d130

Comment 22

[Jason Smith [:jsmith]]

Verified on 2/11.

Comment 23

[Sue]

Attachment: video of issue verify

This issue has been verified successfully on Flame v2.0 & v2.1
See attachment: verify_video.MP4
Reproducing rate: 0/5
Flame 2.0 versions:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3
Build-ID        20141127000203
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141127.034818
FW-Date         Thu Nov 27 03:48:29 EST 2014
Bootloader      L1TC00011880

Flame 2.1 versions:
Gaia-Rev        5372b675e018b6aac97d95ff5db8d4bd16addb9b
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/f34377ae402b
Build-ID        20141127001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141127.035005
FW-Date         Thu Nov 27 03:50:16 EST 2014
Bootloader      L1TC00011880