Last Comment Bug 772363 - Implement installation API for packaged apps
: Implement installation API for packaged apps
Status: VERIFIED FIXED
[qa!]
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM: Apps (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: [:fabrice] Fabrice Desré
: Jason Smith [:jsmith]
Mentors:
Depends on:
Blocks: Apps-Dev-Doc-Needed privileged-apps sign-packaged-apps
  Show dependency treegraph
 
Reported: 2012-07-10 01:34 PDT by Jonas Sicking (:sicking) No longer reading bugmail consistently
Modified: 2012-08-17 22:56 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (15.20 KB, patch)
2012-07-10 15:16 PDT, [:fabrice] Fabrice Desré
21: review+
Details | Diff | Splinter Review

Description Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-10 01:34:46 PDT
Implement a separate mozApps.install function which rather than provide a URL to a manifest, provides a URL to a packaged app, which contains manifest as well as other app resources.
Comment 1 [:fabrice] Fabrice Desré 2012-07-10 15:16:51 PDT
Created attachment 640816 [details] [diff] [review]
patch

I'm moving here Part 1 of bug 769350.

> @@ +412,5 @@
> > +      let ostream = FileUtils.openSafeFileOutputStream(zipFile);
> > +      NetUtil.asyncCopy(aInput, ostream, function (aResult) {
> > +        if (!Components.isSuccessCode(aResult)) {
> > +          // We failed to save the zip.
> > +          cleanup("NETWORK_ERROR");
> 
> This is not really a NETWORK_ERROR

Yep, I changed it to DOWNLOAD_ERROR. 

> @@ +436,5 @@
> > +                        .createInstance(Ci.nsIZipReader);
> > +        try {
> > +          zipReader.open(zipFile);
> > +          if (!zipReader.hasEntry("manifest.webapp")) {
> > +            throw "No manifest.webapp found.";
> 
> Don't you want to call cleanup("INVALID_MANIFEST") here instead?

No, I'm throwing there so the cleanup is called in the catch() and we still close the reader properly in the finally block.
Comment 2 Anant Narayanan [:anant] 2012-07-10 16:08:29 PDT
What is the risk in using the same mozApps.install() function, and have it take either a manifest URL or a package URL? In other words, what is the rationale for a separate installPackage() function?
Comment 3 [:fabrice] Fabrice Desré 2012-07-11 08:39:21 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/484d2214d81a
Comment 4 Jason Smith [:jsmith] 2012-07-11 09:02:18 PDT
(In reply to Fabrice Desré [:fabrice] from comment #3)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/484d2214d81a

Shouldn't we address API concerns in comment 2 before landing this? Has this gone through an API review yet on dev.webapps? I know I saw discussion on dev.webapps about packaged apps, but not sure if I explicitly saw the API part of the discussion (although I could have missed it while reading it).
Comment 5 Jason Smith [:jsmith] 2012-07-11 09:09:34 PDT
(In reply to Jason Smith [:jsmith] from comment #4)
> (In reply to Fabrice Desré [:fabrice] from comment #3)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/484d2214d81a
> 
> Shouldn't we address API concerns in comment 2 before landing this? Has this
> gone through an API review yet on dev.webapps? I know I saw discussion on
> dev.webapps about packaged apps, but not sure if I explicitly saw the API
> part of the discussion (although I could have missed it while reading it).

Also to add to this - We should also address how this affects each platform (desktop, android, and b2g) and know if there are any other pieces that need to be implemented on each side as followups.
Comment 6 Myk Melez [:myk] [@mykmelez] 2012-07-11 10:03:14 PDT
(In reply to Jason Smith [:jsmith] from comment #4)
> (In reply to Fabrice Desré [:fabrice] from comment #3)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/484d2214d81a
> 
> Shouldn't we address API concerns in comment 2 before landing this? Has this
> gone through an API review yet on dev.webapps?

It's often useful to discuss API changes/additions on dev.webapps, but there is no requirement that we do so, and discussions in that forum would not constitute an API review in any case.  API reviews are the responsibility of the folks in charge of standardizing the APIs (namely: Jonas and Anant), and it's up to them to determine whether, how, and when such reviews are conducted.
Comment 7 [:fabrice] Fabrice Desré 2012-07-11 10:10:08 PDT
I pushed a followup to fix a test failure :
https://hg.mozilla.org/integration/mozilla-inbound/rev/633f88dda03b
Comment 8 Anant Narayanan [:anant] 2012-07-11 10:15:19 PDT
I think we can just discuss the API change in this bug, since Jonas opened it.

I'm not against adding an installPackage() function, I'm only trying to determine the reasoning behind it as it seems like the existing install() function would be adequate. If Jonas believes 
there's a good reason, that's great, it would be nice to know what it is.

Fabrice, have you tested your change on Desktop/Android or does this only work on B2G for now?
Comment 9 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-11 20:30:26 PDT
If we have them as the same function we have to detect somehow what the URI is pointing at. I can think of the following solutions:

* Use the mime type of the response.
* Use the extension of the file-part of the URL.
* Detect if the response body looks like a JSON file, or like a zip file (or
  whatever other package format we end up standardizing on)

Using mime types suck. Developers hate having to configure their server to send correct mime types, and many times they can't or don't know how to.

Using extensions is always iffy since in theory file names shouldn't mean anything. And it might make it harder to serve the file from some servers which would want to use URLs like: http://myserver.com/manifestServer.php?id=42231

Looking at the response body always feels scary. It might limit the type of packaging formats we can support in the future if some of them doesn't have a prefixed which can be uniquely distinguishable from whatever a JSON file can start with.


In general, given that the behavior of the two functions are so different it seems worth it to me to be explicit about what the function does.

The only downside that I can see of having two functions is if a store keeps a database of all apps and stores a download-URL for each app. With the two separate functions they would have to store the URL differently for packaged apps, and apps that are hosted by the developer.

However I suspect stores will store those differently anyway since it's hosting the packaged apps and likely will be managing them very differently.
Comment 10 Anant Narayanan [:anant] 2012-07-11 21:39:10 PDT
Thanks for the explanation, Jonas!

We do however already require the manifest to be served with a Content-Type of "application/x-web-app-manifest+json". Is it your intention that we should get rid of that requirement at some point?

The original purpose was mostly for security - it is a way to prove that the developer had control over the server that served a particular app. This is important in a one-app-per-origin world, since in shared hosting environments (geocities, or university sites like uni.edu/~user) we don't want subdirectories to represent an app on behalf of the top-level domain.
Comment 11 [:fabrice] Fabrice Desré 2012-07-12 00:46:21 PDT
(In reply to Anant Narayanan [:anant] from comment #10)
> Thanks for the explanation, Jonas!
> 
> We do however already require the manifest to be served with a Content-Type
> of "application/x-web-app-manifest+json". Is it your intention that we
> should get rid of that requirement at some point?

Note that this is not implemented currently - and I'm reluctant to make that a MUST, for the reasons Jonas explained.

> The original purpose was mostly for security - it is a way to prove that the
> developer had control over the server that served a particular app. This is
> important in a one-app-per-origin world, since in shared hosting
> environments (geocities, or university sites like uni.edu/~user) we don't
> want subdirectories to represent an app on behalf of the top-level domain.

Here also, things should change. We'd like to allow multiple apps per origin, each app being identified by its manifest url instead of its origin.
Comment 12 Anant Narayanan [:anant] 2012-07-12 04:46:41 PDT
(In reply to Fabrice Desré [:fabrice] from comment #11)
> > We do however already require the manifest to be served with a Content-Type
> > of "application/x-web-app-manifest+json". Is it your intention that we
> > should get rid of that requirement at some point?
> 
> Note that this is not implemented currently - and I'm reluctant to make that
> a MUST, for the reasons Jonas explained.

This is already implemented in the Marketplace at-least, if not in Gecko.

> > The original purpose was mostly for security - it is a way to prove that the
> > developer had control over the server that served a particular app. This is
> > important in a one-app-per-origin world, since in shared hosting
> > environments (geocities, or university sites like uni.edu/~user) we don't
> > want subdirectories to represent an app on behalf of the top-level domain.
> 
> Here also, things should change. We'd like to allow multiple apps per
> origin, each app being identified by its manifest url instead of its origin.

Sure, we should be working towards lifting that restriction. We should still be cautious though. For instance, if we only use the manifest URL to identify an app, I would be able to create a manifest for mozilla.github.com/awesomeapp at anantn.github.com/foobar.

In essence, it allows anyone to create an app on behalf of someone else, and increases the burden on the reviewers in our Marketplace to ensure that whoever uploaded the manifest does indeed own the app itself (which in itself can be hard to verify). No doubt, this is critically important for paid apps.
Comment 13 Anant Narayanan [:anant] 2012-07-12 04:51:40 PDT
(In reply to Anant Narayanan [:anant] from comment #12)
> Sure, we should be working towards lifting that restriction. We should still
> be cautious though. For instance, if we only use the manifest URL to
> identify an app, I would be able to create a manifest for
> mozilla.github.com/awesomeapp at anantn.github.com/foobar.

Correction, it won't work on github because of the subdomain (which makes it a different origin), but does on shared hosting environments like example.org/~user1 and example.org/~user2, or places like geocities, where user accounts are subfolders rather than subdomains.

It is also dangerous on places like Dropbox, Google Docs, etc. where user-generated content has publicly available URLs, which will allow any user to be able create an "app" for the top-level domain and potentially charge for it.
Comment 15 Anant Narayanan [:anant] 2012-07-12 10:01:25 PDT
Reference to the bug where we are discussing safely being able to relax the MIME type requirement: bug 712045
Comment 16 Jason Smith [:jsmith] 2012-07-12 10:45:37 PDT
This sounds like this is testable, but I need some pointers on how to understand how to test this. Questions:

- Could you provide a description of the API function at a high-level (I know I've seen discussion on this, just want to make sure I understand it correctly)
- Do we have any automated tests for this? If not, can we get a bug on file to get automated test support for this?
- Could you provide an example of an app demonstrating the use of the API function?
Comment 17 Jason Smith [:jsmith] 2012-07-17 20:27:00 PDT
Could someone address my questions in comment 16? I'd really like to help make sure this works across each major apps platform piece (desktop, android, and b2g).
Comment 18 Jason Smith [:jsmith] 2012-08-06 23:01:16 PDT
Verified on 8/6/2012 build using Fabrice's test app here - http://people.mozilla.org/~fdesre/openwebapps/test.html that the implementation is there at a proof of concept level. More testing and followups will be filed as needed.

Important Notes:

- The implementation for packaged apps only appears to work on Firefox OS. Desktop will fail to install the packaged app (errors out). Android will install the app, but with the incorrect icon, and also fail to launch the app.
- There's more testing to be done here. I'll look into this.

Note You need to log in before you can comment on or make changes to this bug.