We need that to properly expose that an app is privileged or certified.

Comment on attachment 650647 [details] [diff] [review] patch Review of attachment 650647 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/src/nsPrincipal.cpp @@ +1305,5 @@ > + } > + > + PRUint16 status = nsIPrincipal::APP_STATUS_INSTALLED; > + nsCOMPtr<mozIDOMApplication> domApp; > + appsService->GetAppByLocalId(mAppId, getter_AddRefs(domApp)); Why not adding GetAppStatusByLocalId() to nsIAppsService? I don't know if it's worth adding a lot of methods there or not... @@ +1315,5 @@ > + nsresult rv = app->GetAppStatus(&status); > + if (NS_FAILED(rv)) { > + return nsIPrincipal::APP_STATUS_NOT_INSTALLED; > + } > + return status; So, generally speaking, I don't know if I'm a big fan of that approach but for sure, I would prefer to have stuff cached so we don't waste time for each appStatus call. ::: dom/apps/src/Webapps.jsm @@ +699,5 @@ > let app = this._cloneAppObject(this.webapps[aId]); > return app; > }, > > + _cloneAppObjectWithPermissions: function(aApp) { Is there any reason to clone app without permisions? @@ +704,5 @@ > + let res = this._cloneAppObject(aApp); > + res.hasPermission = function(permission) { > + let localId = DOMApplicationRegistry.getAppLocalIdByManifestURL( > + this.manifestURL); > + let uri = Services.io.newURI(this.manifestURL, null, null); this.origin would be better. @@ +709,5 @@ > + let secMan = Cc["@mozilla.org/scriptsecuritymanager;1"] > + .getService(Ci.nsIScriptSecurityManager); > + // XXX for the purposes of permissions checking, this helper > + // should always be called on !isBrowser frames, so we > + // assume false here. I quite don't like that assumption. First of all, the sentence is misleading and you want to say "this helper should never been called with isBrowser=true". Always called with isBrowser=false doesn't mean it's not called when isBrowser=true. "Only", maybe. The main issue is that we are making assumptions based on current security model that might change. Better to have this being generic. ::: dom/interfaces/apps/mozIApplication.idl @@ +16,5 @@ > { > /* Return true if this app has |permission|. */ > boolean hasPermission(in string permission); > + > + /* Application status as defined in nsIPrincipal nit: remove the trailing whitespace and put a dot instead.

Oh, and I forgot to ask: how is |appStatus| set?

FWIW in all of my bugs, Mounir says we should use the permissions manager instead of this enum. I personally don't believe the permissions manager is sufficiently powerful, but there it is.

So it depends how we make the enum work. If we compare the principal's URI with the app's origin and make sure they are in the same origin, I'm okay with using appStatus. We should just make sure this is what we want to do. Right now, it's not doing that so we shouldn't use it.

> Right now, it's not doing that so we shouldn't use it. I see. I completely missed this in my patches. I understand now. Thanks!

(In reply to Mounir Lamouri (:mounir) from comment #1) > @@ +1315,5 @@ > > + nsresult rv = app->GetAppStatus(&status); > > + if (NS_FAILED(rv)) { > > + return nsIPrincipal::APP_STATUS_NOT_INSTALLED; > > + } > > + return status; > > So, generally speaking, I don't know if I'm a big fan of that approach but > for sure, I would prefer to have stuff cached so we don't waste time for > each appStatus call. I added a mHasAppStatus flag and store the value in mAppStatus. > ::: dom/apps/src/Webapps.jsm > @@ +699,5 @@ > > let app = this._cloneAppObject(this.webapps[aId]); > > return app; > > }, > > > > + _cloneAppObjectWithPermissions: function(aApp) { > > Is there any reason to clone app without permisions? This was just badly named, what we want here is create an object that implements mozIApplication. > @@ +709,5 @@ > > + let secMan = Cc["@mozilla.org/scriptsecuritymanager;1"] > > + .getService(Ci.nsIScriptSecurityManager); > > + // XXX for the purposes of permissions checking, this helper > > + // should always be called on !isBrowser frames, so we > > + // assume false here. > > I quite don't like that assumption. > First of all, the sentence is misleading and you want to say "this helper > should never been called with isBrowser=true". Always called with > isBrowser=false doesn't mean it's not called when isBrowser=true. "Only", > maybe. > The main issue is that we are making assumptions based on current security > model that might change. Better to have this being generic. I changed the comment, but I'm not sure what you're suggesting to fix in the code itself.

(In reply to Mounir Lamouri (:mounir) from comment #2) > Oh, and I forgot to ask: how is |appStatus| set? For now, all gaia apps are preloaded as CERTIFIED, and new installed apps are INSTALLED. When we'll get signature support for packaged apps we'll be able to have PRIVILEGED ones also.

I've been saying that this bug will make it so that if I have <iframe mozapp="http://app.com/manifest"> and the iframe's location is http://foo.com, that the frame's app status will be NO_APP. But it doesn't look like this code will do that. Is that correct? Do we have a separate bug for that?

Justin, I'm not sure which app status you're talking about. In this bug I'm dealing with privileges levels as defined here: https://mxr.mozilla.org/mozilla-central/source/caps/idl/nsIPrincipal.idl#231 NO_APP_ID is an appId, not a privilege level.

Sorry, my mistake for not being precise! What will the privilege level of http://foo.com inside the iframe from comment 8 be, before and after this patch?

(In reply to Justin Lebar [:jlebar] from comment #10) > Sorry, my mistake for not being precise! > > What will the privilege level of http://foo.com inside the iframe from > comment 8 be, before and after this patch? If the appId is NO_APP_ID, the privilege will always be APP_STATUS_NOT_INSTALLED, this patch doesn't change anything in this case.

Comment on attachment 651561 [details] [diff] [review] patch v2 Review of attachment 651561 [details] [diff] [review]: ----------------------------------------------------------------- In addition to the comments below, I'm worried about the performances here. For an OOP app, we will do an IPC call to get the app object. That might take some time. Ideally, I would like to have |mAppStatus| initialized at nsPrincipal::Init() but that performance issue might bit us. In general, the fact that the first call to nsPrincipal::GetAppStatus() will do a synchronous IPC call isn't a good thing and might create performance issues because callers will not be able to guess if the value is cached or not when they do the call. I wonder what could be the solutions. Maybe we could cache same DOM App Registry info into the child process? Like the info regarding the apps that are expected to be in the process? Or maybe only the info that were accessed once? ::: caps/src/nsPrincipal.cpp @@ +1301,5 @@ > // Installed apps have a valid app id (not NO_APP_ID or UNKNOWN_APP_ID) > // and they are not inside a mozbrowser. > + if (mAppId == nsIScriptSecurityManager::NO_APP_ID || > + mAppId == nsIScriptSecurityManager::UNKNOWN_APP_ID || mInMozBrowser) { > + return mAppStatus = nsIPrincipal::APP_STATUS_NOT_INSTALLED; That's not a very good practice... |return nsIPrincipal::APP_STATUS_NOT_INSTALLED;| should be enough. mAppStatus shouldn't be used. @@ +1303,5 @@ > + if (mAppId == nsIScriptSecurityManager::NO_APP_ID || > + mAppId == nsIScriptSecurityManager::UNKNOWN_APP_ID || mInMozBrowser) { > + return mAppStatus = nsIPrincipal::APP_STATUS_NOT_INSTALLED; > + } > + nit: trailing whitespaces. @@ +1306,5 @@ > + } > + > + nsCOMPtr<nsIAppsService> appsService = do_GetService(APPS_SERVICE_CONTRACTID); > + if (!appsService) { > + return mAppStatus = nsIPrincipal::APP_STATUS_NOT_INSTALLED; NS_ENSURE_TRUE(appsService, nsIPrincipal::APP_STATUS_NOT_INSTALLED); @@ +1314,5 @@ > + nsCOMPtr<mozIDOMApplication> domApp; > + appsService->GetAppByLocalId(mAppId, getter_AddRefs(domApp)); > + nsCOMPtr<mozIApplication> app = do_QueryInterface(domApp); > + if (!app) { > + return mAppStatus = nsIPrincipal::APP_STATUS_NOT_INSTALLED; NS_ENSURE_TRUE(app, nsIPrincipal::APP_STATUS_NOT_INSTALLED); @@ +1318,5 @@ > + return mAppStatus = nsIPrincipal::APP_STATUS_NOT_INSTALLED; > + } > + > + nsresult rv = app->GetAppStatus(&mAppStatus); > + if (NS_FAILED(rv)) { NS_ENSURE_SUCCESS(app->GetAppStatus(&mAppStatus), nsIPrincipal::APP_STATUS_NOT_INSTALLED) @@ +1323,5 @@ > + return mAppStatus = nsIPrincipal::APP_STATUS_NOT_INSTALLED; > + } > + > + mHasAppStatus = true; > + return mAppStatus; After setting mHasAppStatus to true, check if the app's origin and the principal origin are the same. If they are not, you just set |mAppStatus| to |APP_STATUS_NOT_INSTALLED|. ::: dom/apps/src/Webapps.jsm @@ +707,5 @@ > + let localId = DOMApplicationRegistry.getAppLocalIdByManifestURL( > + this.manifestURL); > + let uri = Services.io.newURI(this.origin, null, null); > + let secMan = Cc["@mozilla.org/scriptsecuritymanager;1"] > + .getService(Ci.nsIScriptSecurityManager); nit: indentation @@ +711,5 @@ > + .getService(Ci.nsIScriptSecurityManager); > + // XXX for the purposes of permissions checking, this helper > + // should never been called with isBrowser=true, so we > + // assume false here. > + let principal = secMan.getAppCodebasePrincipal(uri, localId, What worries me here is what if we call this helper with isBrowser=true... ::: dom/interfaces/apps/mozIApplication.idl @@ +20,5 @@ > + /* Application status as defined in nsIPrincipal. > + * Can be one of: > + * APP_STATUS_INSTALLED = 1; > + * APP_STATUS_PRIVILEGED = 2; > + * APP_STATUS_CERTIFIED = 3; Don't put that list here so we don't have to think about updating it when we change nsIPrincipal and we don't end up with misleading information.

(In reply to Fabrice Desré [:fabrice] from comment #11) > (In reply to Justin Lebar [:jlebar] from comment #10) > > What will the privilege level of http://foo.com inside the iframe from > > comment 8 be, before and after this patch? > > If the appId is NO_APP_ID, the privilege will always be > APP_STATUS_NOT_INSTALLED, this patch doesn't change anything in this case. That's still not my question; let me try again. Suppose I have <iframe mozapp="http://app.com/manifest" src="http://app.com">. The app-id of this frame is 1, and the app status is APP_STATUS_INSTALLED. Now suppose instead I have <iframe mozapp="http://app.com/manifest" src="http://foo.com">. You said earlier that the app-id does not change. What is the app status here?

Okay, on IRC Fabrice and I confirmed that the patch doesn't change current behavior with respect to comment 13. And I happen to know that current behavior is, the app status is APP_STATUS_INSTALLED. But I think matching the behavior I want (APP_STATUS_NOT_INSTALLED) is as simple as modifying nsPrincipal::GetAppStatus() so that if the location of the principal is not same origin as the app's origin, then we return NOT_INSTALLED. I volunteer to write this patch if Fabrice doesn't want to...

> I volunteer to write this patch if Fabrice doesn't want to... Sorry, that came out wrong. Let me try again: Fabrice, do you want to make this change in this bug? If not, I can do it in a follow-up. I'm itching to have that change landed one way or another, because until it is, I'm responsible for a security regression in the full-screen API...

Addressing comments, except the caching of webapps data in the content process.

I refactored AppsService e10s support to load a different jsm (AppsService.jsm) when running in a content process. Both AppsService.jsm and Webapps.jsm now share common code which is itself in AppsUtils.jsm. This last module can be safely used in any process.

Now with all the new .jsm files.

Fixing some test breakage.

Comment on attachment 652995 [details] [diff] [review] Part 1 : nsPrincipal hook up Review of attachment 652995 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/src/nsPrincipal.cpp @@ +1301,5 @@ > // Installed apps have a valid app id (not NO_APP_ID or UNKNOWN_APP_ID) > // and they are not inside a mozbrowser. > + if (mAppId == nsIScriptSecurityManager::NO_APP_ID || > + mAppId == nsIScriptSecurityManager::UNKNOWN_APP_ID || mInMozBrowser) { > + return nsIPrincipal::APP_STATUS_NOT_INSTALLED; You could even set |mHasAppStatus| and |mAppStatus|. @@ +1307,5 @@ > + > + nsCOMPtr<nsIAppsService> appsService = do_GetService(APPS_SERVICE_CONTRACTID); > + NS_ENSURE_TRUE(appsService, nsIPrincipal::APP_STATUS_NOT_INSTALLED); > + > + PRUint16 status = nsIPrincipal::APP_STATUS_INSTALLED; You don't seem to use this... @@ +1316,5 @@ > + > + NS_ENSURE_SUCCESS(app->GetAppStatus(&mAppStatus), nsIPrincipal::APP_STATUS_NOT_INSTALLED); > + > + mHasAppStatus = true; > + nsXPIDLCString origin; nsCAutoString would be good here. @@ +1318,5 @@ > + > + mHasAppStatus = true; > + nsXPIDLCString origin; > + nsresult rv = GetOrigin(getter_Copies(origin)); > + NS_ENSURE_SUCCESS(rv, mAppStatus); You should not return |mAppStatus| and keep |mHasAppStatus| to true if we fail to get the origin. |mHasAppStatus| should be set to true at the and of the method if everything was working and the return value in case of failure should be |APP_STATUS_NOT_INSTALLED|. @@ +1320,5 @@ > + nsXPIDLCString origin; > + nsresult rv = GetOrigin(getter_Copies(origin)); > + NS_ENSURE_SUCCESS(rv, mAppStatus); > + nsString appOrigin; > + NS_ENSURE_SUCCESS(app->GetOrigin(appOrigin), mAppStatus); ditto @@ +1323,5 @@ > + nsString appOrigin; > + NS_ENSURE_SUCCESS(app->GetOrigin(appOrigin), mAppStatus); > + > + if (!appOrigin.Equals(NS_ConvertASCIItoUTF16(origin))) { > + return nsIPrincipal::APP_STATUS_NOT_INSTALLED; nsPrincipal::GetOrigin() is going to return an ASCII origin which means the origin will be puny-encoded. Assuming the app's origin is in UTF16, it might be puny-encoded or not. Do we have rules regarding this? If we don't we should assume it's not puny-encoded and puny-encode it ourselves. What we could do is to take the origin, transform it as an nsIURI and call nsPrincipal::GetOriginForURI(). That may, both origins should be puny-encoded, hopefully. ::: dom/apps/src/Webapps.jsm @@ +82,5 @@ > this.webapps[id].localId = this._nextLocalId(); > } > + > + // Default to a non privileged status. > + if (this.webapps[id].appStatus !== undefined) { You mean " === undefined", don't you? @@ +296,5 @@ > receipts: aApp.receipts ? JSON.parse(JSON.stringify(aApp.receipts)) : null, > installTime: aApp.installTime, > manifestURL: aApp.manifestURL, > + appStatus: aApp.appStatus, > + localId: aApp.localId, Why not setting that in _cloneAsMozIApplication instead of here given that I presume _cloneAppObject() is for web content, isn't it? @@ +709,5 @@ > + _cloneAsMozIApplication: function(aApp) { > + let res = this._cloneAppObject(aApp); > + res.hasPermission = function(permission) { > + let localId = DOMApplicationRegistry.getAppLocalIdByManifestURL( > + this.manifestURL); No need for that, you can use |aApp.localId|. @@ +715,5 @@ > + let secMan = Cc["@mozilla.org/scriptsecuritymanager;1"] > + .getService(Ci.nsIScriptSecurityManager); > + // XXX for the purposes of permissions checking, this helper > + // should never been called with isBrowser=true, so we > + // assume false here. Could you rewrite this this way: // This helper checks an URI inside |aApp|'s origin and part of |aApp| has a // specific permission. It is not checking if browsers inside |aApp| have such // permission.

Comment on attachment 652978 [details] [diff] [review] Part 2: cache webapps in the content process Review of attachment 652978 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me with the comments below fixed. However, I would be more confortable if Vivien could have a look too. ::: dom/apps/src/AppsUtils.jsm @@ +20,5 @@ > + //dump("-*- AppsUtils.jsm: " + s + "\n"); > +} > + > +let AppsUtils = { > + // clones a app object, without the manifest nit: // Clones an app object, without the manifest. @@ +31,5 @@ > + manifestURL: aApp.manifestURL, > + appStatus: aApp.appStatus, > + localId: aApp.localId, > + progress: aApp.progress || 0.0, > + status: aApp.status || "installed" As said in part 1, if this is for the content, we shouldn't add values that aren't part of the DOM object. @@ +100,5 @@ > + debug("getManifestURLByLocalId " + aLocalId); > + for (let id in aApps) { > + let app = aApps[id]; > + if (app.localId == aLocalId) { > + debug("\t" + app.manifestURL); nit: maybe you can remove that? @@ +105,5 @@ > + return app.manifestURL; > + } > + } > + > + return null; return "";

Updating according to real-life conversation. The fields added in _cloneAppObject() are not exposed to content, but we need to persist them between restarts.

Comment on attachment 654443 [details] [diff] [review] Part 1 : nsPrincipal hook up Review of attachment 654443 [details] [diff] [review]: ----------------------------------------------------------------- r-, because you didn't apply some comments without any explanation. ::: caps/src/nsPrincipal.cpp @@ +1326,5 @@ > + NS_ENSURE_SUCCESS(app->GetOrigin(appOrigin), > + nsIPrincipal::APP_STATUS_NOT_INSTALLED); > + > + // We go from string -> nsIURI -> origin to be sure we > + // compare two punned origins. nit: punny-encoded @@ +1328,5 @@ > + > + // We go from string -> nsIURI -> origin to be sure we > + // compare two punned origins. > + nsCOMPtr<nsIURI> appURI; > + nsresult rv = NS_NewURI(getter_AddRefs(appURI), appOrigin); NS_ENSURE_SUCCESS(NS_NewURI(getter_AddRefs(appURI), appOrigin), nsIPrincipal::APP_STATUS_NOT_INSTALLED); (you are not using |rv| here...) @@ +1331,5 @@ > + nsCOMPtr<nsIURI> appURI; > + nsresult rv = NS_NewURI(getter_AddRefs(appURI), appOrigin); > + nsCAutoString appOriginPunned; > + rv = GetOriginForURI(appURI, getter_Copies(appOriginPunned)); > + NS_ENSURE_SUCCESS(rv, nsIPrincipal::APP_STATUS_NOT_INSTALLED); NS_ENSURE_SUCCESS again. ::: dom/apps/src/Webapps.jsm @@ +296,5 @@ > receipts: aApp.receipts ? JSON.parse(JSON.stringify(aApp.receipts)) : null, > installTime: aApp.installTime, > manifestURL: aApp.manifestURL, > + appStatus: aApp.appStatus, > + localId: aApp.localId, You didn't apply my comment here. Why? @@ +708,5 @@ > + _cloneAsMozIApplication: function(aApp) { > + let res = this._cloneAppObject(aApp); > + res.hasPermission = function(permission) { > + let localId = DOMApplicationRegistry.getAppLocalIdByManifestURL( > + this.manifestURL); ditto. @@ +714,5 @@ > + let secMan = Cc["@mozilla.org/scriptsecuritymanager;1"] > + .getService(Ci.nsIScriptSecurityManager); > + // XXX for the purposes of permissions checking, this helper > + // should never been called with isBrowser=true, so we > + // assume false here. ditto.

(In reply to Mounir Lamouri (:mounir) from comment #23) > ::: dom/apps/src/Webapps.jsm > @@ +296,5 @@ > > receipts: aApp.receipts ? JSON.parse(JSON.stringify(aApp.receipts)) : null, > > installTime: aApp.installTime, > > manifestURL: aApp.manifestURL, > > + appStatus: aApp.appStatus, > > + localId: aApp.localId, > > You didn't apply my comment here. Why? I did answer in comment #22 : we need that here, it's not exposed to content. > @@ +708,5 @@ > > + _cloneAsMozIApplication: function(aApp) { > > + let res = this._cloneAppObject(aApp); > > + res.hasPermission = function(permission) { > > + let localId = DOMApplicationRegistry.getAppLocalIdByManifestURL( > > + this.manifestURL); > > ditto. > > @@ +714,5 @@ > > + let secMan = Cc["@mozilla.org/scriptsecuritymanager;1"] > > + .getService(Ci.nsIScriptSecurityManager); > > + // XXX for the purposes of permissions checking, this helper > > + // should never been called with isBrowser=true, so we > > + // assume false here. > > ditto. These two are fixed in part 2, so I did not backport here.

(In reply to Fabrice Desré [:fabrice] from comment #24) > > @@ +714,5 @@ > > > + let secMan = Cc["@mozilla.org/scriptsecuritymanager;1"] > > > + .getService(Ci.nsIScriptSecurityManager); > > > + // XXX for the purposes of permissions checking, this helper > > > + // should never been called with isBrowser=true, so we > > > + // assume false here. > > > > ditto. > > These two are fixed in part 2, so I did not backport here. You did not fix the comment in part 2.

Fixes the latest comments.

ditto

Comment on attachment 655598 [details] [diff] [review] Part 1 : nsPrincipal hook up Review of attachment 655598 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/Webapps.jsm @@ +714,5 @@ > + let secMan = Cc["@mozilla.org/scriptsecuritymanager;1"] > + .getService(Ci.nsIScriptSecurityManager); > + // XXX for the purposes of permissions checking, this helper > + // should never been called with isBrowser=true, so we > + // assume false here. Please, make sure this is really fixed in part 2 locally.

Comment on attachment 655599 [details] [diff] [review] Part 2 : cache webapps in the content process Review of attachment 655599 [details] [diff] [review]: ----------------------------------------------------------------- Fast review, just checking that previous comments were applied. ::: dom/apps/src/AppsUtils.jsm @@ +20,5 @@ > + //dump("-*- AppsUtils.jsm: " + s + "\n"); > +} > + > +let AppsUtils = { > + // Clones a app object, without the manifest. nit: an app

Comment on attachment 655599 [details] [diff] [review] Part 2 : cache webapps in the content process Review of attachment 655599 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/AppsService.jsm @@ +28,5 @@ > + .QueryInterface(Ci.nsISyncMessageSender); > + > + ["Webapps:AddApp", "Webapps:RemoveApp"].forEach((function(aMsgName) { > + this.cpmm.addMessageListener(aMsgName, this); > + }).bind(this)); The Webapps:AddApp, Webapps:RemoveApp does not seems to match the messages sent. ::: dom/apps/src/AppsUtils.jsm @@ +21,5 @@ > +} > + > +let AppsUtils = { > + // Clones a app object, without the manifest. > + _cloneAppObject: function(aApp) { Since this is a new file let's take good practice and give a name to anonymous methods! @@ +33,5 @@ > + localId: aApp.localId, > + progress: aApp.progress || 0.0, > + status: aApp.status || "installed" > + }; > + return clone; nit: You don't really need to |let clone = ... | you can simply return. Not sure if it change anything. @@ +38,5 @@ > + }, > + > + _cloneAsMozIApplication: function(aApp) { > + let res = this._cloneAppObject(aApp); > + let localId = aApp.localId; No need for this variable, it can be use directly in the call to secMan.getAppCodebasePrincipal. @@ +42,5 @@ > + let localId = aApp.localId; > + res.hasPermission = function(aPermission) { > + let uri = Services.io.newURI(this.origin, null, null); > + let secMan = Cc["@mozilla.org/scriptsecuritymanager;1"] > + .getService(Ci.nsIScriptSecurityManager); Indentation is wrong. @@ +53,5 @@ > + aPermission); > + return (perm === Ci.nsIPermissionManager.ALLOW_ACTION); > + }; > + res.QueryInterface = XPCOMUtils.generateQI([Ci.mozIDOMApplication, > + Ci.mozIApplication]); Is there a cleaner way to do that? ::: dom/apps/src/Webapps.jsm @@ +314,5 @@ > // Override the origin with the correct id. > app.origin = "app://" + id; > } > > + let appObject = AppsUtils._cloneAppObject(app); Since you're using _cloneAppObject a lot it seems that you don't want to prefix it with a '_'. @@ +352,5 @@ > ppmm.sendAsyncMessage("Webapps:Install:Return:OK", aData); > Services.obs.notifyObservers(this, "webapps-sync-install", appNote); > + this.children.forEach(function(aMsgMgr) { > + aMsgMgr.sendAsyncMessage("Webapps.AddApp", { id: id, app: appObject }); > + }); Webapps.AddApp -> Webapp::AddApp @@ +574,5 @@ > ppmm.sendAsyncMessage("Webapps:Uninstall:Return:OK", aData); > Services.obs.notifyObservers(this, "webapps-sync-uninstall", appNote); > + this.children.forEach(function(aMsgMgr) { > + aMsgMgr.sendAsyncMessage("Webapps.RemoveApp", { id: id }); > + }); Webapps.RemoveApp -> Webapps::RemoveApp @@ +704,1 @@ > }, Do you really need those wrappers?

Addressing comments.

Comment on attachment 655684 [details] [diff] [review] Part 2 : cache webapps in the content process Review of attachment 655684 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/AppsService.js @@ +19,5 @@ > function AppsService() > { > debug("AppsService Constructor"); > + let inParent = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime) > + .processType == Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT; nit: indent @@ +22,5 @@ > + let inParent = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime) > + .processType == Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT; > + debug("inParent: " + inParent); > + Cu.import(inParent ? "resource://gre/modules/Webapps.jsm" : > + "resource://gre/modules/AppsService.jsm"); I found it hard during my review to remember which file is loaded into the content process and which one is loaded into the main process. What about making it obvious in the name of AppsServices.jsm? ::: dom/apps/src/AppsService.jsm @@ +24,5 @@ > + init: function init() { > + debug("init"); > + this.cpmm = Cc["@mozilla.org/childprocessmessagemanager;1"] > + .getService(Ci.nsIFrameMessageManager) > + .QueryInterface(Ci.nsISyncMessageSender); nit: indent

https://hg.mozilla.org/integration/mozilla-inbound/rev/010b5cb3576d https://hg.mozilla.org/integration/mozilla-inbound/rev/9b96029a9cf0

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/118cc431d56f - leaking in mochitest-2, failing in test_principal_extendedorigin_appid_appstatus.html.

Comment on attachment 655975 [details] [diff] [review] Part 3 : updating tests Review of attachment 655975 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the requested change ::: caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html @@ +291,2 @@ > is(principal.appStatus, Ci.nsIPrincipal.APP_STATUS_INSTALLED, > + 'this should be an installed app '); I would have prefered to have: if (data.isapp) { if (data.test.indexOf("in-app-not-installed") != -1) { is(principal.appStatus, Ci.nsIPrincipal.APP_STATUS_NOT_INSTALLED, 'this should not be an installed app'); } else { is(principal.appStatus, Ci.nsIPrincipal.APP_STATUS_INSTALLED, 'this should be an installed app'); } [...] }

Try push with the updated tests: https://tbpl.mozilla.org/?tree=Try&rev=b172d2e73ca9

Followup to prevent the leak on macos M2. Green try run at https://tbpl.mozilla.org/?tree=Try&rev=a13b812447cd

Comment on attachment 656317 [details] [diff] [review] Part 4: prevent leaks Not to be That Guy, but this code effectively means "ignore this leak" -- it doesn't change anything but whether we report the leak. Should this object and everything it points to actually be leaking until shutdown? (I tried to find it and see for myself, but this file must be created in some dependent patch.)

Justin, the file is created in Part 2. It's a js module loaded in content processes and that we have to keep around until the process dies.

Comment on attachment 656317 [details] [diff] [review] Part 4: prevent leaks Ah. You renamed the file from AppsService.jsm to AppsServiceChild.jsm. I see... r=me, but could you please add a comment somewhere indicating that the cpmm.addMessageListener causes the DOMApplicationRegistry object to live forever? (We've had problems in the past with people assuming permanent objects were transient.)

https://hg.mozilla.org/integration/mozilla-inbound/rev/9db0d3506b7e https://hg.mozilla.org/integration/mozilla-inbound/rev/a4d2747686d2 https://hg.mozilla.org/integration/mozilla-inbound/rev/45181e6a2711 https://hg.mozilla.org/integration/mozilla-inbound/rev/0633977cfcc3

Is this testable from an end-user perspective? It looks like there's some changes to expose the app type in the app object in this patch.

https://hg.mozilla.org/mozilla-central/rev/0633977cfcc3 https://hg.mozilla.org/mozilla-central/rev/45181e6a2711 https://hg.mozilla.org/mozilla-central/rev/a4d2747686d2 https://hg.mozilla.org/mozilla-central/rev/9db0d3506b7e