mozApp.download() packaged app issues

RESOLVED FIXED in Firefox 18

Status

defect
P1
normal
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: etienne, Assigned: fabrice)

Tracking

unspecified
mozilla19
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 6 obsolete attachments)

Reporter

Description

7 years ago
I have a packaged app with |downloadAvailable| true, and calling |.download()| isn't doing much.

Leads:
- We send a |Webapps:Download| message while the ppmm listens for |Webapps::Download|
- |startDownload| is doing a |this.getAppByManifestURL(manifestURL)| while the parameter name is |aManifestURL|

But fixing those isn't enough...

PS: The gaia branch where the basic app update workflow is implemented is here https://github.com/etiennesegonzac/gaia/tree/app-updates
Reporter

Updated

7 years ago
blocking-basecamp: --- → ?
Reporter

Comment 1

7 years ago
Just uncompress at the http server root, I'm pointer fakeapp.com at 127.0.0.1.

The installation is done by opening the browser app, going to fakeapp.com and clicking Install.

The manifest in the zip is different from the one at the root, so the app always has a download available.
Fabrice says he'll take this and that it's a blocker.
Assignee: nobody → fabrice
blocking-basecamp: ? → +
Priority: -- → P1
Assignee

Comment 3

7 years ago
Posted patch wip (obsolete) — Splinter Review
Etienne, can you try this patch? Here's what I got with it:

XXX FIXME : Got a mozContentEvent: webapps-install-granted
-*-*- Webapps.jsm : {"name":"Fake app","installOrigin":"http://fakeapp.com","origin":"http://fakeapp.com","receipts":[],"installTime":1350522676422,"manifestURL":"http://fakeapp.com/manifest.webapp","appStatus":1,"removable":true,"localId":1001,"progress":0,"installState":"pending","downloadAvailable":true,"downloading":true,"readyToApplyDownload":false,"downloadSize":0,"lastUpdateCheck":1350522676422,"etag":"\"3006f0-150-4cc42a828da00\"","basePath":"/home/fabrice/dev/gaia/profile/webapps"}
-*-*- Webapps.jsm : About to download http://fakeapp.com/application.zip
-*-*- Webapps.jsm : onProgress: 5601/5601
[Child 10598] WARNING: NS_ENSURE_TRUE(IsChromeProcess()) failed: file /home/fabrice/dev/inbound/content/base/src/nsFrameMessageManager.cpp, line 687
-*-*- Webapps.jsm : About to fire Webapps:PackageEvent
JavaScript error: app://homescreen.gaiamobile.org/js/grid.js, line 644: Applications.getManifest(...) is undefined

Everything looked good in the webapps.json registry, and indeed when relaunching b2g I had the new app listed and loadable.
Reporter

Comment 4

7 years ago
Posted patch Wip with fixed typo? (obsolete) — Splinter Review
I can't find anywhere where we listen to |Webapps::Download| and the switch case refers to |Webapps:Download|.

So sending a |Webapps:Download|.
Reporter

Comment 5

7 years ago
With this patch I get a:

-*-*- Webapps.jsm : startDownload: No update manifest found at /home/segonzac/Desktop/gaia/profile/webapps/{4ac2a55a-129b-4622-a79f-76f5c0b4dcdc}/update.webapp http://fakeapp.com/manifest.webapp

But it's unclear to me where the update.webapp should come from.

Some more details to clarify:
- No issue at app install
- even if the manifest in the zip is the same that outside, |downloadAvailable| is always true after the first |checkForUpdate()|
- calling |download()| at this point will trigger the "no update manifest error"
Assignee

Comment 6

7 years ago
Posted patch wip v2 (obsolete) — Splinter Review
Another wip for you to try, with also a bunch of new debug statements.

Also, you need to change your test site to use mozApp.installPackage() instead of install() !

Here's a small gaia fix to let this work now that the first manifest is app.updateManifest and not app.manifest for packaged apps:

diff --git a/apps/system/js/permission_manager.js b/apps/system/js/permission_manager.js
index 1ee6ed1..07f8617 100644
--- a/apps/system/js/permission_manager.js
+++ b/apps/system/js/permission_manager.js
@@ -77,8 +77,9 @@ var PermissionManager = (function() {
       return;
     }
 
-    var name = app.manifest.name;
-    var locales = app.manifest.locales;
+    var manifest = app.manifest ? app.manifest : app.updateManifest;
+    var name = manifest.name;
+    var locales = manifest.locales;
     var lang = navigator.language;
     if (locales && locales[lang] && locales[lang].name)
       name = locales[lang].name;
Attachment #672627 - Attachment is obsolete: true
Attachment #672772 - Attachment is obsolete: true
Reporter

Comment 7

7 years ago
Posted patch WIP v3 (obsolete) — Splinter Review
Some events names fixes, a pinch of app retrieval change, a zest of typos corrections.

And...
IT'S UPDATING \o/
Attachment #672906 - Attachment is obsolete: true
Blocks: 803790
No longer blocks: 803790
Reporter

Comment 8

7 years ago
Downloading and updating still work fine, but new issue (trying to group them here).

Every time checkForUpdate is called, downloadAvailable is set to true, even if the app just got updated and the manifest are identical.

Can this just be an apache config error causing etag issues?
Reporter

Updated

7 years ago
Summary: mozApp.download() doesn't do anything (packaged app) → mozApp.download() packaged app issues
Assignee

Comment 9

7 years ago
Posted patch wip 4 (obsolete) — Splinter Review
Etienne, we were not setting the If-None-Match header properly, so we were seeing a new package each time. Can you test again?
Attachment #673228 - Attachment is obsolete: true
Reporter

Comment 10

7 years ago
Some progress...

* After the initial installation I won't get any downloadAvailable until I change the manifest on the server.

* _But_, after an update I get into the same state than before where checkForUpdate always put |downloadAvailable| to true
* The last version of your patch still calls |this.ApplyDownload| instead of |this.applyDownload|
Reporter

Comment 11

7 years ago
Posted patch wip v5 (obsolete) — Splinter Review
Persisting the etag properly on update.
Attachment #674360 - Attachment is obsolete: true
Reporter

Updated

7 years ago
Blocks: 799888
Assignee

Updated

7 years ago
Attachment #674668 - Attachment is patch: true
Assignee

Comment 12

7 years ago
Posted patch patchSplinter Review
Very similar to v5 - I removed a duplicate |let id =...| in startDownload()
Attachment #674668 - Attachment is obsolete: true
Attachment #674765 - Flags: review?(21)
Comment on attachment 674765 [details] [diff] [review]
patch

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

r+ with the debug method turned off by default.

::: 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");

You forgot to comment that back. :)

@@ +848,5 @@
>        aMm.sendAsyncMessage("Webapps:CheckForUpdate:Return:KO", aData);
>        return;
>      }
>  
> +    let installOrigin = app.installOrigin;

Is is used?
Attachment #674765 - Flags: review?(21) → review+
https://hg.mozilla.org/mozilla-central/rev/3e4d63da1bce
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Keywords: verifyme
QA Contact: jsmith
Keywords: verifyme
Whiteboard: [qa-]

Updated

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