772364 - Implement update mechanism for packaged apps

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

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

What this will look like is still to be determined. Either we just re-ping the same URL which we originally downloaded the app from. Or we define a new HTTP-API which the browser can ping to get a list of apps which have new versions available.

Or something else.

Comment 1

[[:fabrice] Fabrice Desré]

We want to avoid downloading apps that have not changed, since packages can be quite big.

We can do it like add-ons: reference a small update manifest from the app manifest and check this update manifest for new versions (note that we don't mandate a version number currently in apps manifests).

Or maybe just using the If-Modified-Since http header could be enough?

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Fabrice Desré [:fabrice] from comment #1)
> Or maybe just using the If-Modified-Since http header could be enough?

Using conditional requests is a very clean solution and one that doesn't re

It is usually better to use ETags (If-None-Match), and Necko will take care of that automatically for us.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Sorry, I hit "Save Changes" too early.

(In reply to Brian Smith (:bsmith) from comment #2)
> (In reply to Fabrice Desré [:fabrice] from comment #1)
> > Or maybe just using the If-Modified-Since http header could be enough?
> 
> Using conditional requests is a very clean solution and one that doesn't re

...require a lot of extra work to implement.

> It is usually better to use ETags (If-None-Match), and Necko will take care
> of that automatically for us.

Actually, that's not true because we're not storing these in the Necko HTTP cache, so we'd need to store the HTTP validation information (Last-Modified or ideally the ETag) as part of the metadata of what app has been installed.

Comment 4

[[:fabrice] Fabrice Desré]

(In reply to Brian Smith (:bsmith) from comment #3)

> Actually, that's not true because we're not storing these in the Necko HTTP
> cache, so we'd need to store the HTTP validation information (Last-Modified
> or ideally the ETag) as part of the metadata of what app has been installed.

We already store an installTime in the DOM registry so we could just reuse it to set the header(s) on the http channel.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Fabrice Desré [:fabrice] from comment #4)
> We already store an installTime in the DOM registry so we could just reuse
> it to set the header(s) on the http channel.

It would be good to store the exact validator (Last-Modified or ETag header) to use because then we don't have to worry about clock mismatches between the server and the client or any other such things.

Comment 6

[[:fabrice] Fabrice Desré]

Attachment: wip Part 1 : add packageURL and lastModified to the DOM registry

Comment 7

[[:fabrice] Fabrice Desré]

Attachment: wip Part 2 : updater component

Comment 8

[[:fabrice] Fabrice Desré]

Attachment: wip Part 3 : b2g packaging

Comment 9

[[:fabrice] Fabrice Desré]

These patches use the If-Modified-Since header to only update modified apps. That works well with my local apache install serving static files.

Some things that are still unknown to me:
- Some parts of the user flow : when and which kind of prompts do we want to show to the user?
- The mozApps API may need a new event on the app object "onupdate" to be used by dashboards when we have finished an update.
- Integration with the marketplace and sites using browserID to authenticate users.

Comment 10

[Patrick McManus [:mcmanus]]

please use etag/if-none-match instead of l-m/i-m-s..

the former doesn't have problems with clock warps, is easier to parse, and doesn't have a problem with subsecond granularity.

Comment 11

[Jason Smith [:jsmith]]

(In reply to Fabrice Desré [:fabrice] from comment #9)
> These patches use the If-Modified-Since header to only update modified apps.
> That works well with my local apache install serving static files.
> 
> Some things that are still unknown to me:
> - Some parts of the user flow : when and which kind of prompts do we want to
> show to the user?
> - The mozApps API may need a new event on the app object "onupdate" to be
> used by dashboards when we have finished an update.
> - Integration with the marketplace and sites using browserID to authenticate
> users.

Might be good to add these open questions here - https://wiki.mozilla.org/Talk:Apps/PackagingProposal. Wil, myself, Krupa have been adding questions here to build an understanding of the packaged apps (documented here - https://wiki.mozilla.org/Apps/PackagingProposal).

Comment 12

[[:fabrice] Fabrice Desré]

Attachment: wip Part 1 : add packageURL and lastModified to the DOM registry

Using Etag instead of If-Modified-Since

Comment 13

[[:fabrice] Fabrice Desré]

Attachment: wip Part 2 : updater component

Also updated to use Etag.

Comment 14

[Gina Yeh [:gyeh] [:ginayeh]]

Hi Fabrice,
I'm going to fit my implementation in bug 744715 into this bug. The attachments was created a few days ago. Please let me know if there's any update from you. Thanks.

Comment 15

[Gina Yeh [:gyeh] [:ginayeh]]

Is there any test case?

Comment 16

[[:fabrice] Fabrice Desré]

re-assigning to mounir.

Comment 17

[Lucas Adamski [:ladamski]]

Ping.. this seems to have stalled out?

Comment 18

[[:fabrice] Fabrice Desré]

That will start again now that we have the API we need to implement designed in bug 790558.

Comment 19

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

Actually, i'm going to close this bug in favor of bug 790558. It tracks the platform work needed to implement update.

Comment 20

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

Arg, I meant to change that comment into a question.

Is there a reason to keep this bug open? I would imagine that bug 790558 will cover the platform work and then we'll have a gaia issue for the remaining UI work.

Comment 21

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

Actually, ignore my ramblings. We'll just use this bug for implementing the API described in bug 790558 comment 0 during the update of a packaged app. That's basically what it was originally intended for.

Comment 22

[Mounir Lamouri (:mounir)]

Fabrice told me he was working/will work on this. Updating the assignee field to make this clearer.

Comment 23

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

Comment on attachment 645781 [details] [diff] [review]
wip Part 2 : updater component

Marking these obsolete as they implement an old proposal.

Comment 24

[[:fabrice] Fabrice Desré]

Attachment: patch

During update of packaged app:
When we detect that an update is available we set "downloadAvailable" to true and fire "downloadavailable".

When the download() function is called, we set .installState to "updating" and start downloading and fire progress events as needed.

If .launch is called during this time I think we should allow the app to start. But it would be running the old version.

When we're done downloading the package we set readyToApplyDownload to true and fire "downloaded".

When mgmt.applyDownload() is called we swap out the package and fire "downloadapplied".

If cancelDownload() is called we reset .installState to "installed" and .downloadProgress to 0. We also delete all downloaded data for the new version.

If an error occurs we behave like for cancelDownload but also set .downloadError appropriately.


Try run at : https://tbpl.mozilla.org/?tree=Try&rev=e36bdd58dc97

Comment 25

[Gregor Wagner [:gwagner]]

Comment on attachment 666090 [details] [diff] [review]
patch

>     return request;
>diff --git a/dom/apps/src/Webapps.jsm b/dom/apps/src/Webapps.jsm
>--- a/dom/apps/src/Webapps.jsm
>+++ b/dom/apps/src/Webapps.jsm
>@@ -13,17 +13,17 @@ let EXPORTED_SYMBOLS = ["DOMApplicationR
> 
> Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> Cu.import("resource://gre/modules/Services.jsm");
> Cu.import("resource://gre/modules/FileUtils.jsm");
> Cu.import('resource://gre/modules/ActivitiesService.jsm');
> Cu.import("resource://gre/modules/AppsUtils.jsm");
> 
> function debug(aMsg) {
>-  //dump("-*-*- Webapps.jsm : " + aMsg + "\n");
>+  dump("-*-*- Webapps.jsm : " + aMsg + "\n");
> }


comment


 
>     this.frameMessages = ["Webapps:ClearBrowserData"];
> 
>     this.messages.forEach((function(msgName) {
>       ppmm.addMessageListener(msgName, this);
>     }).bind(this));
> 
>     cpmm.addMessageListener("Activities:Register:OK", this);
>@@ -481,22 +482,28 @@ let DOMApplicationRegistry = {
>         this.addMessageListener(msg, mm);
>         break;
>       case "Webapps:UnregisterForMessages":
>         this.removeMessageListener(msg, mm);
>         break;
>       case "Webapps:GetList":
>         this.addMessageListener(["Webapps:AddApp", "Webapps:RemoveApp"], mm);
>         return this.webapps;
>+      case "Webapps:Download":
>+        this.startDowload(msg.manifestURL);
>+        break;


startDownload!
I guess we don't have many tests :(


>       case "Webapps:CancelDownload":
>         this.cancelDowload(msg.manifestURL);
>         break;

also Download! Please grep for it. There are more dowload in the code!



> 
>+  startDownload: function cancelDowload(aManifestURL) {


startDownload



>+        function(aId, aManifest) {
>+          // Success! Keep the zip in of TmpD, we'll move it out when
>+          // applyDowload() will be called.
>+          let tmpDir = FileUtils.getDir("TmpD", ["webapps", aId], true, true);
>+
>+          // Save the manifest in TmpD also
>+          let manFile = tmpDir.clone();
>+          manFile.append("manifest.webapp");
>+          DOMApplicationRegistry._writeFile(manFile,
>+                                            JSON.stringify(aManifest),
>+                                            function() { });
>+          // Set state and fire events.
>+          app.dowloading = false;
>+          app.dowloadavailable = false;


download




>+    let dir = FileUtils.getDir(DIRECTORY_NAME, ["webapps", id], true, true);
>+    appFile.moveTo(dir, "application.zip");
>+    manFile.moveTo(dir, "manifest.webapp");
>+
>+    try {
>+      tmpDir.remove(true);
>+    } catch(e) { }
>+


Write some log?




>     function sendError(aError) {
>       aData.error = aError;
>       aMm.sendAsyncMessage("Webapps:CheckForUpdate:Return:KO", aData);
>     }
> 
>     function updatePackagedApp(aManifest) {
>       debug("updatePackagedApp");
>+      let manifest = new DOMApplicationManifest(aManifest, app.manifestURL);
>+      // A package is available: set downloadAvailable to fire the matching
>+      // event.
>+      app.downloadAvailable = true;
>+      app.downloadSize = manifest.size;
>+      aData.event = "downloadavailable";
>+      aData.app = {
>+        downloadAvailable: true,
>+        downloadSize: manifest.size
>+      }
>+      DOMApplicationRegistry._saveApps(function() {
>+        aMM.sendAsyncMessage("Webapps:CheckForUpdate:Return:OK", aData);
>+      });

do you mean aMm? 




>       }
>       // Copy the zip on disk.
>       let zipFile = FileUtils.getFile("TmpD",
>                                       ["webapps", id, "application.zip"], true);
>       let ostream = FileUtils.openSafeFileOutputStream(zipFile);
>+      debug("ok 1");

remove

>       NetUtil.asyncCopy(aInput, ostream, function (aResult) {
>         if (!Components.isSuccessCode(aResult)) {
>           // We failed to save the zip.
>           cleanup("DOWNLOAD_ERROR");
>           return;
>         }
>-
>+        debug("ok 2");

remove

>-            DOMApplicationRegistry.broadcastMessage("Webapps:PackageEvent",
>-                                                    { type: "installed",
>-                                                      manifestURL: aApp.manifestURL,
>-                                                      app: app,
>-                                                      manifest: manifest });
>-            delete DOMApplicationRegistry.downloads[aApp.manifestURL]
>-          });
>+          debug("ok 3");

remove

Comment 26

[[:fabrice] Fabrice Desré]

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

Comment 27

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/25041824911b - one of the things in that push was causing

9351 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_cross_origin.xul | Test timed out.
9354 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_getNotInstalled.xul | Test timed out.
9358 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_app.xul | Test timed out.
9364 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul | Test timed out.
9365 ERROR TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | 4 test timeouts, giving up.
9366 ERROR TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | Skipping 404 remaining tests.
9367 ERROR TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul finished in a non-clean fashion, probably because it didn't call SimpleTest.finish()

Comment 28

[[:fabrice] Fabrice Desré]

re-pushed since this was not the cause of test failure:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6676711a510b

Comment 29

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/6676711a510b

Comment 30

[Rob Hudson [:robhudson]]

This doesn't look right:
>   startDownload: function cancelDownload(aManifestURL) {
https://hg.mozilla.org/mozilla-central/rev/6676711a510b#l2.91

Not sure if the function name matters there though?