Implement navigator.mozApps to register web apps and enumerate them.

Isn't that going to highly increase fingerprinting risks?

draft API outline navigator.apps.self: getter, returns application object for current origin if installed, null otherwise navigator.apps.install(manifestUrl, success, error): success or error is called (no arguments for success, error gets an error object) navigator.apps.uninstall(origin, success, error): dito navigator.apps.list(success, error): success is called with an array of application objects application object properties: manifest: string (original manifest as a string, easy to store, easy to parse) origin: string (origin of the web app) installOrigin: string (store that installed this, or == origin if selfhosted) installTime: long (timestamp of the install) A couple pieces are left out from the original OWA proposal for the JS API, most notably launch of apps and "installData", which is intended for purchase verification. We probably don't really need or want launch, list() gives all the data needed to self-host it. installData might be added as an optional argument to install. We will tackle that separately. Security model: Stores can query only apps they installed (list returns a subset). When called from privileged code, list() shows all installed web apps. install can only install the current origin if called unprivileged. privileged it can install other origins (thats what stores use) uninstall can uninstall the current origin, and things that have installOrigin == current origin. privileged it can uninstall anything. Security interaction: install pops up a door hanger to install the web app, similar to extensions uninstall simply uninstalls (with the limits above, this might need more debate) stores can gain store privileges (install other origins) by simply using the API. the first time they try to do a privileged operation, we ask the user whether thats ok and give a yes/no/always option

comment #2: - a regular web site can only get back data it installed (its own manifest) - a privileged store, after it was authorized by the user, can get back a list of applications that store installed itself - higher privilege than that is only used by chrome/UI code to manually deinstall apps without store involvement

As implementation strategy we probably want to implement an apps service in JS and have the DOM code talk to it. The apps service will be using indexdb (or sqlite directly if we are unlucky), thats probably a lot easier from JS and faster to implement that way. This also makes the apps registry backend replaceable.

Initial thoughts - don't like identifying apps by origins/domains. That's going to make publishing apps more difficult. - I'm not convinced of the need for explicit install() API for self-managing apps. Can't the app specify a manifest through <meta> or something like that and the UA automatically offer install UI? - what's the use case for explicit self-uninstall()? - since installing an app from a store is an explicit user action, it seems like this could be a contextual grant per-install instead of a blanket grant to the app store - I think even enumerating apps installed by a particular lower-trust store is a potential privacy leak. (Think of a giant app aggregator store.) I think enumerating installed apps needs permission grant on the order of managing contacts. > Stores can query only apps they installed (list returns a subset). When called from > privileged code, list() shows all installed web apps. How would UI for explicitly uninstalling self-installed apps work? Thinking about b2g here. Are you proposing two app-store "rings", one for "all apps" and another for less-trusted stores? Nit: instead of apps.list(), apps.enumerate() A problem we're going to face is localizing manifests, but we don't need to solve that here. Maybe that should be up to servers/stores anyway.

The manifest is documented here: https://developer.mozilla.org/en/Apps/App_Manifest http://code.google.com/chrome/apps/docs/developers_guide.html We have to nail down the permissions model, and google added some features we really want (background, in particular), we should merge those.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #6) > Initial thoughts > > - don't like identifying apps by origins/domains. That's going to make > publishing apps more difficult. A domain is bound to a SSL cert. foo.com doesn't control bar.foo.com. Paths don't give that guarantee. foo.bar.com/a can influence foo.bar.com/a/b. Thats my main worry here. But I can be swayed. No strong opinion here. > > - I'm not convinced of the need for explicit install() API for > self-managing apps. Can't the app specify a manifest through <meta> or > something like that and the UA automatically offer install UI? Yeah, I think I agree. We should discover the manifest, and offer install once. install() can be used for additional programmatic install offers beyond that. > > - what's the use case for explicit self-uninstall()? Dunno. It looked symmetrical. Maybe its not needed. Should we ban the self-uninstall case? We need the uninstall API, so it doesn't seem to hurt. > > - since installing an app from a store is an explicit user action, it seems > like this could be a contextual grant per-install instead of a blanket grant > to the app store Fair enough. > > - I think even enumerating apps installed by a particular lower-trust store > is a potential privacy leak. (Think of a giant app aggregator store.) I > think enumerating installed apps needs permission grant on the order of > managing contacts. Store X can only enumerate apps it installed itself, so it won't discover any information it doesn't already have, no? > > > Stores can query only apps they installed (list returns a subset). When called from > > privileged code, list() shows all installed web apps. > > How would UI for explicitly uninstalling self-installed apps work? Thinking > about b2g here. Are you proposing two app-store "rings", one for "all apps" > and another for less-trusted stores? I think there is a system applications widget that shows all installed applications and you can force-uninstall them. On top of that you have stores that access their own set of apps. > > Nit: instead of apps.list(), apps.enumerate() Yeah, I like it. > > A problem we're going to face is localizing manifests, but we don't need to > solve that here. Maybe that should be up to servers/stores anyway. The MDN version of the manifest has that, check it out.

We also need to figure out local caching. OWA doesn't seem to address that, and Google's solution is to either host an app (with normal web permissions) or package an app, which gives it access to chrome's extension apis. I don't think we want any of that. Maybe the simplest solution is to add another manifest field that specifies a .appcache file.

(In reply to Andreas Gal :gal from comment #8) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #6) > > Initial thoughts > > > > - don't like identifying apps by origins/domains. That's going to make > > publishing apps more difficult. > > A domain is bound to a SSL cert. foo.com doesn't control bar.foo.com. Paths > don't give that guarantee. foo.bar.com/a can influence foo.bar.com/a/b. > Thats my main worry here. But I can be swayed. No strong opinion here. > What "influence" are you worried about /a exerting over /a/b? > > > > - what's the use case for explicit self-uninstall()? > > Dunno. It looked symmetrical. Maybe its not needed. Should we ban the > self-uninstall case? We need the uninstall API, so it doesn't seem to hurt. > Fair enough. > > > > - I think even enumerating apps installed by a particular lower-trust store > > is a potential privacy leak. (Think of a giant app aggregator store.) I > > think enumerating installed apps needs permission grant on the order of > > managing contacts. > > Store X can only enumerate apps it installed itself, so it won't discover > any information it doesn't already have, no? > Not if the install is a temporary permission granted by the user, unknown to the store. > > > > A problem we're going to face is localizing manifests, but we don't need to > > solve that here. Maybe that should be up to servers/stores anyway. > > The MDN version of the manifest has that, check it out. Yeah, I'm dumb.

> What "influence" are you worried about /a exerting over /a/b? It simply doesn't create a strong app identity. Not a huge worry. I can easily lean either way. > > Store X can only enumerate apps it installed itself, so it won't discover > > any information it doesn't already have, no? > > > > Not if the install is a temporary permission granted by the user, unknown to > the store. "the user clicked a button, accepted install, but the store doesn't know" This applies to free apps only, paid apps are observable through the verification step. Unclear if this is a problem, more privacy minds needed. We can always tighten it down further, but we want to be careful not to nag the user too much.

Another issue we need to address is verified grant of permissions to apps. joebob.com/dialer.html shouldn't be able to self-install with telephony privileges. Going back to comment 6, maybe that's another argument for having two levels of trust in app stores. Not something we need to solve for v0 though.

(In reply to Andreas Gal :gal from comment #4) > comment #2: > > - a regular web site can only get back data it installed (its own manifest) Why is this ok? Especially, why is this ok, if user has cleared all the cookies etc. Note, for example for navigator.registerProtocolHandler we don't want to add API which allows the page to query whether it has already registered the protocol.

(In reply to Olli Pettay [:smaug] from comment #15) > (In reply to Andreas Gal :gal from comment #4) > > comment #2: > > > > - a regular web site can only get back data it installed (its own manifest) > Why is this ok? Especially, why is this ok, if user has cleared all the > cookies etc. > Installing an app is an action closer to installing an extension; it's a state change that persists until the user (or whatever) uninstalls the app. The "application object" is an immutable bit of sanitized data stuck into the app registry when the app is installed, so it's not something the app can use to track users. The one bit of data exposed to the app (installed/not installed) is something that paid apps will be able to query from a server anyway. What's your concern over this API? The additional bit of data exposed? > Note, for example for navigator.registerProtocolHandler we don't want to add > API > which allows the page to query whether it has already registered the > protocol. Out of curiosity, why not? The bit of identifying information exposed? Can't an application window.open() a URI for the protocol it attempted to install a handler for and discover whether or not it succeeded?

Updated skeleton for Fabrice. We don't need n.a.self, n.a.enumerate() will show the application object for the current origin if its installed, so n.a.self can be emulated easily. After chatting with the apps group, added an optional "receipt" string to install, which can be used for verification purposes. Enumerate uses callbacks, install/uninstall callbacks. For the latter this makes sense since we want to catch events even if it was not content script triggering the action (e.g. auto-install upon discovery). This makes enumerate strangely asymmetrical. In particular, should there be an error callback for enumerate, or should we just have the callback that delivers the list and call the onerror handler in case of an error. Jonas?

Comment on attachment 570619 [details] [diff] [review] patch >diff --git a/dom/Makefile.in b/dom/Makefile.in >--- a/dom/Makefile.in >+++ b/dom/Makefile.in >@@ -59,16 +59,17 @@ DIRS = \ > interfaces/load-save \ > interfaces/xul \ > interfaces/storage \ > interfaces/json \ > interfaces/offline \ > interfaces/geolocation \ > interfaces/notification \ > interfaces/svg \ >+ interfaces/apps \ > $(NULL) > > ifdef MOZ_SMIL > DIRS += interfaces/smil > endif > > DIRS += \ > base \ >diff --git a/dom/base/Makefile.in b/dom/base/Makefile.in >--- a/dom/base/Makefile.in >+++ b/dom/base/Makefile.in >@@ -45,16 +45,18 @@ include $(DEPTH)/config/autoconf.mk > MODULE = dom > LIBRARY_NAME = jsdombase_s > LIBXUL_LIBRARY = 1 > FORCE_STATIC_LIB = 1 > > EXTRA_PP_COMPONENTS = \ > ConsoleAPI.js \ > ConsoleAPI.manifest \ >+ WebApps.js \ >+ WebApps.manifest \ > $(NULL) > > EXTRA_JS_MODULES = ConsoleAPIStorage.jsm \ > $(NULL) > > XPIDLSRCS = \ > nsIEntropyCollector.idl \ > nsIScriptChannel.idl \ >diff --git a/dom/interfaces/apps/Makefile.in b/dom/interfaces/apps/Makefile.in >new file mode 100644 >--- /dev/null >+++ b/dom/interfaces/apps/Makefile.in >@@ -0,0 +1,53 @@ >+# ***** BEGIN LICENSE BLOCK ***** >+# Version: MPL 1.1/GPL 2.0/LGPL 2.1 >+# >+# The contents of this file are subject to the Mozilla Public License Version >+# 1.1 (the "License"); you may not use this file except in compliance with >+# the License. You may obtain a copy of the License at >+# http://www.mozilla.org/MPL/ >+# >+# Software distributed under the License is distributed on an "AS IS" basis, >+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License >+# for the specific language governing rights and limitations under the >+# License. >+# >+# The Original Code is web apps. >+# >+# The Initial Developer of the Original Code is Mozilla Foundation >+# Portions created by the Initial Developer are Copyright (C) 2011 >+# the Initial Developer. All Rights Reserved. >+# >+# Contributor(s): >+# Andreas Gal <gal@mozilla.com> >+# >+# Alternatively, the contents of this file may be used under the terms of >+# either the GNU General Public License Version 2 or later (the "GPL"), or >+# the GNU Lesser General Public License Version 2.1 or later (the "LGPL"), >+# in which case the provisions of the GPL or the LGPL are applicable instead >+# of those above. If you wish to allow use of your version of this file only >+# under the terms of either the GPL or the LGPL, and not to allow others to >+# use your version of this file under the terms of the MPL, indicate your >+# decision by deleting the provisions above and replace them with the notice >+# and other provisions required by the GPL or the LGPL. If you do not delete >+# the provisions above, a recipient may use your version of this file under >+# the terms of any one of the MPL, the GPL or the LGPL. >+# >+# ***** END LICENSE BLOCK ***** >+ >+ >+DEPTH = ../../.. >+topsrcdir = @top_srcdir@ >+srcdir = @srcdir@ >+VPATH = @srcdir@ >+ >+include $(DEPTH)/config/autoconf.mk >+ >+MODULE = dom >+XPIDL_MODULE = dom_apps >+GRE_MODULE = 1 >+ >+XPIDLSRCS = \ >+ nsIDOMApplicationRegistry.idl \ >+ $(NULL) >+ >+include $(topsrcdir)/config/rules.mk >diff --git a/dom/interfaces/apps/nsIDOMApplicationRegistry.idl b/dom/interfaces/apps/nsIDOMApplicationRegistry.idl >new file mode 100644 >--- /dev/null >+++ b/dom/interfaces/apps/nsIDOMApplicationRegistry.idl >@@ -0,0 +1,88 @@ >+/* ***** BEGIN LICENSE BLOCK ***** >+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1 >+ * >+ * The contents of this file are subject to the Mozilla Public License Version >+ * 1.1 (the "License"); you may not use this file except in compliance with >+ * the License. You may obtain a copy of the License at >+ * http://www.mozilla.org/MPL/ >+ * >+ * Software distributed under the License is distributed on an "AS IS" basis, >+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License >+ * for the specific language governing rights and limitations under the >+ * License. >+ * >+ * The Original Code is web apps. >+ * >+ * The Initial Developer of the Original Code is Mozilla Foundation >+ * Portions created by the Initial Developer are Copyright (C) 2008 >+ * the Initial Developer. All Rights Reserved. >+ * >+ * Contributor(s): >+ * Andreas Gal <gal@mozilla.com> (Original Author) >+ * >+ * Alternatively, the contents of this file may be used under the terms of >+ * either the GNU General Public License Version 2 or later (the "GPL"), or >+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"), >+ * in which case the provisions of the GPL or the LGPL are applicable instead >+ * of those above. If you wish to allow use of your version of this file only >+ * under the terms of either the GPL or the LGPL, and not to allow others to >+ * use your version of this file under the terms of the MPL, indicate your >+ * decision by deleting the provisions above and replace them with the notice >+ * and other provisions required by the GPL or the LGPL. If you do not delete >+ * the provisions above, a recipient may use your version of this file under >+ * the terms of any one of the MPL, the GPL or the LGPL. >+ * >+ * ***** END LICENSE BLOCK ***** */ >+ >+#include "domstubs.idl" >+ >+interface nsIApplication; >+ >+[scriptable, uuid(e0c271cb-266b-48c9-a7e4-96590b445c26)] >+interface nsIDOMApplicationRegistryError : nsISupports >+{ >+ const unsigned short DENIED = 1; >+ const unsigned short PERMISSION_DENIED = 2; >+ const unsigned short MANIFEST_URL_ERROR = 3; >+ const unsigned short NETWORK_ERROR = 4; >+ const unsigned short MANIFEST_PARSE_ERROR = 5; >+ const unsigned short INVALID_MANIFEST = 6; >+ >+ readonly attribute short code; >+}; >+ >+[scriptable, function, uuid(be170df5-9154-463b-9197-10a6195eba52)] >+interface nsIDOMApplicationRegistryEnumerateCallback : nsISupports >+{ >+ void handleEvent(in nsIArray applications); >+}; >+ >+[scriptable, function, uuid(ae0ed33d-35cf-443a-837b-a6cebf16bd49)] >+interface nsIDOMApplicationRegistryErrorCallback : nsISupports >+{ >+ void handleEvent(in nsIApplicationRegistryError error); >+}; >+ >+[scriptable, uuid(a6856a3d-dece-43ce-89b9-72dba07f4246)] >+interface nsIDOMApplication : nsISupports >+{ >+ readonly attribute DOMString manifest; >+ readonly attribute DOMString receipt; >+ readonly attribute DOMString origin; >+ readonly attribute DOMString installOrigin; >+ readonly attribute long installTime; >+}; >+ >+[scriptable, uuid(4070ea6f-dca1-4052-8bc6-7a9bcfc314ac)] >+interface nsIDOMApplicationRegistry : nsISupports >+{ >+ void install(in DOMString manifestUrl, >+ [optional] in DOMString receipt); >+ void uninstall(in DOMString origin); >+ void enumerate(in nsIDOMApplicationRegistryEnumerateCallback success, >+ [optional] in nsIDOMApplicationRegistryErrorCallback error); >+ >+ attribute nsIDOMEventListener oninstall; >+ attribute nsIDOMEventListener onuninstall; >+ attribute nsIDOMEventListener onerror; >+}

We probably still need the launch() method, since the launch url is built from the apps origin and the optional launch_path from the manifest. This is useful eg. if you want to serve different urls for different locales

Both urls are in the manifest. Why do we need a privileged launch method for that?

(In reply to Andreas Gal :gal from comment #20) > Both urls are in the manifest. Why do we need a privileged launch method for > that? For sure a web app can find the URL itself - but we may want to have a specific UA behavior when opening apps : using an app tab, focusing an already opened tab from the same origin for instance.

Ok now that's a better argument. I buy that. Add it back in.

What app is expected to use launch()? What does it return? In the case of our home screen / app launcher, we expect to get an iframe back, because we style it and otherwise manipulate it. I can't think of much benefit gained by the home screen using launch() vs. <iframe type="app"> directly, but I could be convinced. General unprivileged apps won't be able to use launch(); they can't directly query which apps are installed so they'd need to guess, which is bad. Instead, we expect they'll use an intents-like system for a level of indirection.

Any dashboard webapp should be able use enumerate() and launch() I think. And what the UA needs to do when calling launch() will be different on desktop/fennec/b2g. The b2g shell is a web based embedder, and as such it will need to implement various prompts (alerts, auth prompts) as well as manage opening windows and launching apps. So the b2g shell will provide the implementation of launch(), but not the dashboard app itself

Still not sure I totally buy that. The launcher wants to animate etc. launching. How would it do that? What does launch(X) return? How does the "window manager" (launcher / home screen so far) bring apps to the foreground or background? Which piece would implement an OS X Expose-style view? If launch(X) is called, and X is already running, what chooses whether to open a new instance of X or bring the existing one to the foreground? We could satisfy the b2g use cases by implementing launch() as an intent/action/activity that gets handled by the home screen. But since the home screen is the launcher, that seems like an unnecessary level of indirection. Maybe it's useful, for replacing the launcher. That implementation definitely makes sense in Firefox.

launch(X) doesn't return anything. Its behavior is UA dependent. The actual implementation needs to be provided by the "window manager" : in b2g this will be the launcher/home screen, on firefox desktop and fennec some component living in chrome. The patch in bug 697006 (desktop implementation) does this for launch() : It checks if an app from this origin is already open in an existing window : if yes, focus it. If no, open the app in a new tab and turn it into a pinned tab. Also, add the origin URL to the session store to reload the app properly on the next run. I understand that if this was only for b2g, things could be simpler - but we need some indirection to support several applications.

Also, b2g could not use the launch() api call if we don't want to implement it there, and just use the manifest to find the start url.

Sounds like we can leave it out for now and add it once the discussion has settled. Most apps will be started from some sort of app screen in the browser chrome anyway.

I'm sold on the b2g home screen calling launch() even though the home screen itself will implement launch(), almost always. But I agree about leaving it out for now: adding it for b2g (assuming launch() will be implemented by the b2g chrome package, and not generally shared, which would be even more work) would mean either taking a dependency on intents/actions/activities or adding another gross hard-coded hack tying the b2g home screen to the b2g chrome package.

Fabrice, you mentioned that you need 2 enumerate APIs. Can you elaborate and upload your current patch? Thanks.

Suggested manifest: { "version": "1.0", "name": "MozillaBall", "description": "Exciting Open Web development action!", "icons": { "16": "/img/icon-16.png", "48": "/img/icon-48.png", "128": "/img/icon-128.png" }, "developer": { "name": "Mozilla Labs", "url": "http://mozillalabs.com" }, "installs_allowed_from": [ "https://appstore.mozillalabs.com" ], "locales": { "es": { "description": "¡Acción abierta emocionante del desarrollo del Web!", "developer": { "url": "http://es.mozillalabs.com/" } }, "it": { "description": "Azione aperta emozionante di sviluppo di fotoricettore!", "developer": { "url": "http://it.mozillalabs.com/" } } }, "default_locale": "en" } Additional fields are allowed and ignored (but stored in the app registry).

meeting fabrice/mhanson/brendan/gal: google's work: http://code.google.com/p/chromium/issues/detail?id=61546 next steps: - prefix (maybe moz-) for vendor properties? (google's permission stuff) - allow data urls for icons, any domain is fine (absolute too) - coordinate with google & write spec

Implementation of the DOM API: A js xpcom component is instantiated for each page that uses n.mozApps. They all communicate with a js module that implement the repository. The js module itself sends observer notifications that the UI integration must use to add the UI bits.

Very simple UI using prompts(). Will move to proper dialogs and doorhangers. A test page is up at http://people.mozilla.com/~fdesre/openwebapps/test.html

Some notes: - implementation of enumerate() : 1) if the calling page has a webapps perm set, it gets the full list of apps. 2) if not and the call is an app, it gets itself 3) if not an app but a store, it gets all the apps installed from this store 4) if not an app or a store, and not authorized, we ask for permission. - there's some security/privacy issue with oninstall() and onuninstall() : just listening to theses events can give more information to a site than what it should get.

(In reply to Fabrice Desré [:fabrice] from comment #36) > Some notes: > - implementation of enumerate() : > 1) if the calling page has a webapps perm set, it gets the full list of > apps. > 2) if not and the call is an app, it gets itself > 3) if not an app but a store, it gets all the apps installed from this store > 4) if not an app or a store, and not authorized, we ask for permission. These seem like very different return values, based on permissions that the page itself does not have the ability to query. In cases like a page that is an app and a store, it's unclear what the result should be, and if a page expects but does not receive store permissions then it gets only its own record, not other records, nor does it seem to have any indication of what subset of apps it is receiving. Also amInstalled is a confirmation of whether the application is installed; if it is not installed then null is returned. With enumerate() an uninstalled app would get a permission dialog instead of this negative confirmation. The separate routines (amInstalled, getInstalledBy, mgmt.list) each indicate what value the page really expects, and the page either receives or does not receive the list that it expects.

I agree that enumerate() is still a bit tricky and the current code probably has flaws. What about doing: 1) if the calling page has a webapps perm set, it gets the full list of apps. 2) if not and the page is a store, return apps from the store, with itself if the store is also an app. 3) if not a store, but an app, return itself 4) if not an app or a store, and not authorized, we ask for permission. We also need to get the permission model ironed out. Currently we ask for permissions for: - setting on* handlers - uninstall() - enumerate() in some cases Should we have different permissions for each, or put them under a single "manage webapps" umbrella?

Isn't that list of enumerate() behavior identical to what you described in Comment 36? I also get the impression there is another API change alluded to but not fully described with on* handlers (that might replace watchUpdates?)

just food for thought: the owa jetpack currently does not make the mgmt api's generally available. They are only injected into our store or dashboard. An idea I had (this is not implemented) around this was additional stores or dashboards could be installed as apps, with the permission requested from the user to access those managment api's. Given that permission, the api's would then be injected for those.

Ian: The change suggested on enumerate() is that we first check if the calling page is a store and potentially an app. And yes, we replaced watchUpdates() by oninstall and onuninstall events. There's also onerror that is used by all calls but enumerate(). Shane: You went the easy way but just whitelisting a few known sites ;)

Removing the old navigator.mozApps API, that was only packaged in xul fennec. I have not removed the old fennec specific UI bits since they may be reused with the new API.

DOM API implementation. An xpcom component exposes the API to content, and uses the message manager to communicate with the repository implementation which is a JS module. The repository fires observer notifications for calls that need application specific UI action (install() and launch()). The following calls are only allowed for pages that have the "webapps-manage" permission set : mozApps.uninstall mozApps.oninstall mozApps.onuninstall mozApps.enumerate behaves differently depending on the permission. If it is set, it will return all apps, if not an installed app with get itself and a store apps installed from the store.

Comment on attachment 576528 [details] [diff] [review] Part 0 : remove old API The changes sounds fine to me.

Comment on attachment 576530 [details] [diff] [review] Part 1 : DOM API >diff --git a/dom/base/Webapps.js b/dom/base/Webapps.js >+ receiveMessage: function(aMessage) { >+ switch(aMessage.name) { nit: add a space after "switch" >diff --git a/dom/base/Webapps.jsm b/dom/base/Webapps.jsm >+let DOMApplicationRegistry = { >+ _writeFile: function ss_writeFile(aFile, aData) { >+ // Initialize the file output stream. >+ let ostream = Cc["@mozilla.org/network/safe-file-output-stream;1"].createInstance(Ci.nsIFileOutputStream); >+ ostream.init(aFile, 0x02 | 0x08 | 0x20, 0600, ostream.DEFER_OPEN); >+ >+ // Obtain a converter to convert our data to a UTF-8 encoded input stream. >+ let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].createInstance(Ci.nsIScriptableUnicodeConverter); >+ converter.charset = "UTF-8"; >+ >+ // Asynchronously copy the data to the file. >+ let istream = converter.convertToInputStream(aData); >+ NetUtil.asyncCopy(istream, ostream, function(rc) { >+ // nothing to do >+ }); >+ }, >+ confirmInstall: function(aData) { >+ let manFile = dir.clone(); >+ manFile.append("manifest.json"); >+ this._writeFile(manFile, JSON.stringify(app.manifest)); >+ >+ this.webapps[id] = app; >+ // add install time to the app object, and remove the manifest >+ this.webapps[id].installTime = (new Date()).getTime(); >+ delete this.webapps[id].manifest; >+ >+ this._writeFile(this.appsFile, JSON.stringify(this.webapps)); >+ >+ this.mm.sendAsyncMessage("Webapps:Install:Return:OK", aData); _writeFile will write out the files async. This code kinda uses the function in a sync way, but ti doesn't look like there would be a problem. On the other hand, you could pass a JS function to _writeFile and call that code in the NetUtil.asyncCopy callback, chaining the code together. >+ _readManifest: function(aId) { >+ let file = this.appsDir.clone(); >+ file.append(aId); >+ file.append("manifest.json"); >+ let data = ""; >+ let fstream = Cc["@mozilla.org/network/file-input-stream;1"].createInstance(Ci.nsIFileInputStream); >+ var cstream = Cc["@mozilla.org/intl/converter-input-stream;1"].createInstance(Ci.nsIConverterInputStream); >+ fstream.init(file, -1, 0, 0); >+ cstream.init(fstream, "UTF-8", 0, 0); >+ let (str = {}) { >+ let read = 0; >+ do { >+ read = cstream.readString(0xffffffff, str); // read as much as we can and put it in str.value >+ data += str.value; >+ } while (read != 0); >+ } >+ cstream.close(); // this closes fstream >+ try { >+ return JSON.parse(data); >+ } catch(e) { >+ return null; >+ } >+ }, FYI: This code does sync I/O, which could be converted to use NetUtil async methods. It would cause the enumerate and other methods to be refactored a bit. >+DOMApplicationManifest = function(aManifest, aOrigin) { >+ if (this._manifest.locales && this._manifest.locales[locale]) >+ this._localeRoot = this._manifest.locales[locale]; >+ else if (this._manifest.locales) { Use {} around the if block too, since the else block needs them. r+, but consider the sync I/O issue. You could fix now or file a followup. If you think async I/O isn't needed, comment that as well.

enumerate() replaces three existing documented functions: amInstalled, getInstalledBy, and mgmt.list. After some conversation on IRC, the primary motivation for this seems to be simplification. To record my own reaction from that discussion: I don't believe this is a simplification, or that it justifies changing the API. I don't see any real problem with combining getInstalledBy (intended to be called by stores) and mgmt.list (intended to be called by privileged dashboards); however, I don't believe that combining these with amInstalled is sensible. An application either wants to retrieve its own application record, or it wants a list of applications, these are two different use cases; that sometimes a page would receive many applications (based on permission, or any installation that happened on the same origin as the page), or would sometimes receive only its own record (if it does not have sufficient permission) will require the caller to program defensively. More realistically the caller won't be programming defensively and there will be peculiar results when they get back an unexpected result, like a dashboard that stubbornly only shows itself as an application. The documentation for the new enumerate() function also itself contains a kind of switch, a series of if-X-then-Y-is-returned statements, which I think is a sign that multiple distinct use cases are being jammed into one function; that does not seem like a simplification to me.

(In reply to Mark Finkle (:mfinkle) from comment #45) > Comment on attachment 576530 [details] [diff] [review] [diff] [details] [review] > _writeFile will write out the files async. This code kinda uses the function > in a sync way, but ti doesn't look like there would be a problem. On the > other hand, you could pass a JS function to _writeFile and call that code in > the NetUtil.asyncCopy callback, chaining the code together. Yep, good idea. I did this. > > FYI: This code does sync I/O, which could be converted to use NetUtil async > methods. It would cause the enumerate and other methods to be refactored a > bit. I'll file a followup on this one. That would be better indeed.

pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/23f26ba05f1a https://hg.mozilla.org/integration/mozilla-inbound/rev/17b4093b5ec5

Maybe we should rename launch_path to launchPath if we can convince google. Not super important I guess.

This needs a security review.

Backed out for various failures in browser-chrome tests https://tbpl.mozilla.org/php/getParsedLog.php?id=7616977&tree=Mozilla-Inbound TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_getshortcutoruri.js | Exception thrown at chrome://mochitests/content/browser/browser/base/content/test/browser_getshortcutoruri.js:120 - TypeError: Application.bookmarks is undefined Bug 646492 - Intermittent infinite loop of "ASSERTION: XPConnect is being called on a scope without a 'Components' property!" in browser/base/content/test/browser_getshortcutoruri.js winding up with "command timed out: 5400 seconds elapsed" TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_keywordBookmarklets.js | Exception thrown at chrome://mochitests/content/browser/browser/base/content/test/browser_keywordBookmarklets.js:7 - TypeError: Application.bookmarks is undefined TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/fuel/test/browser_Application.js | Check to see if an ID exists for the Application Bug 463595 - browser_Application.js uses timeouts and fails intermittently TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/fuel/test/browser_Application.js | Check to see if a name exists for the Application TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/fuel/test/browser_Application.js | Check to see if a version exists for the Application TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/fuel/test/browser_Application.js | Exception thrown at chrome://mochitests/content/browser/browser/fuel/test/browser_Application.js:70 - TypeError: Application.console is undefined TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/fuel/test/browser_ApplicationPrefs.js | Exception thrown at chrome://mochitests/content/browser/browser/fuel/test/browser_ApplicationPrefs.js:12 - TypeError: Application.prefs is undefined TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/fuel/test/browser_ApplicationQuitting.js | Exception thrown at chrome://mochitests/content/browser/browser/fuel/test/browser_ApplicationQuitting.js:12 - TypeError: Application.quit is not a function TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/fuel/test/browser_ApplicationStorage.js | Exception thrown at chrome://mochitests/content/browser/browser/fuel/test/browser_ApplicationStorage.js:3 - TypeError: Application.storage is undefined TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/fuel/test/browser_Bookmarks.js | Exception thrown at chrome://mochitests/content/browser/browser/fuel/test/browser_Bookmarks.js:10 - TypeError: Application.bookmarks is undefined TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/fuel/test/browser_Browser.js | Check access to browser windows Bug 458688 - FUEL test browser_Browser.js still times out occasionally Bug 511096 - SeaMonkey/SMILE port of FUEL Bug 458688 (occasional browser_Browser.js timeouts) TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/fuel/test/browser_Browser.js | Exception thrown at chrome://mochitests/content/browser/browser/fuel/test/browser_Browser.js:20 - TypeError: windows is undefined

I have a really hard time believing any of those failures are related. Fabrice?

(In reply to Andreas Gal :gal from comment #52) > I have a really hard time believing any of those failures are related. I'm checking locally, but it makes little sense to have broken FUEL with these patches.

So, it looks like the use of |nsIDOMApplication| as an interface name conflicts with something in Fuel. Renaming nsIDOMApplication to eg. nsIDOMApplication2 or nsIDOMApp makes the tests pass. That's very odd since there is no nsIDOMApplication in Fuel (there is fuelIApplication and extIApplication) so I wonder if we don't hit an xpidl bug here.

If you have interface nsIDOMFoo, Foo is exposed in the global window scope. So, the patch has probably made ("Application" in window) true, which sounds error prone. Please prefix all non-standard interfaces, nsIDOMMozApplication.

Thanks Olli, If I understand correctly, using nsIDOMMozApplication will make ("MozApplication" in window) true ? Since I don't want to expose anything into window, I will rename interfaces as mozIXXX instead. Any objections?

pushed again after switching to mozIXXX and running the tests successfully locally: https://hg.mozilla.org/integration/mozilla-inbound/rev/21ecdc2d0a6f https://hg.mozilla.org/integration/mozilla-inbound/rev/b9be5d6b2230

https://hg.mozilla.org/mozilla-central/rev/21ecdc2d0a6f https://hg.mozilla.org/mozilla-central/rev/b9be5d6b2230

dom/interfaces/apps/Makefile.in was created in the tree by this bug, but was not listed in toolkit/toolkit-makefiles.sh, so I've included it here: https://hg.mozilla.org/integration/mozilla-inbound/rev/17e8e969fa09

andreas can you be more specific about what exactly we should be sec-reviewing here?

mozApps has a privileged part and certain privacy leak opportunities, we should make sure that gets some review from the sec team

:dchan please do an impl review here

Janet and Mark wrote this up: https://developer.mozilla.org/en/DOM/window.navigator.mozApps Mentioned on Firefox 11 for developers.

(In reply to Eric Shepherd [:sheppy] from comment #63) > Janet and Mark wrote this up: > > https://developer.mozilla.org/en/DOM/window.navigator.mozApps > > Mentioned on Firefox 11 for developers. Aren't we duplicating documentation here? There's a MDN document about this already here - https://developer.mozilla.org/en/Apps/Apps_JavaScript_API. Can we make sure there's a single source of truth for this?

(In reply to Jason Smith [:jsmith] from comment #64) > Aren't we duplicating documentation here? There's a MDN document about this > already here - https://developer.mozilla.org/en/Apps/Apps_JavaScript_API. > Can we make sure there's a single source of truth for this? There is not an overlap here. I just put a link to https://developer.mozilla.org/en/DOM/window.navigator.mozApps on https://developer.mozilla.org/en/Apps/Apps_JavaScript_API. Also, recently Janet Swisher moved mozApps API docs like mozApps.install() https://developer.mozilla.org/en/DOM/Apps.install and mozApps.getSelf() etc. under the DOM docs where they really belong. Possibly some confusion on that until you get used to it.

The implementation seemed fine when I looked at it a couple months ago. There is a mgmt API that is only accessible to privileged/whitelisted pages. Other pages can only query for apps installed on its domain. The objects returned by the DOMApplicationRegistry are shallow copies of the actual app w/o the manifest and with a fake guid. This prevents a vulnerability on a store page from retrieving useful app data from the browser. Granted the store could query to see what is installed, but won't be able to steal receipts. updating the sec-review flag