restarting a stopped install of an app fails with NO_DOWNLOAD_AVAILABLE

VERIFIED FIXED in Firefox 21, Firefox OS v2.0

Status

Core Graveyard
DOM: Apps
--
major
VERIFIED FIXED
5 years ago
4 months ago

People

(Reporter: julienw, Assigned: julienw)

Tracking

Trunk
mozilla21
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:tef+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 fixed, b2g18-v1.0.1 fixed, b2g-v2.0 verified, b2g-v2.1 verified)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Interesting places are :
- setError in AppCacheObserver
- "app.downloadAvailable = false;" in cleanup in downloadPackage (this is from fix to bug 832408)
Flags: needinfo?(fabrice)
(Assignee)

Updated

5 years ago
Blocks: 838561
(Assignee)

Comment 2

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

Updated

5 years ago
Blocks: 802574
(Assignee)

Updated

5 years ago
Blocks: 838337
(Assignee)

Updated

5 years ago
Assignee: nobody → felash
(Assignee)

Comment 3

5 years ago
Created attachment 711356 [details] [diff] [review]
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.
Attachment #711356 - Flags: review?(fabrice)
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.
Attachment #711356 - Flags: review?(fabrice) → feedback+
Flags: needinfo?(fabrice)
(Assignee)

Comment 5

5 years ago
Created attachment 711831 [details] [diff] [review]
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.
Attachment #711831 - Flags: review?(fabrice)
(Assignee)

Updated

5 years ago
Attachment #711356 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
Filed Bug 839491 for the homescreen.

Comment 8

5 years ago
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.
blocking-b2g: tef? → tef+
(Assignee)

Comment 9

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 839491
(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.
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.
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.
Attachment #711831 - Flags: review?(fabrice) → review+
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.
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 14

5 years ago
mmm should we remove the tef+ on this bug, to prevent any accidental landing ?
Let me ask Ryan directly on this one.
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.
status-b2g18: --- → wontfix
status-b2g18-v1.0.0: --- → wontfix
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.
https://hg.mozilla.org/mozilla-central/rev/f301d7eac633
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Per testing on Fabrice's custom build, this patch seems safe to land on the higher up branches. Marking checkin-needed.
status-b2g18: wontfix → affected
status-b2g18-v1.0.0: wontfix → affected
Keywords: checkin-needed, verifyme
QA Contact: jsmith
https://hg.mozilla.org/releases/mozilla-b2g18/rev/db4583506bf6
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/f8a5cf33d130
status-b2g18: affected → fixed
status-b2g18-v1.0.0: affected → fixed
status-firefox19: --- → wontfix
status-firefox20: --- → wontfix
status-firefox21: --- → fixed
Keywords: checkin-needed
Verified on 2/11.
Status: RESOLVED → VERIFIED
Keywords: verifyme
status-b2g18-v1.0.1: --- → fixed
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed

Updated

3 years ago
status-b2g-v2.0: fixed → verified
status-b2g-v2.1: fixed → verified

Comment 23

3 years ago
Created attachment 8530094 [details]
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

Updated

4 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.