If the caller to download tries to start a download but we are already downloading, ignore the call

RESOLVED FIXED in Firefox 21

Status

defect
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: julienw, Assigned: julienw)

Tracking

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

Firefox Tracking Flags

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

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

7 years ago
This is sort of a follow-up to Bug 836909, and a gecko fix for Bug 838337.

In Bug 838337, there is a bug in the homescreen where, for some reason, we call download whereas we're already downloading. This happens for hosted+appcache only. As a result, the appcache is downloaded twice.

I'm not sure what we should do, maybe just ignore the call, because I think that sending an error as in Bug 836909 would not make Gaia happy.
I also think we should just do nothing when a download is already in progress. We'll get the events from the one running anyway.
Assignee

Updated

7 years ago
blocking-b2g: --- → tef?
Assignee

Comment 3

7 years ago
Ok, this would maybe prevent some future bugs but this will not fix bug 838337 after all. I just verified that "downloading" is false with the STR in bug 838337 so I'm removing the nom.
No longer blocks: b2g-app-updates
blocking-b2g: tef? → ---
Assignee

Comment 4

7 years ago
forget comment 3, webapps.json is not saved at this time so I was not seeing correct values. Checked with logs and "downloading" is correctly true.

So that would actually fix Bug 838337.
blocking-b2g: --- → tef?
Assignee

Updated

7 years ago
Summary: If the caller to download tries to start a download but we are already downloading, throw an error → If the caller to download tries to start a download but we are already downloading, ignore the call
Assignee

Updated

7 years ago
Assignee: nobody → felash
Assignee

Comment 5

7 years ago
Posted patch patch v1 (obsolete) — Splinter Review
Couldn't try this yet because of bug 838823
Attachment #710957 - Flags: review?(fabrice)
Assignee

Updated

7 years ago
Depends on: 838823
Assignee

Updated

7 years ago
Attachment #710957 - Flags: review?(fabrice)
Assignee

Comment 6

7 years ago
Posted patch patch v2 (obsolete) — Splinter Review
This can be tested with the patch in Bug 838823.

This works for me with the reported STR.

I also added the check in checkForUpdate that could be removed when Bug 839071 will be fixed.
Attachment #710957 - Attachment is obsolete: true
Attachment #711418 - Flags: review?(fabrice)
Comment on attachment 711418 [details] [diff] [review]
patch v2

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

::: dom/apps/src/Webapps.jsm
@@ +969,5 @@
>        return;
>      }
>  
> +    if (app.downloading) {
> +      debug("startDownload: app (" + aManifestURL + ") is already downloading. Ignoring.");

nit: no need to repeat "startDownload" and the manifestURL
Attachment #711418 - Flags: review?(fabrice) → review+
Assignee

Comment 8

7 years ago
(In reply to Fabrice Desré [:fabrice] from comment #7)
> Comment on attachment 711418 [details] [diff] [review]
> patch v2
> 
> Review of attachment 711418 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/src/Webapps.jsm
> @@ +969,5 @@
> >        return;
> >      }
> >  
> > +    if (app.downloading) {
> > +      debug("startDownload: app (" + aManifestURL + ") is already downloading. Ignoring.");
> 
> nit: no need to repeat "startDownload" and the manifestURL

I used the same format as the log that was just above it but ok, I'll remove this.
Assignee

Comment 9

7 years ago
Posted patch patch v3Splinter Review
fixed nit and carrying r=fabrice
Attachment #711418 - Attachment is obsolete: true
Attachment #711746 - Flags: review+
Assignee

Updated

7 years ago
Keywords: checkin-needed
Assignee

Comment 10

7 years ago
Comment on attachment 711746 [details] [diff] [review]
patch v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): -
User impact if declined: the user can ask an app download (update or install) or a check for update when it's already downloading, which will cause incoherences. An app can also ask for a download.
Testing completed: yes
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

This small patch brings guards around our Webapps download and update implementation so this is a big win (keeping consistency) for such a low risk.
Attachment #711746 - Flags: approval-mozilla-b2g18?
Not a blocker and we're concerned about regressions at this point in 1.0.1 so leaving the approval nomination until v1.1 is on mozilla-b2g18
blocking-b2g: tef? → -
tracking-b2g18: --- → +
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/eca4d9c0301b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Keywords: verifyme
QA Contact: jsmith
Keywords: verifyme
Batch edit: Bugs still affected on b2g18 after 2/13 merge to v1.0.1 branch are affected on v1.0.1 branch.
Comment on attachment 711746 [details] [diff] [review]
patch v3

Go ahead with uplift to mozilla-b2g18 tip for v1.1.0 as per https://wiki.mozilla.org/Release_Management/B2G_Landing#More_Details
Attachment #711746 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+

Updated

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