Build: B2G 18 2/4/2013 Device: Unagi Steps: 1. Start download a large hosted app preloading appcache 2. Cancel the download 3. Select the icon to restart the download 4. While it's downloading, select the icon again Expected: Nothing should happen. Actual: Occasionally I've seen the restart download prompt appear allowing me to restart the download. Obviously this doesn't make sense if the download is already happening.

I don't like the "occasionally" part ;) Did you manage to know how to reproduce this consistently ?

(In reply to Julien Wajsberg [:julienw] from comment #1) > I don't like the "occasionally" part ;) Did you manage to know how to > reproduce this consistently ? This is consistent STR I can get. It just depends on where you click in the app icon.

Uh oh. This is actually worse than I thought after reproducing this twice - if you hit this situation, you can occasionally get stuck downloading the app infinitely without ever finishing the download. You would need to restart the phone to get out of this.

I'll have a look

I reproduced it. I already filed bug 838561 in Gecko for the download function (ie: we should not download again if we're already downloading). There may be another bug somewhere because we should not show the restart dialog anyway if we're downloading. This works for packaged apps so I suspect something is not right for hosted+appcache apps. Homescreen doesn't make a difference at all between these apps so maybe app.downloading is being reset somewhere. Just a wild guess though, I'll have to test that more thoroughly later.

For testers: app from http://everlong.org/mozilla/selfupdatinghosted/ is a good fit.

Unable to test right now because of Bug 838823.

I believe this is a dom:apps issue because app.downloading is false (apply the debug patch to see this). This is not the case for packaged apps though.

For packaged apps, I believe the first progress event correctly set the app.downloading property. If we fix Bug 830463 the first progress event will come as soon as the download is started. For hosted+appcache apps, we never set the app.downloading property.

This needs the patch to bug 838823 to be tested. This just sets app.downloading to true when we get an appcache progress event.

Comment on attachment 711438 [details] [diff] [review] patch v1 [Approval Request Comment] Bug caused by (feature/regressing bug #): none User impact if declined: the user could trigger a restart when he should not. Other not-found-yet bugs may also be fixed by this. Testing completed: yes Risk to taking this patch (and alternatives if risky): none String or UUID changes made by this patch: none this is a nice win for a one-liner

Comment on attachment 711438 [details] [diff] [review] patch v1 great reward for low risk, approving.

Comment on attachment 711438 [details] [diff] [review] patch v1 Renom - in the custom build Fabrice gave me, this works for a hosted app with appcache, but fails with a packaged app doing the same test case.

(In reply to Jason Smith [:jsmith] from comment #16) > Comment on attachment 711438 [details] [diff] [review] > patch v1 > > Renom - in the custom build Fabrice gave me, this works for a hosted app > with appcache, but fails with a packaged app doing the same test case. Actually, Fabrice just mentioned it can't be this patch causing the regression, as it doesn't follow the execution path of a packaged app. Disregard my comment.

(In reply to Jason Smith [:jsmith] from comment #17) > (In reply to Jason Smith [:jsmith] from comment #16) > > Comment on attachment 711438 [details] [diff] [review] > > patch v1 > > > > Renom - in the custom build Fabrice gave me, this works for a hosted app > > with appcache, but fails with a packaged app doing the same test case. > > Actually, Fabrice just mentioned it can't be this patch causing the > regression, as it doesn't follow the execution path of a packaged app. > Disregard my comment. And I just confirmed it. I'm reproducing this on a nightly build of unagi as well. Looks like this bug does reproduce equivalently on packaged apps then...

Filed bug 839715 for the issue I just saw.

(In reply to Jason Smith [:jsmith] from comment #19) > Filed bug 839715 for the issue I just saw. Actually, I can get this bug to reproduce this with the custom build with this patch. So this definitely isn't fixed.

Jason> actually, the "downloading" flag is not set until we get the first progress event. This is Bug 830463 and that's true for both hosted+appcache and packaged apps. Maybe that's what you're seeing here ?

I mean, this will be fixed by the fix I suggested in Bug 830463 but if that's it I don't consider this to be serious enough for v1.

(In reply to Julien Wajsberg [:julienw] from comment #21) > Jason> actually, the "downloading" flag is not set until we get the first > progress event. This is Bug 830463 and that's true for both hosted+appcache > and packaged apps. Maybe that's what you're seeing here ? I'll investigate this on Monday with a central build.

Jason - what were your findings today? We'll come through this list again tomorrow in triage so your input today would be great.

(In reply to Lukas Blakk [:lsblakk] from comment #24) > Jason - what were your findings today? We'll come through this list again > tomorrow in triage so your input today would be great. Just tested this today with a central build - Julien is correct. What I'm apparently seeing is bug 830463. If you try to click the icon before the progress notification appears when downloading has started, you will get the prompt unexpectedly. But once the progress notification has appeared, you'll never be able to get the prompt to come up unexpectedly. So this is safe to uplift and a nice win as well.

(In reply to Julien Wajsberg [:julienw] from comment #22) > I mean, this will be fixed by the fix I suggested in Bug 830463 but if > that's it I don't consider this to be serious enough for v1. Since we just approved the fix in bug 830463 does this patch still need to land? If so, we will hold it until v1.1 is available for uplifts.

Yep, "this" in this sentence was refering to bug 839715 (that was then duped to Bug 830463).

Batch edit: Bugs still affected on b2g18 after 2/13 merge to v1.0.1 branch are affected on v1.0.1 branch.

Lukas, could we finally land this in 1.0.1 ? Thanks

Comment on attachment 711438 [details] [diff] [review] patch v1 No risk, but if this does cause any new regressions, it'll need to be backed out immediately. Approving for both v1-train and v1.0.1.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/2ca46edc485f https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/af2a3c8f6ee6