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

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Julien Wajsberg [:julienw]]

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.

Comment 1

[Julien Wajsberg [:julienw]]

STR from Bug 838337 with app in http://everlong.org/mozilla/selfupdatinghosted/

Comment 2

[[:fabrice] Fabrice Desré]

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.

Comment 3

[Julien Wajsberg [:julienw]]

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.

Comment 4

[Julien Wajsberg [:julienw]]

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.

Comment 5

[Julien Wajsberg [:julienw]]

Attachment: patch v1

Couldn't try this yet because of bug 838823

Comment 6

[Julien Wajsberg [:julienw]]

Attachment: patch v2

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.

Comment 7

[[:fabrice] Fabrice Desré]

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

Comment 8

[Julien Wajsberg [:julienw]]

(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.

Comment 9

[Julien Wajsberg [:julienw]]

Attachment: patch v3

fixed nit and carrying r=fabrice

Comment 10

[Julien Wajsberg [:julienw]]

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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eca4d9c0301b

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eca4d9c0301b

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/81e6d3615ca6