Closed Bug 838337 Opened 7 years ago Closed 7 years ago

When restarting a download for a hosted app preloading appcache, the app icon can be selected again to restart the download again

Categories

(Core Graveyard :: DOM: Apps, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:-, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

RESOLVED FIXED
mozilla21
blocking-b2g -
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 + fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: jsmith, Assigned: julienw)

References

Details

Attachments

(2 files)

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.
Blocks: app-install
I don't like the "occasionally" part ;) Did you manage to know how to reproduce this consistently ?
Component: Gaia::System → Gaia::Homescreen
(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.
tracking-b2g18: --- → ?
I'll have a look
Flags: needinfo?(felash)
Depends on: 838561
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.
Flags: needinfo?(felash)
For testers: app from http://everlong.org/mozilla/selfupdatinghosted/ is a good fit.
Unable to test right now because of Bug 838823.
Depends on: 838823
Attached patch debug patchSplinter Review
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.
Component: Gaia::Homescreen → DOM: Apps
Product: Boot2Gecko → Core
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.
Assignee: nobody → felash
Attached patch patch v1Splinter Review
This needs the patch to bug 838823 to be tested.

This just sets app.downloading to true when we get an appcache progress event.
Attachment #711438 - Flags: review?(fabrice)
Attachment #711438 - Flags: review?(fabrice) → review+
Keywords: checkin-needed
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
Attachment #711438 - Flags: approval-mozilla-b2g18?
https://hg.mozilla.org/mozilla-central/rev/a24513160922
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Keywords: verifyme
QA Contact: jsmith
Comment on attachment 711438 [details] [diff] [review]
patch v1

great reward for low risk, approving.
Attachment #711438 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
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.
Attachment #711438 - Flags: approval-mozilla-b2g18+ → approval-mozilla-b2g18?
(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.
Keywords: verifyme
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.
Keywords: verifyme
Keywords: verifyme
(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
blocking-b2g: --- → tef?
blocking-b2g: tef? → -
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.
Attachment #711438 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.