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.
Interesting places are : - setError in AppCacheObserver - "app.downloadAvailable = false;" in cleanup in downloadPackage (this is from fix to bug 832408)
The alternative is not checking for downloadAvailable if installState is pending, but that would bring us a little more spaghetti..
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+
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)
Attachment #711356 - Attachment is obsolete: true
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.
Filed Bug 839491 for the homescreen.
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+
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.
(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.
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.
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
status-b2g18: affected → fixed
status-b2g18-v1.0.0: affected → fixed
status-firefox19: --- → wontfix
status-firefox20: --- → wontfix
status-firefox21: --- → fixed
Verified on 2/11.
Status: RESOLVED → VERIFIED
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
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
You need to log in before you can comment on or make changes to this bug.