813195 - [Apps] Cannot retry to download hosted apps with cache manifest

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Francisco Jordano [:arcturus] [:francisco]]

When we are unable to download a hosted application that has a cache manifest (this can happen if the user lost connectivity meanwhile downloading and app), we will enter in the following state:

|app.installState === 'pending' && !app.downloading|

There is now way to retry the download. For packaged apps we can call the method |.download()| but this don't work for hosted apps.

Comment 1

[Francisco Jordano [:arcturus] [:francisco]]

:fabrice, IMHO, my desired option is to call always the download() method, no matter if it's a packaged app or if it's a hosted one.

Internally I understand that behaviors are completely different but will be clear for the API semantic.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Calling .download() *should* work. Even for hosted apps. But could be that that is broken.

Comment 3

[[:fabrice] Fabrice Desré]

It's not broken, it's just not there at all.

Comment 4

[[:fabrice] Fabrice Desré]

Attachment: patch

Let me know if that works, I could not find in gaia how to restart the download...

Comment 5

[[:fabrice] Fabrice Desré]

Attachment: wip v2

Better wip. I was actually able to complete a full update cycle with this one, but then gaia kept asking me to update the same set of apps.

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Thanks Fabrice, I was able to test restarting a cancelled appcache download with the attached patch and Francisco's app_resume branch [1].

However, I still see the issue of the system app not being able to receive notifications about a restarted download in same cases. Particularly, the cases where the system app looses the reference to the application which download has been cancelled. I am wondering if we could fire an event (maybe 'install' again or a new 'retryInstall' or 'download' or whatever) over the 'navigator.mozApps.mgmt'.

[1] https://github.com/arcturus/gaia/tree/apps_resume

Comment 7

[Francisco Jordano [:arcturus] [:francisco]]

Nothing else to add to Fernando comment, we were checking with :julienw and they will need to listen to an event in the system app to show the notifications, which one is the best one, I guess we need to decide.

Thanks!

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 683764 [details] [diff] [review]
wip v2

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

r=me with the comments addressed, please :)

::: dom/apps/src/Webapps.jsm
@@ +19,5 @@
>  Cu.import("resource://gre/modules/PermissionsInstaller.jsm");
>  Cu.import("resource://gre/modules/OfflineCacheInstaller.jsm");
>  
>  function debug(aMsg) {
> +  dump("-*-*- Webapps.jsm : " + aMsg + "\n");

Comment this line in the final patch, please :)

@@ +778,1 @@
>        debug("startDownload: No app found for " + aManifestURL);

Should we fire an 'ondownloaderror' here?

@@ +783,5 @@
>      let file = FileUtils.getFile(DIRECTORY_NAME,
>                                   ["webapps", id, "update.webapp"], true);
>  
> +    if (!file.exists()) {
> +      // This is a hosted app, let's check if it has a appcache and download it.

nit: 81 chars :)
nit: Also 'an appcache' or only 'appcache'

@@ +795,5 @@
> +        } else {
> +          // hosted app with no appcache, nothing to do, but we fire a
> +          // downloaded event
> +          DOMApplicationRegistry.broadcastMessage("Webapps:PackageEvent",
> +                                                    { type: "downloaded",

nit: alignment

@@ +798,5 @@
> +          DOMApplicationRegistry.broadcastMessage("Webapps:PackageEvent",
> +                                                    { type: "downloaded",
> +                                                      manifestURL: aManifestURL,
> +                                                      app: app,
> +                                                      manifest: aResults[0].manifest });

I think we are not using the manifest in the message handler http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#604

Comment 10

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37b088859341

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/37b088859341

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c2df97608b76
https://hg.mozilla.org/releases/mozilla-beta/rev/bf2b5ac37ba7

Comment 13

[Tony Chung [:tchung]]

adding smoketest keyword.  this has been appearing when apps have updates and appears in the notifications dialog.   we can test this fix if hosted apps have a manifest available now.

Comment 14

[Jason Smith [:jsmith]]

(In reply to Tony Chung [:tchung] from comment #13)
> adding smoketest keyword.  this has been appearing when apps have updates
> and appears in the notifications dialog.   we can test this fix if hosted
> apps have a manifest available now.

Unrelated. This bug focuses on installed apps. The marketplace apps issue I think is still open a bug and is a separate problem, as it deals with a preloaded app.