Closed Bug 1178533 Opened 9 years ago Closed 9 years ago

Register permissions, system messages etc on navigation to signed packages

Categories

(Core Graveyard :: DOM: Apps, defect, P1)

defect

Tracking

(blocking-b2g:2.5+, firefox44 fixed)

RESOLVED FIXED
mozilla44
blocking-b2g 2.5+
Tracking Status
firefox44 --- fixed

People

(Reporter: pauljt, Assigned: arroway)

References

Details

Attachments

(1 file, 19 obsolete files)

35.99 KB, patch
Details | Diff | Splinter Review
When we navigate to signed packages, this is the equivalent to installing in the current model. We need to do all of the install registration at navigation time instead. This includes:
 - registering permissions
 - registering system messages

This does not include Web Activity registration, which is planned to be done at time the content is Pinned.

As part of this bug, we also need to add code to keep these registrations updated. This could be removal on cache eviction, or perhaps just ensuring they are updated on each navigation.
PS I'm not sure if DOM:apps makes sense here - does this belong more in core:networking? This is going to be a FxOS only thing so I'm not sure.
Assignee: nobody → stephouillon
Priority: -- → P1
blocking-b2g: --- → 2.5+
(In reply to Paul Theriault [:pauljt] from comment #0)
> When we navigate to signed packages, this is the equivalent to installing in
> the current model. We need to do all of the install registration at
> navigation time instead. This includes:
>  - registering permissions
>  - registering system messages
> 
> This does not include Web Activity registration, which is planned to be done
> at time the content is Pinned.
> 
> As part of this bug, we also need to add code to keep these registrations
> updated. This could be removal on cache eviction, or perhaps just ensuring
> they are updated on each navigation.

I propose to create a module to deal with all the "app installation" things
like permissions and system messages. Other than that, it can also set the 
proper origin to this app, just what Bug 1178526 is intended to do.

Following are the reasons:

1) Origin set up is (kind of) a part of installation.

2) The entire manifest is supposed to pass to the install function so it's natural and convenient to get the origin, whereas it's hard and not transparent for necko to do that.

Waiting for Stephanie back and have a discussion then.
Hi Stéphanie,

Just forget about the 'origin' stuff I mentioned in comment 2. We might only need to deal with the permission registration and the system message things. So will you create some XPCOM like what I propose or you have other thoughts?

Probably I will call your functions somewhere like [1]. Thanks!

[1] https://github.com/elefant/gecko-dev/commit/4c7adf22035e999339a5603c048d1f637afac865#diff-f6a3710a33233135e9cc56c1a50c8269R774
Flags: needinfo?(stephouillon)
Hi Henry,
thanks for the heads-up, yesterday I was catching up with the nsec mailing list I was not following before and saw the emails were you and others were talking about that. Indeed, I think that I'll go with what you proposed (with a js xpcom) and come back soon with a first work-in-progress patch.
Thanks!
Flags: needinfo?(stephouillon)
This patch implements an installPackagedApp() method to register permissions.

There are two points at least yet to be solved regarding registering permissions:

1) getting the correct origin for a signed package via the package-identifier field in the manifest (I don't know if it's ready to be used yet? I tested the code by navigating to [1]).

2) generating a correct localId for the installed app (right now, it's a harcoded value to make the poc work). I'm not quite sure of the way it should be done since in the case of an installed package, it's related to the part of the code involving DOMApplicationRegistry (should I use the same way to generate an id via makeAppId() and use the same list of ids?)

This patch is integrated with hchang's work on this branch [2].

@Henry: 
The following patches:
2.a-Fixing-permissions-install-and-adding-return-status.patch 
and 
2.b-Add-return-status-when-calling-InstallPackagedWebapp.patch

can be applied on your repository [3] to get the same code, + some modifications in the PackagedAppService.* files.

[1] http://people.mozilla.org/~fdesre/packages/loqui.pak!//index.html
[2] https://github.com/arroway/gecko-dev/commits/Bug1178533-permission
[3] https://github.com/elefant/gecko-dev/commits/Bug1178533-permission
Flags: needinfo?(hchang)
Attachment #8656083 - Flags: feedback?(fabrice)
Ah, and the tests are useless so far apart from testing the syntax when running, I forgot to remove the code completely.
Comment on attachment 8656083 [details] [diff] [review]
bug-1178533-Add-nsIInstallPackagedWebapp-for-registe.patch

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

::: dom/apps/PermissionsInstaller.jsm
@@ +42,4 @@
>     *        A function called if an error occurs
>     * @returns void
>     **/
> +  

you have some trailing whitespace issues in this patch.

@@ +223,5 @@
>        value: aPermValue,
> +      browserFlag: false,
> +      kind: aKind,
> +      //TODO: hack for poc. How to generate correct localId?
> +      localId: "13371337",

I'm not sure if all the http packages will share the same id or not.

@@ +224,5 @@
> +      browserFlag: false,
> +      kind: aKind,
> +      //TODO: hack for poc. How to generate correct localId?
> +      localId: "13371337",
> +      isCachedPackagedWebapp: true,

that's can't be `true` all the time, right?

::: dom/newapps/InstallPackagedWebapp.js
@@ +21,5 @@
> +  classDescription: "InstallPackagedWebapp JavaScript XPCOM Component",
> +  classID:          Components.ID("{5cc6554a-5421-4a5e-b8c2-c62e8b7f4f3f}"),
> +  contractID:       "@mozilla.org/newapps/installpackagedwebapp;1",
> +
> +  QueryInterface: XPCOMUtils.generateQI([Components.interfaces.nsIInstallPackagedWebapp]),

nit: s/Components.interfaces/Ci

@@ +22,5 @@
> +  classID:          Components.ID("{5cc6554a-5421-4a5e-b8c2-c62e8b7f4f3f}"),
> +  contractID:       "@mozilla.org/newapps/installpackagedwebapp;1",
> +
> +  QueryInterface: XPCOMUtils.generateQI([Components.interfaces.nsIInstallPackagedWebapp]),
> +  

trailing whitespace

@@ +36,5 @@
> +   **/
> +
> +  installPackagedWebapp: function(manifestContent, aOrigin, aManifestURL) {
> +
> +    var aManifest = JSON.parse(manifestContent);

s/var/let

@@ +37,5 @@
> +
> +  installPackagedWebapp: function(manifestContent, aOrigin, aManifestURL) {
> +
> +    var aManifest = JSON.parse(manifestContent);
> + 

whitespace

@@ +46,5 @@
> +      manifest: aManifest,
> +      manifestURL: aManifestURL,
> +      origin: aOrigin,
> +      isPreinstalled: false,
> +      kind: "" //empty if not trusted hosted app

let's remove that.

@@ +49,5 @@
> +      isPreinstalled: false,
> +      kind: "" //empty if not trusted hosted app
> +    }, false, function() {
> +      debug("Error installing permissions for " + aOrigin);
> +      return false;

here only your inner function returns `false`, not installPackagedWebapp. I think you'll have to make it all async...

::: dom/permission/PermissionSettings.jsm
@@ +72,5 @@
> +
> +    let app;
> +    // Test if app is cached (signed streamable package) or installed via DOMApplicationRegistry
> +    if (aData.isCachedPackagedWebapp) {
> +      app = {localId: aData.localId, kind: aData.kind};

hm, `kind` is useless here. That's a leftover from THA that we should clean up (http://mxr.mozilla.org/mozilla-central/source/dom/apps/PermissionsTable.jsm#644 doesn't use it anymore).
Attachment #8656083 - Flags: feedback?(fabrice)
(In reply to Stephanie Ouillon [:arroway] from comment #5)
> Created attachment 8656083 [details] [diff] [review]
> bug-1178533-Add-nsIInstallPackagedWebapp-for-registe.patch
> 
> This patch implements an installPackagedApp() method to register permissions.
> 
> There are two points at least yet to be solved regarding registering
> permissions:
> 
> 1) getting the correct origin for a signed package via the
> package-identifier field in the manifest (I don't know if it's ready to be
> used yet? I tested the code by navigating to [1]).
> 

Not yet :( We are investigating Bug 1178526. There's already some progress
on it. 

> 2) generating a correct localId for the installed app (right now, it's a
> harcoded value to make the poc work). I'm not quite sure of the way it
> should be done since in the case of an installed package, it's related to
> the part of the code involving DOMApplicationRegistry (should I use the same
> way to generate an id via makeAppId() and use the same list of ids?)
> 

The app id will be obtained by the loadcontext. If you navigate the page
from browser app, the app id would be the browser app's app id. My understanding is
in the long run the use of app id will be obsoleted but not for now (at least 2.5)

> This patch is integrated with hchang's work on this branch [2].
> 
> @Henry: 
> The following patches:
> 2.a-Fixing-permissions-install-and-adding-return-status.patch 
> and 
> 2.b-Add-return-status-when-calling-InstallPackagedWebapp.patch
> 
> can be applied on your repository [3] to get the same code, + some
> modifications in the PackagedAppService.* files.
> 
> [1] http://people.mozilla.org/~fdesre/packages/loqui.pak!//index.html
> [2] https://github.com/arroway/gecko-dev/commits/Bug1178533-permission
> [3] https://github.com/elefant/gecko-dev/commits/Bug1178533-permission

Great! I'll take it a look and give it a test. Thanks :)
Flags: needinfo?(hchang)
Attachment #8656083 - Attachment is obsolete: true
Attachment #8656700 - Flags: feedback?(fabrice)
Thanks for the review Fabrice, I tried to adress your comments in path v2 (the diff with the previous patch is 3.a).
See my comments below:

(In reply to [:fabrice] Fabrice Desré from comment #9)
> Comment on attachment 8656083 [details] [diff] [review]
> bug-1178533-Add-nsIInstallPackagedWebapp-for-registe.patch
> 
> Review of attachment 8656083 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/PermissionsInstaller.jsm
> @@ +42,4 @@
> >     *        A function called if an error occurs
> >     * @returns void
> >     **/
> > +  
> 
> you have some trailing whitespace issues in this patch.

Should be fixed now.

> 
> @@ +223,5 @@
> >        value: aPermValue,
> > +      browserFlag: false,
> > +      kind: aKind,
> > +      //TODO: hack for poc. How to generate correct localId?
> > +      localId: "13371337",
> 
> I'm not sure if all the http packages will share the same id or not.

Based on Henry's comment, I now get the appId from the loadcontext. Concretely, I'm now getting it as an argument of installPackagedWebapp().
I need to see with Henry if my implementation is correct (see patch 3.b).

> 
> @@ +224,5 @@
> > +      browserFlag: false,
> > +      kind: aKind,
> > +      //TODO: hack for poc. How to generate correct localId?
> > +      localId: "13371337",
> > +      isCachedPackagedWebapp: true,
> 
> that's can't be `true` all the time, right?

Right, now I'm passing it as an argument from installPackagedWebapp when calling PermissionsInstaller.addPermission().

> 
> ::: dom/newapps/InstallPackagedWebapp.js
> @@ +21,5 @@
> > +  classDescription: "InstallPackagedWebapp JavaScript XPCOM Component",
> > +  classID:          Components.ID("{5cc6554a-5421-4a5e-b8c2-c62e8b7f4f3f}"),
> > +  contractID:       "@mozilla.org/newapps/installpackagedwebapp;1",
> > +
> > +  QueryInterface: XPCOMUtils.generateQI([Components.interfaces.nsIInstallPackagedWebapp]),
> 
> nit: s/Components.interfaces/Ci

fixed

> 
> @@ +22,5 @@
> > +  classID:          Components.ID("{5cc6554a-5421-4a5e-b8c2-c62e8b7f4f3f}"),
> > +  contractID:       "@mozilla.org/newapps/installpackagedwebapp;1",
> > +
> > +  QueryInterface: XPCOMUtils.generateQI([Components.interfaces.nsIInstallPackagedWebapp]),
> > +  
> 
> trailing whitespace

fixed

> 
> @@ +36,5 @@
> > +   **/
> > +
> > +  installPackagedWebapp: function(manifestContent, aOrigin, aManifestURL) {
> > +
> > +    var aManifest = JSON.parse(manifestContent);
> 
> s/var/let

fixed

> 
> @@ +37,5 @@
> > +
> > +  installPackagedWebapp: function(manifestContent, aOrigin, aManifestURL) {
> > +
> > +    var aManifest = JSON.parse(manifestContent);
> > + 
> 
> whitespace

fixed

> 
> @@ +46,5 @@
> > +      manifest: aManifest,
> > +      manifestURL: aManifestURL,
> > +      origin: aOrigin,
> > +      isPreinstalled: false,
> > +      kind: "" //empty if not trusted hosted app
> 
> let's remove that.

done

> 
> @@ +49,5 @@
> > +      isPreinstalled: false,
> > +      kind: "" //empty if not trusted hosted app
> > +    }, false, function() {
> > +      debug("Error installing permissions for " + aOrigin);
> > +      return false;
> 
> here only your inner function returns `false`, not installPackagedWebapp. I
> think you'll have to make it all async...

I modified this part by throwing an exception instead.

> 
> ::: dom/permission/PermissionSettings.jsm
> @@ +72,5 @@
> > +
> > +    let app;
> > +    // Test if app is cached (signed streamable package) or installed via DOMApplicationRegistry
> > +    if (aData.isCachedPackagedWebapp) {
> > +      app = {localId: aData.localId, kind: aData.kind};
> 
> hm, `kind` is useless here. That's a leftover from THA that we should clean
> up
> (http://mxr.mozilla.org/mozilla-central/source/dom/apps/PermissionsTable.
> jsm#644 doesn't use it anymore).

I've just removed it from my code, should I open a new bug and fix this in a different patch? (it sounds better than cleaning ii in this bug).
Comment on attachment 8656700 [details] [diff] [review]
v2-bug-1178533-Add-nsIInstallPackagedWebapp-for-registe.patch

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

::: dom/apps/PermissionsInstaller.jsm
@@ +42,4 @@
>     *        A function called if an error occurs
>     * @returns void
>     **/
> +

undo this change

@@ +217,5 @@
> +                                         aPermValue,
> +                                         aOrigin,
> +                                         aManifestURL,
> +                                         aAppId,
> +                                         aIsCachedPackage) {

Why don't you keep the same signature and just update the list of used properties?

::: dom/newapps/InstallPackagedWebapp.js
@@ +36,5 @@
> +   *        The app id
> +   * @returns boolean
> +   **/
> +
> +  installPackagedWebapp: function(manifestContent, aOrigin, aManifestURL, aAppId) {

nit: s/manifestContent/aManifestContent

@@ +39,5 @@
> +
> +  installPackagedWebapp: function(manifestContent, aOrigin, aManifestURL, aAppId) {
> +
> +    try {
> +      var isSuccess = true;

s/var/let

@@ +40,5 @@
> +  installPackagedWebapp: function(manifestContent, aOrigin, aManifestURL, aAppId) {
> +
> +    try {
> +      var isSuccess = true;
> +      let aManifest = JSON.parse(manifestContent);

let manifest = ...

@@ +42,5 @@
> +    try {
> +      var isSuccess = true;
> +      let aManifest = JSON.parse(manifestContent);
> +      //TODO: get package identifier from the manifest to build
> +      //the signed packaged origin.

nit: space after //

@@ +52,5 @@
> +        appId: aAppId,
> +        isPreinstalled: false,
> +        isCachedPackage: true
> +      }, false, function() {
> +        throw "Error installing permissions in PermissionsInstaller";

that will throw *after* this method returns, so is that useful compared to just Cu.reportError(...)?

@@ +61,5 @@
> +      return isSuccess;
> +    }
> +    catch(ex) {
> +      Cu.reportError(ex);
> +      return !isSuccess;

you always want to return false here, right?

::: dom/permission/PermissionSettings.jsm
@@ +104,4 @@
>      }
>  
>      if (aAllowAllChanges ||
> +        this._isChangeAllowed(principal, aData.type, aData.value)) {

I guess this change should be in another patch ;)
Attachment #8656700 - Flags: feedback?(fabrice)
Attachment #8656084 - Attachment is obsolete: true
Attachment #8656086 - Attachment is obsolete: true
Attachment #8656700 - Attachment is obsolete: true
Attachment #8656701 - Attachment is obsolete: true
Attachment #8656702 - Attachment is obsolete: true
Attachment #8661313 - Flags: review?(fabrice)
Comment on attachment 8661314 [details] [diff] [review]
2.Register-permissions-to-signed-packages.patch

Hello Valentin, I saw you reviewed Henry's work for bug 1178525, this patch is adding some code in PackagedAppService. Can I give it to you to review or should I ask somebody else? Thx!
Attachment #8661314 - Flags: review?(valentin.gosu)
Henry, 
would you have any idea who I could ask for a review for the modifications you made in caps/nsIScriptSecurityManager (Bobby Holley maybe as I saw you requested in bug 1163254?)? Thx!
Flags: needinfo?(hchang)
@fabrice: the new version of the patch is hopefully addressing your previous comments, as well as fixing some issues such as loading the module in b2g (this happens to be useful sometimes...).
There is currently a bug related to parent side permissions checking (cf email on the new security list), waiting to be fixed with bug 1191740.
Comment on attachment 8661314 [details] [diff] [review]
2.Register-permissions-to-signed-packages.patch

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

r=me with these few nits.

::: netwerk/protocol/http/PackagedAppService.cpp
@@ +862,5 @@
>    LOG(("Install this packaged app."));
> +  bool value = false;
> +  bool *isSuccess = &value;
> +
> +  nsCOMPtr<nsIInstallPackagedWebapp> installer = do_GetService("@mozilla.org/newapps/installpackagedwebapp;1");

Try to keep it under 80 chars per line if possible.

@@ +881,5 @@
> +  installer->InstallPackagedWebapp(mManifestContent.get(),
> +                                   packageOrigin.get(),
> +                                   manifestURL.get(),
> +                                   mAppId,
> +                                   isSuccess);

Can't you pass &value here, and avoid the extra variable?
Attachment #8661314 - Flags: review?(valentin.gosu) → review+
Comment on attachment 8661313 [details] [diff] [review]
1.bug-1178533-Add-nsIInstallPackagedWebapp-for-registe.patch

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

If you choose to use nsIScriptSecurityManager.createCodebasePrincipalFromOrigin for signed package in PermissionSettings.jsm, then you don't need to pass the appId/inBrowser in. The origin you pass in already contain all of those information (assume you use GetOrigin in the patch part 2) and you can get them after you create the principal based on the origin.

::: dom/apps/PermissionsInstaller.jsm
@@ +201,4 @@
>        origin: aApp.origin,
>        manifestURL: aApp.manifestURL,
>        value: aPermValue,
> +      browserFlag: false || aApp.browserFlag,

Same here. We actually don't need the browserFlag here according to your design in PermissionSettings.jsm::_internalAddPermission.

::: dom/newapps/InstallPackagedWebapp.js
@@ +50,5 @@
> +        manifest: manifest,
> +        manifestURL: aManifestURL,
> +        origin: aOrigin,
> +        localId: aAppId,
> +        browserFlag: true,

I think we don't need localId and browserFlag here since they will not be used if isCachedPackage is true.

::: dom/permission/PermissionSettings.jsm
@@ +72,5 @@
> +    let principal;
> +    // Test if app is cached (signed streamable package) or installed via DOMApplicationRegistry
> +    if (aData.isCachedPackage) {
> +      // If the app is from packaged web app, the origin includes origin attributes already.
> +      app = {localId: aData.localId};

We can obtain the appId from the principal that we are gonna create based on origin below.
(In reply to Stephanie Ouillon [:arroway] from comment #20)
> Henry, 
> would you have any idea who I could ask for a review for the modifications
> you made in caps/nsIScriptSecurityManager (Bobby Holley maybe as I saw you
> requested in bug 1163254?)? Thx!

Yes I think Bobby Holley is very suitable for reviewing the change :)
Flags: needinfo?(hchang)
(In reply to Valentin Gosu [:valentin] from comment #22)
> Comment on attachment 8661314 [details] [diff] [review]
> 2.Register-permissions-to-signed-packages.patch

Also, I would love to see some tests for this functionality.
See netwerk/test/unit/test_packaged_app_*.js and netwerk/test/mochitest/test_web_packaged_app.html
Attachment #8661313 - Attachment is obsolete: true
Attachment #8661313 - Flags: review?(fabrice)
Attachment #8661836 - Flags: review?(fabrice)
Hello Bobby, could you have a look at this patch? 
It defines a method to create the principal from an origin passed from netwerk/protocol/http/PackagedAppService.cpp (see patch 0002), and is called in dom/permission/PermissionSettings.jsm (see patch 0001).
Thx!
Attachment #8661316 - Attachment is obsolete: true
Attachment #8661845 - Flags: review?(bobbyholley)
Comment on attachment 8661837 [details] [diff] [review]
0002-Bug-1178533-Register-permissions-to-signed-packages-.patch r=valentin

Valentin, thanks for the review, I addressed your comments.
Not sure if I need to r? you again on this new patch, but doing it anyway to be sure.
Attachment #8661837 - Flags: review?(valentin.gosu)
Henry, I addressed your comments, it should look much better that way. Thx for the review!
Flags: needinfo?(hchang)
Comment on attachment 8661845 [details] [diff] [review]
0003-Bug-1178533-Use-origin-to-create-the-principal-which.patch

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

Thanks for cleaning up this API gap!

Please move the guts of this method into a new overload over BasePrincipal::CreateCodebasePrincipal which takes an nsACString.

Also, please remove principalFromOrigin from BrowserUtils.jsm and fix up the consumers. It would also be good to move the checks for "[" and "moz-nullprincipal" into nsScriptSecurityManager::CreateCodebasePrincipalsFromOrigin, and assert against those cases in BasePrincipal::CreateCodebasePrincipla.
Attachment #8661845 - Flags: review?(bobbyholley) → review-
Comment on attachment 8661837 [details] [diff] [review]
0002-Bug-1178533-Register-permissions-to-signed-packages-.patch r=valentin

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

(In reply to Stephanie Ouillon [:arroway] from comment #29)
> Comment on attachment 8661837 [details] [diff] [review]
> 0002-Bug-1178533-Register-permissions-to-signed-packages-.patch r=valentin
> 
> Valentin, thanks for the review, I addressed your comments.
> Not sure if I need to r? you again on this new patch, but doing it anyway to
> be sure.

Thanks! After an r+ you needn't ask for another review if changes are trivial.
Attachment #8661837 - Flags: review?(valentin.gosu) → review+
(In reply to Bobby Holley (:bholley) from comment #31)
> Comment on attachment 8661845 [details] [diff] [review]
> 0003-Bug-1178533-Use-origin-to-create-the-principal-which.patch
> 
> Review of attachment 8661845 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for cleaning up this API gap!
> 
> Please move the guts of this method into a new overload over
> BasePrincipal::CreateCodebasePrincipal which takes an nsACString.
> 
> Also, please remove principalFromOrigin from BrowserUtils.jsm and fix up the
> consumers. It would also be good to move the checks for "[" and
> "moz-nullprincipal" into
> nsScriptSecurityManager::CreateCodebasePrincipalsFromOrigin, and assert
> against those cases in BasePrincipal::CreateCodebasePrincipla.

Thanks for the review, Bobby!

Hi Stephanie, 

Just let me know if you need help addressing these comments :)
Flags: needinfo?(hchang) → needinfo?(stephouillon)
Got it, thanks ;)
Flags: needinfo?(stephouillon)
In response to the question on IRC - comment 31 is talking about adding an overload on the C++-only BasePrincipal class, not on nsIScriptSecurityManager. See elsewhere how all the JS-facing APIs funnel into BasePrincipal::CreateCodebasePrincipal.
Comment on attachment 8661836 [details] [diff] [review]
0001-Bug-1178533-Add-nsIInstallPackagedWebapp-for-registe.patch

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

f+ because I really want to see full tests checking that permissions are set up properly.

::: dom/apps/PermissionsInstaller.jsm
@@ +192,4 @@
>     *        The permission value.
>     * @param object aApp
>     *        The just-installed app configuration.
> +   *        The properties used are manifestURL, origin, appId, isCachedPackage

nit: add full stop at the end of sentence.

::: dom/newapps/InstallPackagedWebapp.js
@@ +23,5 @@
> +  classInfo: XPCOMUtils.generateCI({
> +    classID:          Components.ID("{5cc6554a-5421-4a5e-b8c2-c62e8b7f4f3f}"),
> +    classDescription: "InstallPackagedWebapp JavaScript XPCOM Component",
> +    interfaces: [Ci.nsIInstallPackagedWebapp]
> +  }),

you don't need classInfo

@@ +51,5 @@
> +        isPreinstalled: false,
> +        isCachedPackage: true
> +      }, false);
> +
> +      // TODO: register app handlers (system msg)

Can you file a bug for that and add the bug number here?

::: dom/newapps/test/xpcshell/test_install.js
@@ +30,5 @@
> +  aOrigin = "http://test.com";
> +  aManifestURL = "http://test.com/manifest.json";
> +  manifestString = JSON.stringify(manifest);
> +  res = mod.installPackagedWebapp(manifestString, aOrigin, aManifestURL, appId);
> +  equal(res, true);

You're not really testing that we set permissions here, just that nothing is blowing up ;)

::: dom/permission/PermissionSettings.jsm
@@ +69,5 @@
>    _internalAddPermission: function _internalAddPermission(aData, aAllowAllChanges, aCallbacks) {
>      // TODO: Bug 1196644 - Add signPKg parameter into PermissionSettings.jsm
> +    let app;
> +    let principal;
> +    // Test if app is cached (signed streamable package) or installed via DOMApplicationRegistry

nit: full stop at end of comment.
Attachment #8661836 - Flags: review?(fabrice) → feedback+
See Also: → 1206059
Thanks for the review Fabrice. With this new patch, I adressed your comments and wrote some (more useful) xpcshell unit tests. 
To check the permissions, I adapted getPermission and removepermission in PermissionSettings to support how we're using the origin to get the principal and the isCachedPackage flag.
Attachment #8661836 - Attachment is obsolete: true
Attachment #8663077 - Flags: review?(fabrice)
Hi Bobby,
I refactored the code according to your comments (thanks for your precision about BasePrincipal btw :) and I removed the principalFromOrigin function in BrowserUtils.jsm.
Attachment #8661845 - Attachment is obsolete: true
Attachment #8663078 - Flags: review?(bobbyholley)
Comment on attachment 8663078 [details] [diff] [review]
0003-Bug-1178533-Use-origin-to-create-the-principal-which.patch

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

Looks great. Please check which of the JS files starting importing BrowserUtils.jsm in order to use principalFromOrigin, and try removing the import (you'll need a try push to make sure nothing else started depending on it in the mean time).

r=me with that. Thanks for the patch!
Attachment #8663078 - Flags: review?(bobbyholley) → review+
Comment on attachment 8663077 [details] [diff] [review]
0001-Bug-1178533-Add-nsIInstallPackagedWebapp-for-registe.patch

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

r=me with nits fixed

::: b2g/installer/package-manifest.in
@@ +737,3 @@
>  ; ANGLE on Win32
>  #ifdef XP_WIN32
>  #ifndef HAVE_64BIT_BUILD

I think you will have to also add these to firefox's package-manifest.in to make packages work with mulet.
And you need to add them to b2gdroid/installer/package-manifest.in

::: dom/newapps/interfaces/nsIInstallPackagedWebapp.idl
@@ +8,5 @@
> +interface nsIInstallPackagedWebapp : nsISupports
> +{
> +  boolean installPackagedWebapp(in string manifestContent,
> +                                in string aOrigin,
> +                                in string aManifestURL);

nit: either aXyz or xzy everywhere.
Comment on attachment 8663077 [details] [diff] [review]
0001-Bug-1178533-Add-nsIInstallPackagedWebapp-for-registe.patch

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

r=me with nits fixed

::: b2g/installer/package-manifest.in
@@ +737,3 @@
>  ; ANGLE on Win32
>  #ifdef XP_WIN32
>  #ifndef HAVE_64BIT_BUILD

I think you will have to also add these to firefox's package-manifest.in to make packages work with mulet.
And you need to add them to b2gdroid/installer/package-manifest.in

::: dom/newapps/interfaces/nsIInstallPackagedWebapp.idl
@@ +8,5 @@
> +interface nsIInstallPackagedWebapp : nsISupports
> +{
> +  boolean installPackagedWebapp(in string manifestContent,
> +                                in string aOrigin,
> +                                in string aManifestURL);

nit: either aXyz or xzy everywhere.
Attachment #8663077 - Flags: review?(fabrice) → review+
See Also: → 1180088
Thanks for your reviews!
Uploading final patch in hg format with nits fixed, and pushing to try server as soon as I get back commit access to it.
Attachment #8661314 - Attachment is obsolete: true
Attachment #8661837 - Attachment is obsolete: true
Attachment #8663077 - Attachment is obsolete: true
Attachment #8663078 - Attachment is obsolete: true
(uploaded patch r+ with a commit message without try commands)
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=14564460&repo=mozilla-inbound
Flags: needinfo?(stephouillon)
It seems the test failed on Android platform
There was some lines missing in package-manifest.in for android, I'll push the fix on the try server to test.
Flags: needinfo?(stephouillon)
Fixed the issue on android and passed the tests on try (https://treeherder.mozilla.org/#/jobs?repo=try&revision=a56a1f61f49d)/.
Attachment #8664399 - Attachment is obsolete: true
Hi Stephanie,
sorry had to back this out again, seems this was causing test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=14625971&repo=mozilla-inbound
Flags: needinfo?(stephouillon)
The tests are passing on the emulator, but it's triggering an assertion failure in the cycle collector when the xpcom thread is shutdown, apparently because it tries to do a cleanup when it's too late.
I'm working on fixing it. My understanding is that it affects only the emulator so far because of perf issues.

logcat:
I/GonkMemoryPressure( 1093): Observed XPCOM shutdown.
I/Gecko   ( 1093): [1093] ###!!! ASSERTION: Failed Dispatch after xpcom-shutdown-threads: 'false', file /home/arroway/dev/B2G-flame/gecko/xpcom/threads/nsThread.cpp, line 587
E/Gecko   ( 1093): mozalloc_abort: [1093] ###!!! ASSERTION: Failed Dispatch after xpcom-shutdown-threads: 'false', file /home/arroway/dev/B2G-flame/gecko/xpcom/threads/nsThread.cpp, line 587
F/MOZ_CRASH( 1093): Hit MOZ_CRASH() at /home/arroway/dev/B2G-flame/gecko/memory/mozalloc/mozalloc_abort.cpp:33
Flags: needinfo?(stephouillon)
Finally fixed the issue on the emulator.
Waiting for try server results here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=17191998435e
Attachment #8667477 - Attachment is obsolete: true
Resultats are green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfffb42a1d6a
except for the builds which failed for graphene and horizon, but it looks like an infrastructure issue.
I hope this one is the good one.
https://hg.mozilla.org/mozilla-central/rev/08360a185f3f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Blocks: 1210992
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.