758269 - Install permissions from manifest to Permission Manager

Status: RESOLVED FIXED

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

Description

[Jim Straus]

When an app is installed the set of permissions from the manifest need to be set in the Permission Manager.  The Permission Manager sets the defaults for the given permissions based on the type (web app, trusted app, certified app).

Comment 2

[David Dahl :ddahl]

Now that I have read through the webapps code, I need to know what the possible valid permissions are, as well as how they will be structured in the manifest. The current docs do not cover this: https://developer.mozilla.org/en/OpenWebApps/The_Manifest

Perhaps there is a design doc or feature page with this information? I have read https://wiki.mozilla.org/Features/Mobile/webapps - but the only info on permissions is a screenshot, and is, I think, a fennec-specific page.

Comment 3

[Lucas Adamski [:ladamski]]

Gregor is implementing that list of permissions I believe.  We have bugs open to also put all these fields into the manifest.

Comment 4

[David Dahl :ddahl]

Attachment: v 1 WIP Patch

Gregor: Does this approach look correct?

Comment 5

[David Dahl :ddahl]

Attachment: v 2 WIP

sorry the last patch had a bunch of emacs-fixes-spacing-and-tabs madness in it

Comment 6

[David Dahl :ddahl]

Attachment: v 3 WIP patch

added removal of permissions

Comment 7

[David Dahl :ddahl]

Attachment: WIP - mochitest is not working

dclarke:

The tests for the webapps are difficult to decifer - how would I check the installed manifest to make sure they match the permissions I think were requested by the original app installation?

Comment 8

[[e-mail] [:onecyrenus]]

Comment on attachment 650239 [details] [diff] [review]
WIP - mochitest is not working

followed up via vidyo / chat.

Comment 9

[David Dahl :ddahl]

Attachment: Install perms working, issue with uninstall test

Install perms working as well as test. Uninstall test is not working need a better approach here as the test is kind of clunky in dynamically testing nsIDOMPermissionSettings interface after uninstall process. I could hard code the app url and permissions as plan B.

Comment 10

[[:fabrice] Fabrice Desré]

Comment on attachment 650352 [details] [diff] [review]
Install perms working, issue with uninstall test

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

I only looked at Webappsjsm - it's hard to say if it's doing the right thing in detail without knowing what this nsIDOMPermissionSettings interface is.

::: dom/apps/src/Webapps.jsm
@@ +20,5 @@
> +XPCOMUtils.defineLazyServiceGetter(this,
> +                                   "PermSettings",
> +                                   "@mozilla.org/permissionSettings;1",
> +                                   "nsIDOMPermissionSettings");
> +

Where is this implemented? I could not find anything about this interface in mxr

@@ +293,5 @@
> +                        content: UNKNOWN_ACTION,
> +                        app: UNKNOWN_ACTION,
> +                        trusted: UNKNOWN_ACTION,
> +                        certified: UNKNOWN_ACTION
> +                      },

that permission doesn't exist.

@@ +344,5 @@
> +                        certified: UNKNOWN_ACTION
> +                      },
> +
> +};
> +

That's a huge list! Can we move it to its own file?
Also, s/trusted/privileged since this is the new terminology.

Comment 11

[David Dahl :ddahl]

Attachment: Patch for review

This patch installs (and removes) permissions via the new nsIDOMPermissionSettings interface in bug 770731 - tests passing.

(fabrice, sorry for the earlier confusion. just marked this bug dependent on bug 770731)

Comment 12

[[:fabrice] Fabrice Desré]

Comment on attachment 650608 [details] [diff] [review]
Patch for review

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

We're getting close, but we should wait for bug 781620 to be sure that everything is coherent.

::: dom/apps/src/PermissionsTable.jsm
@@ +10,5 @@
> +                        "UNKNOWN_ACTION",
> +                        "ALLOW_ACTION",
> +                        "DENY_ACTION",
> +                        "PROMPT_ACTION",
> +                        "PROMPT_REMEMBER_ACTION",];

Do we really need to export all the *_ACTION?

@@ +18,5 @@
> +const DENY_ACTION = Ci.nsIPermissionManager.DENY_ACTION;
> +const PROMPT_ACTION = Ci.nsIPermissionManager.PROMPT_ACTION;
> +const PROMPT_REMEMBER_ACTION = Ci.nsIPermissionManager.PROMPT_REMEMBER_ACTION;
> +
> +const PERMS_TABLE = { idle: {

Some properties there are |"property"|, some just |property|. I prefer the second form.

I didn't check all the permissions listed here, but you're missing at least "pin-app".

Also, since this will need to match the appStatus in nsIPrincipal (https://mxr.mozilla.org/mozilla-central/source/caps/idl/nsIPrincipal.idl#241), using an array for each permission may be easier:
idle: [null, ALLOW_ACTION, ALLOW_ACTION, ALLOW_ACTION]

::: dom/apps/src/Webapps.jsm
@@ +287,5 @@
> +          } else {
> +            PermSettings.set(perm,
> +                             app.manifestURL,
> +                             app.origin,
> +                             PERMS_TABLE[perm].app,

This part will need to change a bit after bug 781620 lands, where we add a appStatus that matches what nsIPrincipal expects.

@@ +531,1 @@
>  

For app.permissions to be serialized properly, you must add it to _cloneAppObject

::: dom/tests/mochitest/webapps/apps/super_crazy.webapp
@@ +1,4 @@
>  {
>    "name": "Super Crazy Basic App",
> +  "installs_allowed_from": [ "*" ],
> +  "permissions": ["idle", "sms", "telephony"]

This is not how permissions will look like in manifests. The format is documented in bug 778326 comments #5 and #7

Comment 13

[Mounir Lamouri (:mounir)]

Comment on attachment 650608 [details] [diff] [review]
Patch for review

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

::: dom/apps/src/PermissionsTable.jsm
@@ +18,5 @@
> +const DENY_ACTION = Ci.nsIPermissionManager.DENY_ACTION;
> +const PROMPT_ACTION = Ci.nsIPermissionManager.PROMPT_ACTION;
> +const PROMPT_REMEMBER_ACTION = Ci.nsIPermissionManager.PROMPT_REMEMBER_ACTION;
> +
> +const PERMS_TABLE = { idle: {

Wouldn't that be more readable to have something like:
content: { "foo", "bar", ... },
app: { "foo", "bar", ... },
privileged: [...]
[...]

Also, "idle" isn't allowed to content. Battery and network do not require permissions. Same goes for screen-orientation and likely vibration.

Comment 14

[David Dahl :ddahl]

(In reply to Fabrice Desré [:fabrice] from comment #12)
> Comment on attachment 650608 [details] [diff] [review]
> ::: dom/apps/src/PermissionsTable.jsm
> @@ +10,5 @@
> > +                        "UNKNOWN_ACTION",
> > +                        "ALLOW_ACTION",
> > +                        "DENY_ACTION",
> > +                        "PROMPT_ACTION",
> > +                        "PROMPT_REMEMBER_ACTION",];
> 
> Do we really need to export all the *_ACTION?
> 
Perhaps not.

> @@ +18,5 @@
> > +const DENY_ACTION = Ci.nsIPermissionManager.DENY_ACTION;
> > +const PROMPT_ACTION = Ci.nsIPermissionManager.PROMPT_ACTION;
> > +const PROMPT_REMEMBER_ACTION = Ci.nsIPermissionManager.PROMPT_REMEMBER_ACTION;
> > +
> > +const PERMS_TABLE = { idle: {
> 
> Some properties there are |"property"|, some just |property|. I prefer the
> second form.
> 
any property containing a dash must be in quotes or you get a syntax error in JS.

> I didn't check all the permissions listed here, but you're missing at least
> "pin-app".
> 
I was also pretty sure this list in inaccurate. Is there a google doc somewhere that details the perms?

> Also, since this will need to match the appStatus in nsIPrincipal
> (https://mxr.mozilla.org/mozilla-central/source/caps/idl/nsIPrincipal.
> idl#241), using an array for each permission may be easier:
> idle: [null, ALLOW_ACTION, ALLOW_ACTION, ALLOW_ACTION]
> 
ah, ok.

> ::: dom/apps/src/Webapps.jsm
> @@ +287,5 @@
> > +          } else {
> > +            PermSettings.set(perm,
> > +                             app.manifestURL,
> > +                             app.origin,
> > +                             PERMS_TABLE[perm].app,
> 
> This part will need to change a bit after bug 781620 lands, where we add a
> appStatus that matches what nsIPrincipal expects.

sounds good. I will watch for that update.

> 
> @@ +531,1 @@
> >  
> 
> For app.permissions to be serialized properly, you must add it to
> _cloneAppObject
> 
ok.

> ::: dom/tests/mochitest/webapps/apps/super_crazy.webapp
> @@ +1,4 @@
> >  {
> >    "name": "Super Crazy Basic App",
> > +  "installs_allowed_from": [ "*" ],
> > +  "permissions": ["idle", "sms", "telephony"]
> 
> This is not how permissions will look like in manifests. The format is
> documented in bug 778326 comments #5 and #7

Got it, thanks!

Comment 15

[David Dahl :ddahl]

Attachment: WIP Patch

Uploading for Gregor to look at

Comment 16

[Justin Lebar (not reading bugmail)]

I'm pretty confused by this approach.

If we grant the set of "default permissions" to an app at install-time, doesn't that make it difficult for us to ever add another default permission?

In particular, suppose I have an app A.  I install it, and it gets one permission, X, by default.  Now I upgrade Gecko to v2.  With Gecko v2, apps get permissions X and Y by default.  But app A only has permission X, because it was installed before the Gecko upgrade.

So either:

 1) On every Gecko upgrade, we have to keep track of which default permissions we added and update all apps' permissions, or
 2) Every time Gecko starts up, we have to go through and ensure that all apps have, at minimum, the set of default permissions applicable for their "level" (untrusted, trusted, certified), or
 3) Every time an app starts up, we have to go through and ensure that it has, at minimum, the set of default permissions applicable for its "level", or
 4) We hard-code in the policy that "trusted apps get permission X automatically" into the permission manager.

(4) seems like the simplest way to do this; (1-3) seem much more complicated and error-prone.

Comment 17

[Lucas Adamski [:ladamski]]

I think Jonas had advocated for a slightly different approach, which was to require apps to declare all of the permissions they wanted to us regardless of whether they were implicit or explicit.  I think that would solve this particular problem, at the expense of requiring developer to enumerate a lot more permissions.

Comment 18

[Justin Lebar (not reading bugmail)]

(In reply to Lucas Adamski from comment #17)
> I think Jonas had advocated for a slightly different approach, which was to
> require apps to declare all of the permissions they wanted to us regardless
> of whether they were implicit or explicit.  I think that would solve this
> particular problem, at the expense of requiring developer to enumerate a lot
> more permissions.

That approach also pushes the onus to be aware of implicit permissions which become available in new versions of gecko down to the app developer, which I don't think is particularly friendly.

Comment 19

[Lucas Adamski [:ladamski]]

The thinking there was that these permissions would mostly be additive; adding new features the app would need to be updated to take advantage of anyway.  If the permission change is subtractive then I think you're kinda boned either way.

Comment 20

[Justin Lebar (not reading bugmail)]

An upgrade goes through the same process as an install?

Comment 21

[Justin Lebar (not reading bugmail)]

>+const PERMS_TABLE = { idle: {
>+                        content: ALLOW_ACTION,
>+                        app: ALLOW_ACTION,
>+                        privileged: ALLOW_ACTION,
>+                        certified: ALLOW_ACTION
>+                      },

Idle is certified-only.  See bug 780507.

>+                      "screen-orientation": {
>+                        content: ALLOW_ACTION,
>+                        app: ALLOW_ACTION,
>+                        privileged: ALLOW_ACTION,
>+                        certified: ALLOW_ACTION
>+                      },

Content does not have permission to lock screen orientation unless it's
full-screen.  I think the way to encode this is as a DENY; the relevant
full-screen code will (once I fix it) ignore the DENY if we're full-screen.

>+                      "wifi-information": {
>+                        content: DENY_ACTION,
>+                        app: DENY_ACTION,
>+                        privileged: PROMPT_ACTION,
>+                        certified: ALLOW_ACTION
>+                      },

If the app is privileged, someone has signed off on it.  I don't think it needs
to prompt before accessing wifi.


Could you please check the security discussions for the APIs below?  These
should all be written up on the wiki, and I think the permissions here may not
match the conclusions there.  If the permissions below do match the conclusions
in the wiki, then please let me know so we can figure out if these are indeed
the right thing.

>+                      "resource-lock": {
>+                        content: ALLOW_ACTION,
>+                        app: ALLOW_ACTION,
>+                        privileged: ALLOW_ACTION,
>+                        certified: ALLOW_ACTION
>+                      },

>+                      sensor: {
>+                        content: ALLOW_ACTION,
>+                        app: ALLOW_ACTION,
>+                        privileged: ALLOW_ACTION,
>+                        certified: ALLOW_ACTION
>+                      },

>+                      sms: {
>+                        content: PROMPT_ACTION,
>+                        app: PROMPT_ACTION,
>+                        privileged: PROMPT_ACTION,
>+                        certified: ALLOW_ACTION
>+                      },

>+                      telephony: {
>+                        content: PROMPT_ACTION,
>+                        app: PROMPT_ACTION,
>+                        privileged: PROMPT_ACTION,
>+                        certified: ALLOW_ACTION
>+                      },

>+                      contacts: {
>+                        content: PROMPT_ACTION,
>+                        app: PROMPT_ACTION,
>+                        privileged: PROMPT_ACTION,
>+                        certified: ALLOW_ACTION
>+                      },

Comment 22

[Justin Lebar (not reading bugmail)]

Add to the list of ones I think are wrong:

>+                      geolocation: {
>+                        content: ALLOW_ACTION,
>+                        app: ALLOW_ACTION,
>+                        privileged: ALLOW_ACTION,
>+                        certified: PROMPT_ACTION
>+                      },

This should be prompt for content, right?

>+                      sensor: {
>+                        content: ALLOW_ACTION,
>+                        app: ALLOW_ACTION,
>+                        privileged: ALLOW_ACTION,
>+                        certified: ALLOW_ACTION
>+                      },

Content can't silently access the accelerometers, right?  So this is prompt?

Comment 23

[David Dahl :ddahl]

(In reply to Justin Lebar [:jlebar] from comment #22)
> Add to the list of ones I think are wrong:
> 
> >+                      geolocation: {
> >+                        content: ALLOW_ACTION,
> >+                        app: ALLOW_ACTION,
> >+                        privileged: ALLOW_ACTION,
> >+                        certified: PROMPT_ACTION
> >+                      },
> 
> This should be prompt for content, right?
> 
> >+                      sensor: {
> >+                        content: ALLOW_ACTION,
> >+                        app: ALLOW_ACTION,
> >+                        privileged: ALLOW_ACTION,
> >+                        certified: ALLOW_ACTION
> >+                      },
> 
> Content can't silently access the accelerometers, right?  So this is prompt?

Indeed, I have been re-working these based on the perms matrix here: https://docs.google.com/spreadsheet/ccc?key=0Akyz_Bqjgf5pdENVekxYRjBTX0dCXzItMnRyUU1RQ0E#gid=0

I'll have a new patch soon. Of course now I wonder if the approach is correct here based on all of the earlier discussion.

Comment 24

[Justin Lebar (not reading bugmail)]

> Of course now I wonder if the approach is correct here based on all of the earlier discussion.

I personally think it's silly to require apps to request "popup" permission in order to get the prompt-less popup windows they're entitled to, or to require the app to request "orientation-lock" permission if it wants to lock the orientation while it's not full-screen.

It's a non-trivial amount of work for the app developer to figure out how to tweak their manifest to get it Just Right, and I don't see what the advantage is to us or our users of asking the app to explicitly enumerate all of the permissions that we're willing to grant it by virtue of being an app.  

But if on every app upgrade we take any new Gecko-provided permissions into account (and that's what I understand is suggested by this patch, even if it doesn't /do/ that), then that would at least be sensible.

Comment 25

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

I think our general policy should be: Everything that a fullscreened normal webpage can't do should be enumerated in the manifest.

So circumventing popup blockers should be enumerated. But asking for orientation events or using orientation locks doesn't need to be enumerated.

However "getting orientation events while in the background" would need to be enumerated.

Comment 26

[David Dahl :ddahl]

As specified in https://bugzilla.mozilla.org/show_bug.cgi?id=778326#c14

a permission entry in a manifest like:

    "contacts": {
      "description": "To read and write your contacts",
      "access": "readwrite"
    }

would map to 3 distinct permissions (read, create, write) in the nsIPermissionManager database? If so, how should this be done? Do we add 3 permissions like: contacts-read, contacts-create, contacts-write? 

Or, do we add an 'access' property to the nsIPermissionManager interface?

Comment 27

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

We would have 3 distinct entries in nsIPermissionManager: contacts-read, contacts-create, contacts-write. Each of them can be set to ACCESS, DENY, PROMPT or PROMPT_REMEMBER.

When we need to prompt, we read all three properties to know which exact permissions that the app has requested. (We can use this to affect which exact language to put in the prompt, though I think the decision for now from Lucas is that he wants prompts to not surface exact permissions).

Once the prompt has been approved, and if the user chose to "remember this decision", we change all of the contacts-* properties that had PROMPT/PROMPT_REMEMBER to either ACCESS or DENY.

Comment 28

[David Dahl :ddahl]

Attachment: Patch 2 making progress

Getting closer here, able to generate proper permission types, tests might actually pass if this was not happening:

-*- PermissionSettings: Set called with: fmradio-create, null, http://www.example.com:80/chrome/dom/tests/mochitest/webapps/apps/super_crazy.webapp, http://www.example.com,  1, true
No Permission to set permission

I have this set in test_install_app.xul: 

SpecialPowers.setBoolPref("dom.mozPermissionSettings.enabled", true);
SpecialPowers.addPermission("permissions", true, document);

(the test can be run like so: TEST_PATH=dom/tests/mochitest/webapps/test_install_app.xul make -C . mochitest-chrome)

This patch requires the patch from bug 770731

Comment 29

[Lucas Adamski [:ladamski]]

To confirm, yes I don't want to prompt with details of read/write/create.  Either we'd have to prompt three times or the prompts get very complicated.  We can revisit displaying more granularity in the future but for now lets keep it simple.

Comment 30

[Justin Lebar (not reading bugmail)]

Ping on comment 20.

Comment 31

[David Dahl :ddahl]

(In reply to Justin Lebar [:jlebar] from comment #20)
> An upgrade goes through the same process as an install?

I think so, we remove all of the existing perms for the app, re-adding them as we parse the manifest as they are potentially different.

Comment 32

[David Dahl :ddahl]

Attachment: Patch 3

Gregor:

I think we still need the PermissionSettings interface to remove the access argument in bug 770731, right?

Comment 33

[Lucas Adamski [:ladamski]]

(In reply to Justin Lebar [:jlebar] from comment #20)
> An upgrade goes through the same process as an install?

At a high level, yes, except we shouldn't reset existing permission settings.  Any new explicit permission requests would trigger a new prompt.

Comment 34

[David Dahl :ddahl]

Attachment: Latest patch, testing issues

Latest patch for gregor to look at

Comment 35

[David Dahl :ddahl]

(In reply to David Dahl :ddahl from comment #34)
> Created attachment 654245 [details] [diff] [review]
> Latest patch, testing issues
> 
> Latest patch for gregor to look at

Also, the test I am running is: TEST_PATH=dom/tests/mochitest/webapps/test_install_app.xul make -C . mochitest-chrome

Comment 36

[Gregor Wagner [:gwagner]]

Comment on attachment 654245 [details] [diff] [review]
Latest patch, testing issues

>+const PERMS_TABLE = { idle: {
>+                        content: DENY_ACTION,
>+                        app: DENY_ACTION,
>+                        privileged: ALLOW_ACTION,
>+                        certified: ALLOW_ACTION
>+                      },

I don't think we need this. The idle API won't use the permission manager for v1.
We have to decide if we want to add also future permissions or just currently used ones.

>+                      "resource-lock": {
>+                        content: ALLOW_ACTION,
>+                        "content-fullscreen": PROMPT_ACTION, // XXXddahl: not sure what this should look like
>+                        app: ALLOW_ACTION,
>+                        privileged: ALLOW_ACTION,
>+                        certified: ALLOW_ACTION
>+                      },

Yes that's a good questions. Do we have any spec for this?


>+                      "settings-read": {
>+                        content: DENY_ACTION,
>+                        app: DENY_ACTION,
>+                        privileged: DENY_ACTION,
>+                        certified: ALLOW_ACTION
>+                      },
>+                      "settings-write": {
>+                        content: DENY_ACTION,
>+                        app: DENY_ACTION,
>+                        privileged: DENY_ACTION,
>+                        certified: ALLOW_ACTION
>+                      },

There is only one permission for settings now.


>--- a/dom/permission/PermissionSettings.js
>+++ b/dom/permission/PermissionSettings.js
>@@ -1,15 +1,15 @@
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>  * You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> "use strict";
> 
>-let DEBUG = 0;
>+let DEBUG = 1;

We shouldn't change this.


So the remaining questions are:
How should we proceed with "fullscreen" permissions?

How should we store the WEB_ACTIVITIES_ACTION "permission"
I don't think we have to store it.

Comment 37

[David Dahl :ddahl]

Attachment: Patch 4, test in progress

Marco: Any chance you have a moment to look at the browser chrome test on this patch - the notification is never triggered unless the browser tab's window.body is clicked on. I cannot reliably make the nav.mozApps.install() trigger. Is this a browser chrome issue?

Comment 38

[Marco Bonardo [:mak]]

So you don't see the popupshown event? what about popupshowing? Did you try to just put an info() at the beginning of the handler to see if it runs, or does it just run only when you focus the window?
Btw, nothing that jumps at my eyes, I see a couple other tests use similar methods and listeners, so it's supposed to just work. I should probably apply the patch and run it, I may try later.

Comment 39

[David Dahl :ddahl]

(In reply to Marco Bonardo [:mak] from comment #38)
> So you don't see the popupshown event? what about popupshowing? 

Yep, i see it, but the popup does not always have childNodes, so it times out...
 
> Did you try
> to just put an info() at the beginning of the handler to see if it runs, or
> does it just run only when you focus the window?

It seems to only run when you focus the window. I wonder if this is a linux window manager thing?

> Btw, nothing that jumps at my eyes, I see a couple other tests use similar
> methods and listeners, so it's supposed to just work. I should probably
> apply the patch and run it, I may try later.

Thanks!

Comment 40

[David Dahl :ddahl]

Attachment: Latest Patch, better test

A quick update: I am finding some event issues getting my browser chrome test to run without human intervention (a click at the right moment seems necessary to get the doorhanger to trigger). The existing test suite is mochitest chrome and is a bit difficult to follow and customize, so I stopped trying to use it in favor of a single new browser chrome test.

Doug: I was hoping you could apply this patch and maybe help me walk through this event problem. 

I currently have the permissions manifest being read, the permission installed correctly - and have passing tests for this. 

The patch seems to uninstall permissions, but the tests are not running, which may be an issue with DOMRequest. I should perhaps instead change the test to listen for a notification and bypass the DOMRequest object. I am continuing to fix this right now.

A note on our approach:
I'm not sure, but I feel like we could have designed this Apps API to be testable in xpcshell - what do you think?

Comment 41

[David Dahl :ddahl]

Attachment: Latest, better test

Comment 42

[David Dahl :ddahl]

(In reply to David Dahl :ddahl from comment #41)
> Created attachment 655663 [details] [diff] [review]
> Latest, better test

dependent patch: https://bug770731.bugzilla.mozilla.org/attachment.cgi?id=655664

Comment 43

[Marco Bonardo [:mak]]

handlePopup looks strange, apart the argument that should be aEvent, not aPanel, why do you need all those focus calls and focus after synthesizing a click? also, PopupNotifications.panel.childNodes (or aEvent.target.childNodes is likely the same) should be available immediately, and you should stop listening to popupshown.
You can also likely use synthesizeMouseAtCenter, at least on the button.

Comment 44

[Doug Turner (:dougt)]

The test hangs on the fm-radio test.

Comment 45

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

Comment on attachment 655187 [details] [diff] [review]
Patch 4, test in progress

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

::: dom/apps/src/PermissionsTable.jsm
@@ +39,5 @@
> +//                       },
> +// };
> +
> +// // XXXddahl: Offload these permsissions to web activities? Do we even need these listed here? Maybe only for the 'record'?
> +// const PERMS_TABLE_WEB_ACTIVITIES = {

I don't think there is any value in a table like this. The fact that an API is "available through webactivities" basically mean that the API isn't available at all, but that you can get similar functionality using WebActivities. So from a Gecko point of view "available through webactivities" simply means "not available".

@@ +84,5 @@
> +// };
> +
> +// NOTE: Commented-out permissions are not in the works yet
> +
> +const PERMS_TABLE = { // idle: {

In this table you should also list which properties use an 'access' property. I.e. the contacts API has a concept of "read" vs. "write" vs. "create", however geolocation and prompt API has no such concept.

@@ +90,5 @@
> +                      //   app: DENY_ACTION,
> +                      //   privileged: ALLOW_ACTION,
> +                      //   certified: ALLOW_ACTION
> +                      // }, // XXX: Not used in initial verison
> +                      "battery-status": {

I think that the APIs which are "allow" everywhere should simply not go through any of this code. We should just grant access everywhere.

@@ +96,5 @@
> +                        app: ALLOW_ACTION,
> +                        privileged: ALLOW_ACTION,
> +                        certified: ALLOW_ACTION
> +                      },
> +                      "network-information": {

Same for this one.

@@ +104,5 @@
> +                        certified: ALLOW_ACTION
> +                      },
> +                      "resource-lock": {
> +                        content: ALLOW_ACTION,
> +                        "content-fullscreen": PROMPT_ACTION, // XXXddahl: not sure what this should look like

This seems reversed. Shouldn't the 'content' part be set to PROMPT and the fullscreen be set to ALLOW?

Hmm.. though is there a point to putting 'content' in this table at all? Since content doesn't go through the installation code. Or are you using this table somehow elsewhere too?

@@ +109,5 @@
> +                        app: ALLOW_ACTION,
> +                        privileged: ALLOW_ACTION,
> +                        certified: ALLOW_ACTION
> +                      },
> +                      vibration: {

This can also be removed.

@@ +116,5 @@
> +                        privileged: ALLOW_ACTION,
> +                        certified: ALLOW_ACTION
> +                      },
> +                      "screen-orientation": {
> +                        content: ALLOW_ACTION, // XXXddahl: Matrix says inplicit (fullscreen-only) - should this be DENY_ACTION??

This should be the same as resource-lock

@@ +132,5 @@
> +                      geolocation: {
> +                        content: PROMPT_ACTION,
> +                        app: PROMPT_ACTION,
> +                        privileged: PROMPT_ACTION,
> +                        certified: PROMPT_ACTION

Actually, the matrix says that this should be 'allow' for certified apps. Though personally I'd prefer that this prompted there too, but we should stick to the matrix for now.

@@ +135,5 @@
> +                        privileged: PROMPT_ACTION,
> +                        certified: PROMPT_ACTION
> +                      },
> +                      sensor: {
> +                        content: PROMPT_ACTION,

This doesn't match what the matrix says.

@@ +142,5 @@
> +                        certified: ALLOW_ACTION
> +                      },
> +                      "camera": {
> +                        content: WEB_ACTIVITIES_ACTION,
> +                        app: WEB_ACTIVITIES_ACTION,

See comment above. I think we should simply say "DENY" for these.

Not removing more of this table for now, would like to get the above questions answered first.

::: dom/apps/src/Webapps.jsm
@@ +95,5 @@
> +    suffixes.push("create");
> +    break;
> +  case READCREATE:
> +    suffixes.push("read");
> +    suffixes.push("create");

suffixes.push("read", "create");

@@ +404,5 @@
> +      for (let permName in manifest.permissions) {
> +        if (!PERMS_TABLE[permName]) {
> +          Cu.reportError("'" + permName + "'" +
> +                         " is not a valid Webapps permission type");
> +          continue;

I think we should abort if this happens, and not just ignore any unknown permissions.

Comment 46

[David Dahl :ddahl]

Attachment: Patch 5: test is coming along

Still working out the install and removal of permissions. The test is now more helpful as it shows that there might be some problems with the dependent patch in bug 770731. Also, the focus issue is still there, going to test it on a mac next to rule out a potential window manager / focus issue.

Comment 47

[David Dahl :ddahl]

via gdb I have determined that the permission entry is added tot he database. Mid-test I was able to manually search the sqlite db:

102|example.com|sms-read|1|0|0|0|1
103|example.com|sms-create|1|0|0|0|1
104|example.com|sms-write|1|0|0|0|1
105|example.com|telephony|1|0|0|0|1
106|example.com|contacts-read|1|0|0|0|1
107|example.com|fmradio-read|1|0|0|0|1
108|example.com|fmradio-create|1|0|0|0|1

Looks like the appID is set to 0 - which seems wrong to me - somewhere in the DOMApplicationRegistry this id is created, somewhere else in Webapps.jsm this id should be saved to the manifest and writtem to disk in a json file in the priofile.

still digging - I need to review the tests for this part of the Webapps code

Comment 48

[David Dahl :ddahl]

(In reply to Marco Bonardo [:mak] from comment #38)
> I should probably
> apply the patch and run it, I may try later.

No worries, Marco, thanks. The popup issue is only a problem on my exotic window manager, works correctly on Mac. Sorry to bother you.

Comment 49

[David Dahl :ddahl]

see bug 770731 comment 25 https://bugzilla.mozilla.org/show_bug.cgi?id=770731#c25 for the latest issue...

Comment 50

[David Dahl :ddahl]

Attachment: Patch 6

Was going to switch to mochitest plain but then I realized i had no way to dismiss the popup notification. Of course before getting to that I find the test throwing an exception that never makes it back up to the debug logging... sigh.

Back to the mochitest browser chrome test, see bug 770731 comment 25

still digging here.

Comment 51

[David Dahl :ddahl]

Ok, here I am logging the localID lookup routine from AppService.js that calls into Webapps.jsm: 

Webapps.jsm: >>> pprint recurse into {dabe2254-1b04-47be-90b6-b016896fea3d}
Webapps.jsm: **** pprint [object Object]
Webapps.jsm: installOrigin: http://mochi.test:8888
Webapps.jsm: origin: http://mochi.test:8888
Webapps.jsm: receipts:
Webapps.jsm: >>> pprint recurse into receipts
Webapps.jsm: **** pprint
Webapps.jsm: installTime: 1346265197421
Webapps.jsm: manifestURL: http://mochi.test:8888/browser/dom/tests/browser/test-webapp.webapp
Webapps.jsm: progress: 0
Webapps.jsm: status: installed
Webapps.jsm: permissions: [object Object]
Webapps.jsm: >>> pprint recurse into permissions
Webapps.jsm: **** pprint [object Object]
Webapps.jsm: sms: [object Object]
Webapps.jsm: >>> pprint recurse into sms
Webapps.jsm: **** pprint [object Object]
Webapps.jsm: description: sms, baby!
Webapps.jsm: access: readwrite
Webapps.jsm: telephony: [object Object]
Webapps.jsm: >>> pprint recurse into telephony
Webapps.jsm: **** pprint [object Object]
Webapps.jsm: description: phones and stuff
Webapps.jsm: contacts: [object Object]
Webapps.jsm: >>> pprint recurse into contacts
Webapps.jsm: **** pprint [object Object]
Webapps.jsm: description: To read and write your contacts
Webapps.jsm: access: readonly
Webapps.jsm: fmradio: [object Object]
Webapps.jsm: >>> pprint recurse into fmradio
Webapps.jsm: **** pprint [object Object]
Webapps.jsm: description: read create test for fmradio
Webapps.jsm: access: readcreate
Webapps.jsm: localId: 6

The app was installed, has a guid name and localID of 6. All permissions are there as in the manifest. The localId does not end up in the PermissionsManager sqlite database, it ends up being "0".

Comment 52

[David Dahl :ddahl]

Attachment: Patch 7

I think the problem is in the way we get a principal to do a permission lookup. they do not seem to match between setting the perm and getting it later.

Comment 53

[Myk Melez [:myk] [@mykmelez]]

(In reply to David Dahl :ddahl from comment #50)
> Was going to switch to mochitest plain but then I realized i had no way to
> dismiss the popup notification.

I experimented with a mochitest-plain test in my webapps test unrefactoring patch (bug 785545), and I found that something like the following code would press the Install button in an install confirmation dialog:

  var { classes: Cc, interfaces: Ci } = SpecialPowers.wrap(Components);

  // XXX Ideally we'd get the exact window in which our test is running, 
  // on the off chance it isn't the most recent window.
  var popupPanel = Cc["@mozilla.org/appshell/window-mediator;1"].
                   getService(Ci.nsIWindowMediator).
                   getMostRecentWindow("navigator:browser").
                   PopupNotifications.panel;

  function onPopupShown() {
    popupPanel.removeEventListener("popupshown", onPopupShown, false);
    SpecialPowers.wrap(this).childNodes[0].button.doCommand();
  }
  popupPanel.addEventListener("popupshown", onPopupShown, false);

Comment 54

[David Dahl :ddahl]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #53)
> (In reply to David Dahl :ddahl from comment #50)
> > Was going to switch to mochitest plain but then I realized i had no way to
> > dismiss the popup notification.
> 
> I experimented with a mochitest-plain test in my webapps test unrefactoring
> patch (bug 785545), and I found that something like the following code would
> press the Install button in an install confirmation dialog:

Thanks Myk. I think I have it working now with a browser-chrome test.

Comment 55

[David Dahl :ddahl]

Attachment: Patch 8 - install tests work

Ok, I have a mostly working test suite for installation and removal of permissions. The permissions 'remove' operation all have failing tests. The PermissionSetting component only has a get and set methods - so I call set() with the perm name and "unknown" access to "remove" the permission. This does not seem to work, so I added a check to see if there was already a perm installed when you set a perm to "unknown" access. In this case, set() calls the pm.removeFromPrincipal(). This also does not work.

When the PermissionSetting component goes to remove the

Comment 56

[David Dahl :ddahl]

(In reply to David Dahl :ddahl from comment #55)

> 
> When the PermissionSetting component goes to remove the...

 ... permission, I think there is another principal mismatch. Still digging here. 

Gregor: I am hoping you can take a look at this as soon as you have a moment to spare:)

Comment 57

[Mounir Lamouri (:mounir)]

Comment on attachment 656993 [details] [diff] [review]
Patch 8 - install tests work

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

Please, do not keep commented code in your patches. Also, use the apps from dom/tests/mochitests/webapps/ instead of adding yet another webapp to the test suite. Note that you could have a mochitest-chrome.

::: dom/tests/browser/Makefile.in
@@ +21,5 @@
>    browser_autofocus_preference.js \
>    browser_bug396843.js \
> +	browser_webapps_permissions.js \
> +	test-webapp.webapp \
> +	test-webapps-permissions.html \

nit: indentation

Comment 58

[Gregor Wagner [:gwagner]]

There is a lot of commented out code in there. Could you remove it?

Comment 59

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

David: Did you see my comments in comment 45? It doesn't seem like you have addressed any of them.

Comment 60

[David Dahl :ddahl]

(In reply to Jonas Sicking (:sicking) from comment #59)
> David: Did you see my comments in comment 45? It doesn't seem like you have
> addressed any of them.

I did see the comments, I will address them this morning

Comment 61

[Doug Turner (:dougt)]

Comment on attachment 656993 [details] [diff] [review]
Patch 8 - install tests work

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

::: dom/apps/src/PermissionsTable.jsm
@@ +5,5 @@
> +"use strict";
> +
> +const Ci = Components.interfaces;
> +
> +var EXPORTED_SYMBOLS = ["PERMS_TABLE",

I am okay with exporting all uppercase symbols for strings.  But is there a rule that says that objects get a different style?  f.e. "PermissionTable"

::: dom/apps/src/Webapps.js
@@ +17,5 @@
>  });
>  
> +function debug(aMsg)
> +{
> +  dump("-*- Webapps.js: " + aMsg + "\n");

Lets not check this in without a if (DEBUG) condition around it.

@@ +70,5 @@
>        return;
>      let app = msg.app;
>      switch (aMessage.name) {
>        case "Webapps:Install:Return:OK":
> +        Services.obs.notifyObservers(null, "webapps-install-ok", JSON.stringify(app));

What type is app here?  Should we pass app as a subject instead of stringifying it?

@@ +401,5 @@
>      // We want Webapps:Install:Return:OK and Webapps:Uninstall:Return:OK to be boradcasted
>      // to all instances of mozApps.mgmt
> +    if (!((msg.oid == this._id && req)
> +       || aMessage.name == "Webapps:Install:Return:OK" || aMessage.name == "Webapps:Uninstall:Return:OK")) {
> +      let _topic;

remove the _ prefix.

@@ +413,5 @@
> +        _notify = true;
> +      }
> +      if (_notify) {
> +        debug("notifying...");
> +        Services.obs.notifyObservers(null, "webapps-install-ok", JSON.stringify(app));

Shouldn't this be notifyObservers(null, **topic**, JSON.stringify(app));


Also:

You can rewrite this without the locals and without the extra branch:


if (aMessage.name == "Webapps:Install:Return:OK") {
  Services.obs.notifyObservers(null, "webapps-install-ok", JSON.stringify(app));
  return;
}

if (aMessage.name == "Webapps:Uninstall:Return:OK") { 
   Services.obs.notifyObservers(null, "webapps-uninstall-ok", JSON.stringify(app));
   return;
}

debug("unhandled message " + aMessage.name");

@@ +430,5 @@
>          Services.DOMRequest.fireSuccess(req, convertAppsArray(msg.apps, this._window));
>          break;
>        case "Webapps:Install:Return:OK":
> +        debug("WebApps Install OK");
> +        debug(this._onuninstall);

If you are going to keep these debug() stmt's, please join them.

::: dom/apps/src/Webapps.jsm
@@ +6,5 @@
> +let DEBUG = 0;
> +var debug = function (s){};
> +if (DEBUG) {
> +  debug = function (s) { dump("-*- Webapps.jsm: " + s + "\n"); };
> +}

not:

var debug = function(s) {
  if (DEBUG) {
    dump("-*- Webapps.jsm: " + s + "\n");
  }
}

@@ +9,5 @@
> +  debug = function (s) { dump("-*- Webapps.jsm: " + s + "\n"); };
> +}
> +function log(aMsg)
> +{
> +  dump("Webapps.jsm: " + aMsg + "\n");

why have a separate log() function?

@@ +23,5 @@
> +      log(">>> pprint recurse into " + prop);
> +      pprint(aObj[prop], true);
> +    }
> +  }
> +}

As a sidenote, this should be really be built into the language.

@@ +55,5 @@
> +  1: "allow",
> +  2: "deny",
> +  3: "prompt",
> +  4: "prompt-remember"
> +};

Do these numbers match anything else is gecko?  If so, it might be nice to reference that here.

@@ +101,5 @@
> + * @param string aPermName
> + * @param string aAccess
> + * @returns Array
> + **/
> +function expandPermissions(aPermName, aAccess)

ah, this is cool.  I need to probably do this with device storage too.

@@ +447,5 @@
> +    // pprint(appObject, true);
> +
> +    let manifest = new DOMApplicationManifest(app.manifest, app.origin);
> +    // check the manifest for permissions
> +    if (manifest.permissions) {

return early.

Also, how can this happen and what do we do about it?

@@ +454,5 @@
> +          Cu.reportError("'" + permName + "'" +
> +                         " is not a valid Webapps permission type");
> +          continue;
> +        }
> +        else {

No need for else here.

@@ +458,5 @@
> +        else {
> +          let perms = [];
> +          // get access:
> +          if (manifest.permissions[permName].access) {
> +            perms = expandPermissions(permName,

It would kind of be nice if you could call expandPermission even if manifest.permissions[permName].access is null/undefined.  In that case, it will just return permName.  

If you did this, you could simplify this a bit and just do:

perms.push(expandPermissions(manifest.permissions, permName));

::: dom/tests/browser/browser_webapps_permissions.js
@@ +31,5 @@
> +const uninstalledPermsToTest = {
> +  "sms-read": "unknown",
> +  "sms-create": "unknown",
> +  "sms-write": "unknown",
> +  telephony: "unknown",

"telephony"

@@ +55,5 @@
> +
> +  browser.addEventListener("DOMContentLoaded", function onLoad(event) {
> +    browser.removeEventListener("DOMContentLoaded", onLoad, false);
> +    gWindow = browser.contentWindow;
> +    Services.prefs.setBoolPref("dom.mozPermissionSettings.enabled", true);

special powers.

@@ +148,5 @@
> +    gWindow.document.body.focus();
> +    let notifications = aPopup.target.childNodes;
> +    let notification = notifications[0];
> +    EventUtils.synthesizeMouse(notification.button, 20, 10, {});
> +  });

This is pretty fragile.  Is there a way we can generate a click on this button instead of sending mouse events?

Comment 62

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

We had a discussion about how to use permission manager for B2G today. My current proposal was to at install time write "prompt" vs "prompt-permanent" into the permission manager depending on if it's a "normal" vs "privileged" app, and I think that's what the current patch does.

However both Mounir and Jlebar felt that differenting between "prompt" and "prompt-permanent" in the permission manager was an abuse of the API. Instead we should make that decision by looking at the appStatus property directly on the nsIPrincipal.

This sounds ok to me, and removes the problem of what to do if some properties say "prompt" and some say "prompt-remember" when putting up a dialog.

Comment 63

[David Dahl :ddahl]

Attachment: Patch 9 ALL TESTS PASS;)

Install and Uninstall tests all pass! I will be updating the patch with Jonas and dougt's review comments.

Also, I should figure out how to test an 'app update' where the permissions change between releases. Will this require the test to overwrite the webapp manifest between the "install" and "re-install" tests? Or, can the webapp use an alternate manifest located elsewhere? I am not sure.

Comment 64

[David Dahl :ddahl]

(In reply to Doug Turner (:dougt) from comment #61)
> Comment on attachment 656993 [details] [diff] [review]
>
> @@ +70,5 @@
> >        return;
> >      let app = msg.app;
> >      switch (aMessage.name) {
> >        case "Webapps:Install:Return:OK":
> > +        Services.obs.notifyObservers(null, "webapps-install-ok", JSON.stringify(app));
> 
> What type is app here?  Should we pass app as a subject instead of
> stringifying it?

It is an object, however, I have removed the notification as it is not needed anymore.

> 
> @@ +401,5 @@
> >      // We want Webapps:Install:Return:OK and Webapps:Uninstall:Return:OK to be boradcasted
> >      // to all instances of mozApps.mgmt
> > +    if (!((msg.oid == this._id && req)
> > +       || aMessage.name == "Webapps:Install:Return:OK" || aMessage.name == "Webapps:Uninstall:Return:OK")) {
> > +      let _topic;
> 
> remove the _ prefix.

All of these parts were removed as well.


> 
> @@ +430,5 @@
> >          Services.DOMRequest.fireSuccess(req, convertAppsArray(msg.apps, this._window));
> >          break;
> >        case "Webapps:Install:Return:OK":
> > +        debug("WebApps Install OK");
> > +        debug(this._onuninstall);
> 
> If you are going to keep these debug() stmt's, please join them.

They will all be removed

> 
> ::: dom/apps/src/Webapps.jsm
> @@ +6,5 @@
> > +let DEBUG = 0;
> > +var debug = function (s){};
> > +if (DEBUG) {
> > +  debug = function (s) { dump("-*- Webapps.jsm: " + s + "\n"); };
> > +}
> 
> not:
> 
> var debug = function(s) {
>   if (DEBUG) {
>     dump("-*- Webapps.jsm: " + s + "\n");
>   }
> }

Sorry, this is unclear. Is the first or second approach better? 

> 
> @@ +9,5 @@
> > +  debug = function (s) { dump("-*- Webapps.jsm: " + s + "\n"); };
> > +}
> > +function log(aMsg)
> > +{
> > +  dump("Webapps.jsm: " + aMsg + "\n");
> 
> why have a separate log() function?
> 
debug is 5 letters? - I removed it

> @@ +23,5 @@
> > +      log(">>> pprint recurse into " + prop);
> > +      pprint(aObj[prop], true);
> > +    }
> > +  }
> > +}
> 
> As a sidenote, this should be really be built into the language.
> 

Hear Hear! The only thing I miss from python is better object introspection and pprint


> @@ +55,5 @@
> > +  1: "allow",
> > +  2: "deny",
> > +  3: "prompt",
> > +  4: "prompt-remember"
> > +};
> 
> Do these numbers match anything else is gecko?  If so, it might be nice to
> reference that here.

I addded a comment, they correspond to Ci.nsIPermissionManager.ALLOW_ACTION, etc

> 
> @@ +101,5 @@
> > + * @param string aPermName
> > + * @param string aAccess
> > + * @returns Array
> > + **/
> > +function expandPermissions(aPermName, aAccess)
> 
> ah, this is cool.  I need to probably do this with device storage too.

thx. is it the exact same? should we add it to a utils jsm somewhere?
> 
> @@ +447,5 @@
> > +    // pprint(appObject, true);
> > +
> > +    let manifest = new DOMApplicationManifest(app.manifest, app.origin);
> > +    // check the manifest for permissions
> > +    if (manifest.permissions) {
> 
> return early.

ah, yes. my bad.

> 
> Also, how can this happen and what do we do about it?

The only thing you MUST have in a manifest is the name of the app and where it can be installed from. Permissions are not required. 

> 
> @@ +458,5 @@
> > +        else {
> > +          let perms = [];
> > +          // get access:
> > +          if (manifest.permissions[permName].access) {
> > +            perms = expandPermissions(permName,
> 
> It would kind of be nice if you could call expandPermission even if
> manifest.permissions[permName].access is null/undefined.  In that case, it
> will just return permName.  
> 
Nice!

> If you did this, you could simplify this a bit and just do:
> 
> perms.push(expandPermissions(manifest.permissions, permName));

I think that would push an array onto the array, no? 

> ::: dom/tests/browser/browser_webapps_permissions.js


> 
> @@ +148,5 @@
> > +    gWindow.document.body.focus();
> > +    let notifications = aPopup.target.childNodes;
> > +    let notification = notifications[0];
> > +    EventUtils.synthesizeMouse(notification.button, 20, 10, {});
> > +  });
> 
> This is pretty fragile.  Is there a way we can generate a click on this
> button instead of sending mouse events?
I think so. Lemme poke at that.

Thanks!

Comment 65

[David Dahl :ddahl]

(In reply to Jonas Sicking (:sicking) from comment #45)
> Comment on attachment 655187 [details] [diff] [review]
> Patch 4, test in progress
> 
> Review of attachment 655187 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/src/PermissionsTable.jsm
> @@ +39,5 @@
> > +//                       },
> > +// };
> > +
> > +// // XXXddahl: Offload these permsissions to web activities? Do we even need these listed here? Maybe only for the 'record'?
> > +// const PERMS_TABLE_WEB_ACTIVITIES = {
> 
> I don't think there is any value in a table like this. The fact that an API
> is "available through webactivities" basically mean that the API isn't
> available at all, but that you can get similar functionality using
> WebActivities. So from a Gecko point of view "available through
> webactivities" simply means "not available".

I removed it. I thought I might use something like that, but it seems superfluous now.
> 
> @@ +84,5 @@
> > +// };
> > +
> > +// NOTE: Commented-out permissions are not in the works yet
> > +
> > +const PERMS_TABLE = { // idle: {
> 
> In this table you should also list which properties use an 'access'
> property. I.e. the contacts API has a concept of "read" vs. "write" vs.
> "create", however geolocation and prompt API has no such concept.

I do not think that is documented in the perms matrix, I did add a column for "Granular Perms" 

> 
> @@ +90,5 @@
> > +                      //   app: DENY_ACTION,
> > +                      //   privileged: ALLOW_ACTION,
> > +                      //   certified: ALLOW_ACTION
> > +                      // }, // XXX: Not used in initial verison
> > +                      "battery-status": {
> 
> I think that the APIs which are "allow" everywhere should simply not go
> through any of this code. We should just grant access everywhere.

ok, removed them, added a comment to that effect


> 
> @@ +104,5 @@
> > +                        certified: ALLOW_ACTION
> > +                      },
> > +                      "resource-lock": {
> > +                        content: ALLOW_ACTION,
> > +                        "content-fullscreen": PROMPT_ACTION, // XXXddahl: not sure what this should look like
> 
> This seems reversed. Shouldn't the 'content' part be set to PROMPT and the
> fullscreen be set to ALLOW?

Fixed.

> 
> Hmm.. though is there a point to putting 'content' in this table at all?
> Since content doesn't go through the installation code. Or are you using
> this table somehow elsewhere too?
>
Not using it anywhere else, I just was mimicing what others have documented. I can remove the content property.
 
> @@ +116,5 @@
> > +                        privileged: ALLOW_ACTION,
> > +                        certified: ALLOW_ACTION
> > +                      },
> > +                      "screen-orientation": {
> > +                        content: ALLOW_ACTION, // XXXddahl: Matrix says inplicit (fullscreen-only) - should this be DENY_ACTION??
> 
Fixed it.

> This should be the same as resource-lock
> 
> @@ +132,5 @@
> > +                      geolocation: {
> > +                        content: PROMPT_ACTION,
> > +                        app: PROMPT_ACTION,
> > +                        privileged: PROMPT_ACTION,
> > +                        certified: PROMPT_ACTION
> 
> Actually, the matrix says that this should be 'allow' for certified apps.
> Though personally I'd prefer that this prompted there too, but we should
> stick to the matrix for now.

I changed to to reflect the matrix

> 
> @@ +135,5 @@
> > +                        privileged: PROMPT_ACTION,
> > +                        certified: PROMPT_ACTION
> > +                      },
> > +                      sensor: {
> > +                        content: PROMPT_ACTION,
> 
> This doesn't match what the matrix says.
Fixed it.

> 
> @@ +142,5 @@
> > +                        certified: ALLOW_ACTION
> > +                      },
> > +                      "camera": {
> > +                        content: WEB_ACTIVITIES_ACTION,
> > +                        app: WEB_ACTIVITIES_ACTION,
> 
> See comment above. I think we should simply say "DENY" for these.
> 
Done.

> 
> ::: dom/apps/src/Webapps.jsm
> @@ +95,5 @@
> > +    suffixes.push("create");
> > +    break;
> > +  case READCREATE:
> > +    suffixes.push("read");
> > +    suffixes.push("create");
> 
> suffixes.push("read", "create");
> 
Gah! thanks.

> @@ +404,5 @@
> > +      for (let permName in manifest.permissions) {
> > +        if (!PERMS_TABLE[permName]) {
> > +          Cu.reportError("'" + permName + "'" +
> > +                         " is not a valid Webapps permission type");
> > +          continue;
> 
> I think we should abort if this happens, and not just ignore any unknown
> permissions.

OK, will do an uninstall if we hit this issue

Thanks!

Comment 66

[David Dahl :ddahl]

Attachment: Patch 10, Review comments addressed, some tests failing

Must sanity check all perms to be installed and make sure perm adding still works.

Comment 67

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

Comment on attachment 657507 [details] [diff] [review]
Patch 10, Review comments addressed, some tests failing

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

AFAICT you're still missing code for dealing with the fact that the 'access' property doesn't apply to all permissions. Let me know if you need help with figuring out which permissions it applies to.

::: dom/apps/src/PermissionsTable.jsm
@@ +29,5 @@
> +// XXX TODO: Specify all granular permissions for each permission type: READ, READCREATE, READWRITE, null
> +
> +const PermissionsTable = { "resource-lock": {
> +                             content: PROMPT_ACTION,
> +                             "content-fullscreen": ALLOW_ACTION,

I still don't understand how you are planning on using .content/.content-fullscreen given that those never go through an install step and so there's no opportunity to put anything in the permission manager based on this table.

I think for non-installed content we simply have to write the desired behavior into respective API implementation.

::: dom/apps/src/Webapps.jsm
@@ +101,5 @@
> +  {
> +    let result = [];
> +    for (let idx in aSuffixes) {
> +      result.push(aPermName + "-" + aSuffixes[idx]);
> +    }

return aSuffixes.map(function(suf) { return aPermName + "-" + suf });

@@ +496,5 @@
> +              // Do not overwrite an existing permission
> +              continue;
> +            }
> +          }
> +          let perm = PermissionsTable[permName].app;

Why always .app here? Shouldn't you be getting .app, .privileged or .certified depending on what type of app this is?

Comment 68

[David Dahl :ddahl]

(In reply to Jonas Sicking (:sicking) from comment #67)
> Comment on attachment 657507 [details] [diff] [review]
> Patch 10, Review comments addressed, some tests failing
> 
> Review of attachment 657507 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> AFAICT you're still missing code for dealing with the fact that the 'access'
> property doesn't apply to all permissions. Let me know if you need help with
> figuring out which permissions it applies to.

Correct. I added a column to the matrix called "Granular Permissions". We should get everyone who should know what they are to add them to the matrix. Who should we ask to drive that? I can do more research today as well.

> 
> ::: dom/apps/src/PermissionsTable.jsm
> @@ +29,5 @@
> > +// XXX TODO: Specify all granular permissions for each permission type: READ, READCREATE, READWRITE, null
> > +
> > +const PermissionsTable = { "resource-lock": {
> > +                             content: PROMPT_ACTION,
> > +                             "content-fullscreen": ALLOW_ACTION,
> 
> I still don't understand how you are planning on using
> .content/.content-fullscreen given that those never go through an install
> step and so there's no opportunity to put anything in the permission manager
> based on this table.

This comment and property was put there to get input from reviewers. I don't know. 

> 
> I think for non-installed content we simply have to write the desired
> behavior into respective API implementation.
> 
OK. We should document this for each API - perhaps in the matrix?


> @@ +496,5 @@
> > +              // Do not overwrite an existing permission
> > +              continue;
> > +            }
> > +          }
> > +          let perm = PermissionsTable[permName].app;
> 
> Why always .app here? Shouldn't you be getting .app, .privileged or
> .certified depending on what type of app this is?

I just added some logic for this. New patch is about to be uploaded. Need to check the bitrot.

Comment 69

[David Dahl :ddahl]

Attachment: Patch 11, most comments addressed, all tests passing

Saving work, I am going to write another test that makes sure an app re-install preserves changed permissions, for instance, where 'prompt-remember' is converted to 'allow'

Comment 70

[David Dahl :ddahl]

Ok, a question: the matrix does not mention 'prompt-remember' at all, am I to infer it via 'Prompting Support' && 'Prompting Required' columns - where if 'Prompting Required' we can assume 'PROMPT' always and if not, 'PROMPT REMEMBER'?

Also, the lack of "granular permissions" in the matrix is blocking me right now. Granular Permissions are READ, READCREATE, READWRITE (or all 3) or NONE

Comment 71

[Lucas Adamski [:ladamski]]

PROMPT REMEMBER and PROMPT should be states not permissions.  What I mean by that is if the user is prompted for a permission that can choose to persist the choice they make here, or not respectively.

The permission prompt has a dialog that includes a "remember my choice" toggle.  This toggle is OFF by default for non-privileged apps, and ON by default for privileged apps.

See https://www.dropbox.com/sh/ug5dd6d7rub0p5x/8qm21MMs1m/Gaia_AppSecurity_20120904.pdf for the wireframes.

Comment 72

[David Dahl :ddahl]

(In reply to David Dahl :ddahl from comment #69)
> I am going to write another test that makes sure an app
> re-install preserves changed permissions, for instance, where
> 'prompt-remember' is converted to 'allow'

Ok, this test should be written with the patch in bug 773114 in light of the clarification in comment 71

Comment 73

[David Dahl :ddahl]

Attachment: Patch 12

Adding a new test that makes sure on re-install we remove any permissions not in the updated app's manifest. This test is failing where I try to over-write the manifest between installs of the app:

The overwriting bit looks like:

  const PERMS_FILE = 0644;

  const MODE_WRONLY   = 0x02;
  const MODE_CREATE   = 0x08;
  const MODE_TRUNCATE = 0x20;

  let file = Cc["@mozilla.org/file/directory_service;1"].
             getService(Ci.nsIProperties).
             get("XCurProcD", Ci.nsIFile);
  file.append("test-webapp.webapp");
  let fos = Cc["@mozilla.org/network/file-output-stream;1"].
            createInstance(Ci.nsIFileOutputStream);
  if (!file.exists()) {
    file.create(Ci.nsILocalFile.NORMAL_FILE_TYPE, PERMS_FILE);
  }
  fos.init(file, MODE_WRONLY | MODE_CREATE | MODE_TRUNCATE, PERMS_FILE, 0);
  fos.write(newManifestTxt, newManifestTxt.length);
  fos.close();

I don't think this ever gets over-written or is there an asynchronous write problem? This looks like sync file writing APIs

... firing up gdb ...

Comment 74

[Mounir Lamouri (:mounir)]

Comment on attachment 659299 [details] [diff] [review]
Patch 12

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

Regarding permissions, it seems that it would be better to actually remove all permissions for the app and then install all of those marked in the manifest.

Comment 75

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

As long as we don't remove any DENY entries from the permission manager. If the user once has said "no" to allowing some permission, he/she shouldn't be reasked after an update.

Comment 76

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

Oh, but we likely wouldn't want to remove any ACCEPT either, since the user shouldn't be re-asked about something they have already granted access to.

However we should remove anything that the app no longer lists in the permissions property in the manifest. Including if the permission manager contains an ACCEPT entry for that permission. But probably not if the permission contains a DENY since we want to remember that if a later update requests that permission again.

Comment 77

[Mounir Lamouri (:mounir)]

Indeed! I should have think a bit more of the details before writing that comment...

Comment 78

[David Dahl :ddahl]

Attachment: Patch 13, 1 test failure

Mak:

Curious if you can spot a problem in my test where I attempt to overwrite a manifest file in the same directory where the test is running. It is in dom/tests/webapps/browser/browser_webapps_perms_reinstall.js

The ASSERT mentioned above seems to be unrelated

Comment 79

[David Dahl :ddahl]

Comment on attachment 660243 [details] [diff] [review]
Patch 13, 1 test failure

Gah! It just occurred to me that the file I am trying to copy or move is a symlink.

Comment 80

[David Dahl :ddahl]

Attachment: Patch 14 - All install, uninstall and reinstall tests passing

Finally. With nice browser-chrome-tests that test install, uninstall, and reinstall.

A note: in PermissionSettings.js I have to force permissions setting for these tests to run. SuperPowers seems to not work. I have spoken with gregor about this - Gregor was going to tweak his patch in bug 770731, which this patch depends on.

Comment 81

[David Dahl :ddahl]

Comment on attachment 660593 [details] [diff] [review]
Patch 14 - All install, uninstall and reinstall tests passing

Gregor,

can you revisit the issue on getting permission to set permissions via PermissionSettings.js in bug 770731?

Comment 82

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

Comment on attachment 660593 [details] [diff] [review]
Patch 14 - All install, uninstall and reinstall tests passing

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

::: dom/apps/src/PermissionsTable.jsm
@@ +16,5 @@
> +const UNKNOWN_ACTION = Ci.nsIPermissionManager.UNKNOWN_ACTION;
> +const ALLOW_ACTION = Ci.nsIPermissionManager.ALLOW_ACTION;
> +const DENY_ACTION = Ci.nsIPermissionManager.DENY_ACTION;
> +const PROMPT_ACTION = Ci.nsIPermissionManager.PROMPT_ACTION;
> +const PROMPT_REMEMBER_ACTION = Ci.nsIPermissionManager.PROMPT_REMEMBER_ACTION;

Remove PROMPT_REMEMBER_ACTION. Why not set these constants to string values instead. That way you won't have to do the conversion later.

@@ +26,5 @@
> +
> +// XXX TODO: Specify all granular permissions for each permission type: READ, READCREATE, READWRITE, null
> +
> +const PermissionsTable = { "resource-lock": {
> +                             content: PROMPT_ACTION,

As previously noted, I think you should remove the content properties here. We don't ever go through an install step for plain content and so we won't ever have a chance to look things up in this table. By putting info here we just risk someone thinking that flipping these values to something more secure will actually do something.

@@ +167,5 @@
> +                           },
> +                           attention: {
> +                             content: DENY_ACTION,
> +                             app: DENY_ACTION,
> +                             privileged: ALLOW_ACTION,

This one should be DENY

::: dom/apps/src/Webapps.jsm
@@ +124,5 @@
> + * @param string aPermName
> + * @param string aAccess
> + * @returns Array
> + **/
> +function expandPermissions(aPermName, aAccess)

This function needs to be a lot smarter with regards to the access property.

First of all not all permissions have an access property. See the Permissions.txt file for which ones support an access property.
http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/Permission.txt?force=1

(If the second line is empty, the access property is not permitted.)

Second, for the permissions that do support an access property, if no access property is supplied we should treat it as "readonly".

You should track in the PermissionsTable which properties support access.

@@ +131,5 @@
> +    return [aPermName];
> +  }
> +  function makePermArray(aPermName, aSuffixes)
> +  {
> +    let result = [];

This is unused.

@@ +559,5 @@
> +        for (let perm in newManifest.permissions) {
> +          let _perms = expandPermissions(perm,
> +                                         newManifest.permissions[perm].access);
> +          for (let idx in _perms) {
> +            newPerms.push(_perms[idx]);

just do newPerms = newPerms.concat(_perms);

@@ +563,5 @@
> +            newPerms.push(_perms[idx]);
> +          }
> +        }
> +
> +        for (let perm in aExistingApp.permissions) {

It would be safer to go through all the permissions in PermissionsTable here instead. That way we clear out any permissions that could be leftover from a failed previous install or some such.

@@ +575,5 @@
> +        for (let idx in oldPerms) {
> +          let index = newPerms.indexOf(oldPerms[idx]);
> +          if (index == -1) {
> +            // Remove the deprecated permission
> +            PermSettings.set(oldPerms[idx],

Actually, we should probably add a PermSettings.remove function. Can you file a followup bug on that? Or does setting to "unknown" remove the permissions?

Also, you should not remove a permission if it has been set to DENIED. Otherwise an app can simply update twice to clear all DENIED settings and get to re-prompt the user.

@@ +642,5 @@
> +                         aData.app.manifestURL,
> +                         aData.app.origin,
> +                         permStr,
> +                         false);
> +        aAppObject.permissions[permName] = newManifest.permissions[permName];

Why is this last line needed?

@@ +1029,5 @@
>            continue;
> +
> +        if (this.webapps[record.id].manifest.permissions) {
> +          for (let permName in this.webapps[record.id].permissions) {
> +            // check to see if the permission already exists

I don't understand what this code is trying to do. It looks like it's currently not doing anything.

Comment 83

[David Dahl :ddahl]

(In reply to Jonas Sicking (:sicking) from comment #82)
> Comment on attachment 660593 [details] [diff] [review]
> Patch 14 - All install, uninstall and reinstall tests passing
> 
> Review of attachment 660593 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/src/PermissionsTable.jsm
> @@ +16,5 @@
> > +const UNKNOWN_ACTION = Ci.nsIPermissionManager.UNKNOWN_ACTION;
> > +const ALLOW_ACTION = Ci.nsIPermissionManager.ALLOW_ACTION;
> > +const DENY_ACTION = Ci.nsIPermissionManager.DENY_ACTION;
> > +const PROMPT_ACTION = Ci.nsIPermissionManager.PROMPT_ACTION;
> > +const PROMPT_REMEMBER_ACTION = Ci.nsIPermissionManager.PROMPT_REMEMBER_ACTION;
> 
> Remove PROMPT_REMEMBER_ACTION. Why not set these constants to string values
> instead. That way you won't have to do the conversion later.

I would prefer to use these values in a consistent manner, either as strings or ints, but the api in bug 770731 converts them to strings. 

> 
> @@ +26,5 @@
> > +
> > +// XXX TODO: Specify all granular permissions for each permission type: READ, READCREATE, READWRITE, null
> > +
> > +const PermissionsTable = { "resource-lock": {
> > +                             content: PROMPT_ACTION,
> 
> As previously noted, I think you should remove the content properties here.
> We don't ever go through an install step for plain content and so we won't
> ever have a chance to look things up in this table. By putting info here we
> just risk someone thinking that flipping these values to something more
> secure will actually do something.
> 
> @@ +167,5 @@
> > +                           },
> > +                           attention: {
> > +                             content: DENY_ACTION,
> > +                             app: DENY_ACTION,
> > +                             privileged: ALLOW_ACTION,
> 
> This one should be DENY
> 
> ::: dom/apps/src/Webapps.jsm
> @@ +124,5 @@
> > + * @param string aPermName
> > + * @param string aAccess
> > + * @returns Array
> > + **/
> > +function expandPermissions(aPermName, aAccess)
> 
> This function needs to be a lot smarter with regards to the access property.
> 
> First of all not all permissions have an access property. See the
> Permissions.txt file for which ones support an access property.
> http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/Permission.
> txt?force=1
> 
> (If the second line is empty, the access property is not permitted.)
> 
> Second, for the permissions that do support an access property, if no access
> property is supplied we should treat it as "readonly".
> 
> You should track in the PermissionsTable which properties support access.
> 
ok,added an access property where needed per permission, also increased the scrutiny of each expandpermissions operation. The validity of each permission requested is checked. One issue I have now is that the expanded permissions cannot be tested as there are no permssions that expand for webapps, only certified and privileged apps. Hmm, can I specify in the manifest that the app being installed is certified?

> @@ +131,5 @@
> > +    return [aPermName];
> > +  }
> > +  function makePermArray(aPermName, aSuffixes)
> > +  {
> > +    let result = [];
> 
> This is unused.
> 

removed

> @@ +559,5 @@
> > +        for (let perm in newManifest.permissions) {
> > +          let _perms = expandPermissions(perm,
> > +                                         newManifest.permissions[perm].access);
> > +          for (let idx in _perms) {
> > +            newPerms.push(_perms[idx]);
> 
> just do newPerms = newPerms.concat(_perms);
>

got it.
 
> @@ +563,5 @@
> > +            newPerms.push(_perms[idx]);
> > +          }
> > +        }
> > +
> > +        for (let perm in aExistingApp.permissions) {
> 
> It would be safer to go through all the permissions in PermissionsTable here
> instead. That way we clear out any permissions that could be leftover from a
> failed previous install or some such.

Ok, cool. That is comprehensive.

> 
> @@ +575,5 @@
> > +        for (let idx in oldPerms) {
> > +          let index = newPerms.indexOf(oldPerms[idx]);
> > +          if (index == -1) {
> > +            // Remove the deprecated permission
> > +            PermSettings.set(oldPerms[idx],
> 
> Actually, we should probably add a PermSettings.remove function. Can you
> file a followup bug on that? Or does setting to "unknown" remove the
> permissions?

I asked about this in bug 770731, but I will file a bug nonetheless. I tried to add a mechanism to handle that in PermissionPromptHelper.jsm to handle removal in add() if a perm is going to be set to 'unknown' and already exists. I revisited the code and now it works. 

> 
> Also, you should not remove a permission if it has been set to DENIED.
> Otherwise an app can simply update twice to clear all DENIED settings and
> get to re-prompt the user.

Ok, not sure if this should be added to installPermissions (in the 'reinstall' code path) or in PermissionPromptHelper's 'addPermission'. I worry that a new manifest without a previously-denied permission will not remove the denied permission, and a later manifest that re-introduces the permission as 'allow' will not reset it away from 'denied'. My mind is twisted right now, need more thought on this.:)   
> 
> @@ +642,5 @@
> > +                         aData.app.manifestURL,
> > +                         aData.app.origin,
> > +                         permStr,
> > +                         false);
> > +        aAppObject.permissions[permName] = newManifest.permissions[permName];
> 
> Why is this last line needed?

I was parroting existing code, where properties are updated into the appObject inside confirmInstall()
> 
> @@ +1029,5 @@
> >            continue;
> > +
> > +        if (this.webapps[record.id].manifest.permissions) {
> > +          for (let permName in this.webapps[record.id].permissions) {
> > +            // check to see if the permission already exists
> 
> I don't understand what this code is trying to do. It looks like it's
> currently not doing anything.

That is unneeded, removed. Thanks!

Uploading a new patch momentarily...

Comment 84

[David Dahl :ddahl]

Attachment: Patch 15, comments addressed, new issues

Comment 85

[Mounir Lamouri (:mounir)]

Comment on attachment 661030 [details] [diff] [review]
Patch 15, comments addressed, new issues

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

::: dom/apps/src/PermissionsTable.jsm
@@ +36,5 @@
> +                             app: ALLOW_ACTION,
> +                             privileged: ALLOW_ACTION,
> +                             certified: ALLOW_ACTION
> +                           },
> +                           "screen-orientation": {

There is no such permission.

@@ +46,5 @@
> +                             app: PROMPT_ACTION,
> +                             privileged: PROMPT_ACTION,
> +                             certified: ALLOW_ACTION
> +                           },
> +                           sensor: {

Never heard about that one.

@@ +61,5 @@
> +                             app: ALLOW_ACTION,
> +                             privileged: ALLOW_ACTION,
> +                             certified: ALLOW_ACTION
> +                           },
> +                           "background-service": {

Nor this one.

@@ +152,5 @@
> +                             app: ALLOW_ACTION,     // Matrix indicates '?'
> +                             privileged: ALLOW_ACTION,
> +                             certified: ALLOW_ACTION
> +                           },
> +                           attention: {

What's that?

Comment 86

[Lucas Adamski [:ladamski]]

https://mxr.mozilla.org/mozilla-central/source/extensions/cookie/Permission.txt

Comment 87

[David Dahl :ddahl]

(In reply to Mounir Lamouri (:mounir) from comment #85)
> Comment on attachment 661030 [details] [diff] [review]
> Patch 15, comments addressed, new issues
> 
 +                           attention: {
> 
> What's that?

All of these were at one point defined here: https://docs.google.com/spreadsheet/ccc?key=0Akyz_Bqjgf5pdENVekxYRjBTX0dCXzItMnRyUU1RQ0E#gid=0

Some may have changed names

Comment 88

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

(In reply to David Dahl :ddahl from comment #83)
> > Also, you should not remove a permission if it has been set to DENIED.
> > Otherwise an app can simply update twice to clear all DENIED settings and
> > get to re-prompt the user.
> 
> Ok, not sure if this should be added to installPermissions (in the
> 'reinstall' code path) or in PermissionPromptHelper's 'addPermission'. I
> worry that a new manifest without a previously-denied permission will not
> remove the denied permission, and a later manifest that re-introduces the
> permission as 'allow' will not reset it away from 'denied'. My mind is
> twisted right now, need more thought on this.:)   

I think this is actually the behavior we want.

If we end up with DENY in the permission database then it's because the user has explicitly said "I want to deny this permission permanently" through UI. Either though the prompt, or through the settings application.

If a later version of the app wants to use that capability, we should still honor the user's explicit decision.

We have talked about having some sort of statusbar-notification to users that an app is trying to use a permission which the user has denied, so that the user can be reminded that they made such a decision and that this might be affecting the behavior of the app. However that's orthogonal to updates etc since it can just as much happen if the user has had an app installed for a longer time.

But this isn't something we should worry about for v1.

Comment 89

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

Comment on attachment 661030 [details] [diff] [review]
Patch 15, comments addressed, new issues

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

This mostly looks right

::: dom/apps/src/PermissionsTable.jsm
@@ +66,5 @@
> +                             app: ALLOW_ACTION,
> +                             privileged: ALLOW_ACTION,
> +                             certified: ALLOW_ACTION
> +                           },
> +                           "tcp-socket": {

This should be called network-tcp

@@ +77,5 @@
> +                             privileged: PROMPT_ACTION,
> +                             certified: ALLOW_ACTION,
> +                             access: ["contacts-read",
> +                                      "contacts-write",
> +                                      "contacts-create"

Nit, seems simpler to just put "read", "write" and "create" in there and let the prefix be implicit.

@@ +152,5 @@
> +                             app: ALLOW_ACTION,     // Matrix indicates '?'
> +                             privileged: ALLOW_ACTION,
> +                             certified: ALLOW_ACTION
> +                           },
> +                           attention: {

I actually think we have this one. It's for the ability of a certified app to overlay an attention page on top of the screen. It's used by the telephony app and the alarm app I believe.

::: dom/apps/src/Webapps.jsm
@@ +103,5 @@
> +  let manifestStatus = Ci.nsIPrincipal.APP_STATUS_INSTALLED;
> +
> +  switch(type) {
> +  case "web":
> +    manifestStatus = Ci.nsIPrincipal.APP_STATUS_INSTALLED;

Nit: I'd just return right here.

@@ +128,5 @@
> +function expandPermissions(aPermName, aAccess)
> +{
> +  if (!PermissionsTable[aPermName]) {
> +    Cu.reportError("Unknown permission: " + aPermName);
> +    return []; // XXXddahl: throw an error and cease install/reinstall?

Yeah, throwing seems safer. We should never be getting here with an unknown permission name, right?

@@ +131,5 @@
> +    Cu.reportError("Unknown permission: " + aPermName);
> +    return []; // XXXddahl: throw an error and cease install/reinstall?
> +  }
> +  if (!aAccess && !PermissionsTable[aPermName].access) {
> +    return [aPermName];

You don't need the !aAccess check here. In fact if aAccess isn't empty when PermissionsTable[aPermName].access is set, we should probably treat that as an invalid manifest. That should be checked at the same place that we check for unknown permission types.

@@ +136,5 @@
> +  }
> +
> +  function makePermArray(aPermName, aSuffixes)
> +  {
> +    let result = [];

This guy is still here :)

@@ +158,5 @@
> +  default:
> +    return [];
> +  }
> +
> +  let permArr =  makePermArray(aPermName, requestedSuffixes);

Nit: Whitespace too much after '='

@@ +159,5 @@
> +    return [];
> +  }
> +
> +  let permArr =  makePermArray(aPermName, requestedSuffixes);
> +  if (!(permArr.length <= PermissionsTable[aPermName].access)) {

This isn't right. You should simply filter the permissions based on what's in the permission table. Otherwise someone won't be able to get "readwrite" permission to settings.

Actually, just remove this check given that you have the loop below.

@@ +168,5 @@
> +  let expandedPerms = [];
> +  for (let idx in permArr) {
> +    let index = PermissionsTable[aPermName].access.indexOf(permArr[idx]);
> +    if (index != -1) {
> +      Cu.reportError("Requested permission access does not exist. " +

And just ignore the permission here. Otherwise the "settings" permission won't work.

@@ +583,5 @@
> +                                         newManifest.permissions[perm].access);
> +          newPerms = newPerms.concat(_perms);
> +        }
> +
> +        for (let idx in AllPossiblePermissions) {

You could just do a for-in loop in PermissionsTable here instead. That way you don't need the extra AllPossiblePermissions array

@@ +589,5 @@
> +          debug("permission: " + AllPossiblePermissions[idx]);
> +          if (index == -1) {
> +            debug("removing: " + AllPossiblePermissions[idx] + "\n\n");
> +            // Remove the deprecated permission
> +            PermSettings.set(AllPossiblePermissions[idx],

Only do this if the permission isn't DENY

@@ +612,5 @@
> +      }
> +
> +      let perms = expandPermissions(permName,
> +                                    newManifest.permissions[permName].access);
> +      let existingPerms = {};

What's the purpose of this variable? You put a bunch of data into it, but you never actually use it.

@@ +622,5 @@
> +                                       null,
> +                                       aData.app.manifestURL,
> +                                       aData.app.origin,
> +                                       false);
> +          if ((_perm in EXISTING_PERMS)) {

Might be simpler to simply do _perm != "unknown" here. That way you can also get rid of EXISTING_PERMS

@@ +646,5 @@
> +        default:
> +          // Cannot determine app type, abort install
> +          this.uninstall(aData);
> +          Cu.reportError("Webapps.jsm: Cannot determine app type, install cancelled");
> +          return;

Hmm.. Rather than spreading these uninstall calls all over installPermissions, maybe have a top-leve try-catch and throw the error message, and have the catch report the error and call uninstall.

::: dom/permission/PermissionPromptHelper.jsm
@@ +111,5 @@
> +    debug("remove permission? ",  perm, aData.type, action);
> +    if (perm != Ci.nsIPermissionManager.UNKNOWN_ACTION &&
> +        action == Ci.nsIPermissionManager.UNKNOWN_ACTION) {
> +      debug(">>> remove permission: ",  perm, aData.type, action);
> +      permissionManager.removeFromPrincipal(principal, aData.type);

Why is this needed?

Comment 90

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

> > Why not set these constants to string values
> > instead. That way you won't have to do the conversion later.
> 
> I would prefer to use these values in a consistent manner, either as strings
> or ints, but the api in bug 770731 converts them to strings. 

I'm suggesting to consistently use the string values :)

Comment 91

[Mounir Lamouri (:mounir)]

(In reply to Lucas Adamski from comment #86)
> https://mxr.mozilla.org/mozilla-central/source/extensions/cookie/Permission.
> txt

(In reply to David Dahl :ddahl from comment #87)
> (In reply to Mounir Lamouri (:mounir) from comment #85)
> > Comment on attachment 661030 [details] [diff] [review]
> > Patch 15, comments addressed, new issues
> > 
>  +                           attention: {
> > 
> > What's that?
> 
> All of these were at one point defined here:
> https://docs.google.com/spreadsheet/
> ccc?key=0Akyz_Bqjgf5pdENVekxYRjBTX0dCXzItMnRyUU1RQ0E#gid=0
> 
> Some may have changed names

'sensors' and 'screen-orientation' do not appear in Permission.txt. Also, those permissions simply don't exist.
Also, I never heard of any use of a permission called 'background-services'. AFAIK, there will be no such thing in v1.

Comment 92

[David Dahl :ddahl]

(In reply to Jonas Sicking (:sicking) from comment #89)
> Comment on attachment 661030 [details] [diff] [review]
> Patch 15, comments addressed, new issues
> 
> Review of attachment 661030 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 
> @@ +77,5 @@
> > +                             privileged: PROMPT_ACTION,
> > +                             certified: ALLOW_ACTION,
> > +                             access: ["contacts-read",
> > +                                      "contacts-write",
> > +                                      "contacts-create"
> 
> Nit, seems simpler to just put "read", "write" and "create" in there and let
> the prefix be implicit.

I did this because it makes the check against privileges that need removal a quick, uncomplicated operation.
> 
> @@ +152,5 @@
> > +                             app: ALLOW_ACTION,     // Matrix indicates '?'
> > +                             privileged: ALLOW_ACTION,
> > +                             certified: ALLOW_ACTION
> > +                           },
> > +                           attention: {
> 
> I actually think we have this one. It's for the ability of a certified app
> to overlay an attention page on top of the screen. It's used by the
> telephony app and the alarm app I believe.
> 
ok, leaving it in for now

> ::: dom/apps/src/Webapps.jsm
> @@ +103,5 @@
> > +  let manifestStatus = Ci.nsIPrincipal.APP_STATUS_INSTALLED;
> > +
> > +  switch(type) {
> > +  case "web":
> > +    manifestStatus = Ci.nsIPrincipal.APP_STATUS_INSTALLED;
> 
> Nit: I'd just return right here.

I only moved this function here so it can be used again by my code as it was inside another function, not sure if that change will adversely affect the original caller

> 
> @@ +128,5 @@
> > +function expandPermissions(aPermName, aAccess)
> > +{
> > +  if (!PermissionsTable[aPermName]) {
> > +    Cu.reportError("Unknown permission: " + aPermName);
> > +    return []; // XXXddahl: throw an error and cease install/reinstall?
> 
> Yeah, throwing seems safer. We should never be getting here with an unknown
> permission name, right?

I updated the patch to throw.

> 
> @@ +131,5 @@
> > +    Cu.reportError("Unknown permission: " + aPermName);
> > +    return []; // XXXddahl: throw an error and cease install/reinstall?
> > +  }
> > +  if (!aAccess && !PermissionsTable[aPermName].access) {
> > +    return [aPermName];
> 
> You don't need the !aAccess check here. In fact if aAccess isn't empty when
> PermissionsTable[aPermName].access is set, we should probably treat that as
> an invalid manifest. That should be checked at the same place that we check
> for unknown permission types.

I think we need both as we will still need to return just [aPermName] when !aAccess && aPermName exists in PermissionsTable

> 
> @@ +136,5 @@
> > +  }
> > +
> > +  function makePermArray(aPermName, aSuffixes)
> > +  {
> > +    let result = [];
> 
> This guy is still here :)
> 
Gah! thx!


> > +  let permArr =  makePermArray(aPermName, requestedSuffixes);
> > +  if (!(permArr.length <= PermissionsTable[aPermName].access)) {
> 
> This isn't right. You should simply filter the permissions based on what's
> in the permission table. Otherwise someone won't be able to get "readwrite"
> permission to settings.
> 
> Actually, just remove this check given that you have the loop below.
> 
> @@ +168,5 @@
> > +  let expandedPerms = [];
> > +  for (let idx in permArr) {
> > +    let index = PermissionsTable[aPermName].access.indexOf(permArr[idx]);
> > +    if (index != -1) {
> > +      Cu.reportError("Requested permission access does not exist. " +
> 
> And just ignore the permission here. Otherwise the "settings" permission
> won't work.
I am not sure if I understand, plus, I updated this again before I go this feedback.  

> 
> @@ +583,5 @@
> > +                                         newManifest.permissions[perm].access);
> > +          newPerms = newPerms.concat(_perms);
> > +        }
> > +
> > +        for (let idx in AllPossiblePermissions) {
> 
> You could just do a for-in loop in PermissionsTable here instead. That way
> you don't need the extra AllPossiblePermissions array

I tried to do that but feel like it adds more embedded loops and the code is much simpler to understand if we pre-configure an AllPossiblePermissions array. This also makes the explicit access property array useful instead of having to re-concatenate "perm-" + read, etc.
> 
> @@ +589,5 @@
> > +          debug("permission: " + AllPossiblePermissions[idx]);
> > +          if (index == -1) {
> > +            debug("removing: " + AllPossiblePermissions[idx] + "\n\n");
> > +            // Remove the deprecated permission
> > +            PermSettings.set(AllPossiblePermissions[idx],
> 
> Only do this if the permission isn't DENY

OK

> 
> @@ +612,5 @@
> > +      }
> > +
> > +      let perms = expandPermissions(permName,
> > +                                    newManifest.permissions[permName].access);
> > +      let existingPerms = {};
> 
> What's the purpose of this variable? You put a bunch of data into it, but
> you never actually use it.
> 
It was previously used, I removed it


> @@ +622,5 @@
> > +                                       null,
> > +                                       aData.app.manifestURL,
> > +                                       aData.app.origin,
> > +                                       false);
> > +          if ((_perm in EXISTING_PERMS)) {
> 
> Might be simpler to simply do _perm != "unknown" here. That way you can also
> get rid of EXISTING_PERMS

Gotten rid of, but I did some things differently based on my own revisiting og this code and your comments

> 
> @@ +646,5 @@
> > +        default:
> > +          // Cannot determine app type, abort install
> > +          this.uninstall(aData);
> > +          Cu.reportError("Webapps.jsm: Cannot determine app type, install cancelled");
> > +          return;
> 
> Hmm.. Rather than spreading these uninstall calls all over
> installPermissions, maybe have a top-leve try-catch and throw the error
> message, and have the catch report the error and call uninstall.

Ok, sounds good.

> 
> ::: dom/permission/PermissionPromptHelper.jsm
> @@ +111,5 @@
> > +    debug("remove permission? ",  perm, aData.type, action);
> > +    if (perm != Ci.nsIPermissionManager.UNKNOWN_ACTION &&
> > +        action == Ci.nsIPermissionManager.UNKNOWN_ACTION) {
> > +      debug(">>> remove permission: ",  perm, aData.type, action);
> > +      permissionManager.removeFromPrincipal(principal, aData.type);
> 
> Why is this needed?

I was trying to put in a stop-gap for not having a PermissionSettings.remove method. I can remove this if you like. new patch uploading soon.

Comment 93

[David Dahl :ddahl]

Attachment: Patch 16

Added a removePerms() method for the uninstall() routine. The browser_webapps_perms_reinstall.js tests all pass now, but the uninstall tests fail from browser_webapps_permissions.js:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_permissions.js | uninstall: device-storage-read is unknown - Got unknown, expected deny
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_permissions.js | uninstall: device-storage-read is unknown - Got unknown, expected deny
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_permissions.js | uninstall: device-storage-read is unknown - Got unknown, expected deny

Trying to track down why the uninstall routine shows the same permission 3X. Maybe another set of eyes can tell me - I bet it is a problem in expandPermissions()

Comment 94

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

(In reply to David Dahl :ddahl from comment #92)
> (In reply to Jonas Sicking (:sicking) from comment #89)
> > Comment on attachment 661030 [details] [diff] [review]
> > Patch 15, comments addressed, new issues
> > 
> > Review of attachment 661030 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > 
> > @@ +77,5 @@
> > > +                             privileged: PROMPT_ACTION,
> > > +                             certified: ALLOW_ACTION,
> > > +                             access: ["contacts-read",
> > > +                                      "contacts-write",
> > > +                                      "contacts-create"
> > 
> > Nit, seems simpler to just put "read", "write" and "create" in there and let
> > the prefix be implicit.
> 
> I did this because it makes the check against privileges that need removal a
> quick, uncomplicated operation.

Simply filter before appending the "contacts-" prefix.


> > ::: dom/apps/src/Webapps.jsm
> > @@ +103,5 @@
> > > +  let manifestStatus = Ci.nsIPrincipal.APP_STATUS_INSTALLED;
> > > +
> > > +  switch(type) {
> > > +  case "web":
> > > +    manifestStatus = Ci.nsIPrincipal.APP_STATUS_INSTALLED;
> > 
> > Nit: I'd just return right here.
> 
> I only moved this function here so it can be used again by my code as it was
> inside another function, not sure if that change will adversely affect the
> original caller

I'm fine with having the function live where it lives. But rather than

function () {
  ...
  x = null;
  switch (y) {
    case 1: x = ...;
    case 2: x = ...;
    case 3: x = ...;
  }
  return x;
}

simply do

function () {
  ...
  switch (y) {
    case 1: return ...;
    case 2: return ...;
    case 3: return ...;
  }
}


> > @@ +131,5 @@
> > > +    Cu.reportError("Unknown permission: " + aPermName);
> > > +    return []; // XXXddahl: throw an error and cease install/reinstall?
> > > +  }
> > > +  if (!aAccess && !PermissionsTable[aPermName].access) {
> > > +    return [aPermName];
> > 
> > You don't need the !aAccess check here. In fact if aAccess isn't empty when
> > PermissionsTable[aPermName].access is set, we should probably treat that as
> > an invalid manifest. That should be checked at the same place that we check
> > for unknown permission types.
> 
> I think we need both as we will still need to return just [aPermName] when
> !aAccess && aPermName exists in PermissionsTable

If someone specifies in the manifest:

permissions: {
  browser: {
    description: "...",
    access: "readwrite"
  }
}

then we should treat that as an invalid manifest. Just like if someone specifies

permissions: {
  foopy: {
    description: "...",
  }
}

Currently the code will catch and reject the latter. However the former is not properly rejected.

Once you fix that I believe you can remove "!aAccess && " from the above if statement.


> > > +  let permArr =  makePermArray(aPermName, requestedSuffixes);
> > > +  if (!(permArr.length <= PermissionsTable[aPermName].access)) {
> > 
> > This isn't right. You should simply filter the permissions based on what's
> > in the permission table. Otherwise someone won't be able to get "readwrite"
> > permission to settings.
> > 
> > Actually, just remove this check given that you have the loop below.
> > 
> > @@ +168,5 @@
> > > +  let expandedPerms = [];
> > > +  for (let idx in permArr) {
> > > +    let index = PermissionsTable[aPermName].access.indexOf(permArr[idx]);
> > > +    if (index != -1) {
> > > +      Cu.reportError("Requested permission access does not exist. " +
> > 
> > And just ignore the permission here. Otherwise the "settings" permission
> > won't work.
> I am not sure if I understand, plus, I updated this again before I go this
> feedback.  

How would someone get read/write access to settings with the code as it is? If they put

settings: {
 access: "readonly"
}

they only get read access.

If they put

settings: {
 access: "readwrite"
}

they will get an error since permArr.length will be 3 and PermissionsTable[aPermName].access.length will be 2.

What they should put in the manifest is

settings: {
 access: "readwrite"
}

which should grant them "settings-read" and "settings-write" and nothing else.

> > @@ +583,5 @@
> > > +                                         newManifest.permissions[perm].access);
> > > +          newPerms = newPerms.concat(_perms);
> > > +        }
> > > +
> > > +        for (let idx in AllPossiblePermissions) {
> > 
> > You could just do a for-in loop in PermissionsTable here instead. That way
> > you don't need the extra AllPossiblePermissions array
> 
> I tried to do that but feel like it adds more embedded loops and the code is
> much simpler to understand if we pre-configure an AllPossiblePermissions
> array. This also makes the explicit access property array useful instead of
> having to re-concatenate "perm-" + read, etc.

I don't understand what you mean. It would be the same number of loops. Just change

for (let idx in AllPossiblePermissions) {
  let index = newPerms.indexOf(AllPossiblePermissions[idx]);

to

for (let permName in PermissionsTable) {
  let index = newPerms.indexOf(permName);

Comment 95

[David Dahl :ddahl]

(In reply to Jonas Sicking (:sicking) from comment #94)
> (In reply to David Dahl :ddahl from comment #92)
> > (In reply to Jonas Sicking (:sicking) from comment #89)
> > > Comment on attachment 661030 [details] [diff] [review]
> > > Patch 15, comments addressed, new issues
> > > 
> > > Review of attachment 661030 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > 
> > > @@ +77,5 @@
> > > > +                             privileged: PROMPT_ACTION,
> > > > +                             certified: ALLOW_ACTION,
> > > > +                             access: ["contacts-read",
> > > > +                                      "contacts-write",
> > > > +                                      "contacts-create"
> > > 
> > > Nit, seems simpler to just put "read", "write" and "create" in there and let
> > > the prefix be implicit.
> > 
> > I did this because it makes the check against privileges that need removal a
> > quick, uncomplicated operation.
> 
> Simply filter before appending the "contacts-" prefix.
> 

Ok, I have changed this to only require ["read", "write"], etc.

>
> Currently the code will catch and reject the latter. However the former is
> not properly rejected.
> 
> Once you fix that I believe you can remove "!aAccess && " from the above if
> statement.
> 
Gotcha, fixed.

> How would someone get read/write access to settings with the code as it is?
> If they put
> 
> settings: {
>  access: "readonly"
> }
> 
> they only get read access.
> 
> If they put
> 
> settings: {
>  access: "readwrite"
> }
> 
> they will get an error since permArr.length will be 3 and
> PermissionsTable[aPermName].access.length will be 2.
> 
> What they should put in the manifest is
> 
> settings: {
>  access: "readwrite"
> }
> 
> which should grant them "settings-read" and "settings-write" and nothing
> else.
> 
Ok, fixed it.


> I don't understand what you mean. It would be the same number of loops. Just
> change
> 
> for (let idx in AllPossiblePermissions) {
>   let index = newPerms.indexOf(AllPossiblePermissions[idx]);
> 
> to
> 
> for (let permName in PermissionsTable) {
>   let index = newPerms.indexOf(permName);

I also needed a helper function to do the mapping, but it is inside Webapps.jsm. I removed AllPossiblePermissions from the PermissionsTable, instead building it on the fly.

Now, I am having a problem that might be an async issue where I want to remove certain permissions, but the time it takes to get(), then optionally set() 'non-deny' permissions to 'unknown' takes too long - in the mean time I think the record of the webapp's ID is removed and the nsIPermissionManager lookup fails in PermissionSettings.jsm. This is inside Webapps.jsm's removePermissions()

uploading latest patch now

Comment 96

[David Dahl :ddahl]

Attachment: Patch 17

Comment 97

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

> I also needed a helper function to do the mapping, but it is inside
> Webapps.jsm. I removed AllPossiblePermissions from the PermissionsTable,
> instead building it on the fly.

Ooh, I had missed that you were also expanding permissions. Maybe leave AllPossiblePermissions the way it was. Sorry to make you run in circles :(

> Now, I am having a problem that might be an async issue where I want to
> remove certain permissions, but the time it takes to get(), then optionally
> set() 'non-deny' permissions to 'unknown' takes too long - in the mean time
> I think the record of the webapp's ID is removed and the
> nsIPermissionManager lookup fails in PermissionSettings.jsm. This is inside
> Webapps.jsm's removePermissions()

Good catch!

Though, can we avoid any get-after-set? For permissions *not* in the manifest you need to "check if it's set to deny, otherwise remove". For permissions in the manifest you need to "check if set at all, otherwise set to new value".

Or are tests failing due to the async behavior?

Comment 98

[David Dahl :ddahl]

(In reply to Jonas Sicking (:sicking) from comment #97)
> > I also needed a helper function to do the mapping, but it is inside
> > Webapps.jsm. I removed AllPossiblePermissions from the PermissionsTable,
> > instead building it on the fly.
> 
> Ooh, I had missed that you were also expanding permissions. Maybe leave
> AllPossiblePermissions the way it was. Sorry to make you run in circles :(

Ok, np.

> 
> > Now, I am having a problem that might be an async issue where I want to
> > remove certain permissions, but the time it takes to get(), then optionally
> > set() 'non-deny' permissions to 'unknown' takes too long - in the mean time
> > I think the record of the webapp's ID is removed and the
> > nsIPermissionManager lookup fails in PermissionSettings.jsm. This is inside
> > Webapps.jsm's removePermissions()
> 
> Good catch!
> 
> Though, can we avoid any get-after-set? For permissions *not* in the
> manifest you need to "check if it's set to deny, otherwise remove". For
> permissions in the manifest you need to "check if set at all, otherwise set
> to new value".
> 
> Or are tests failing due to the async behavior?

I think the tests are failing due to the async behavior, we cannot positively identify the app by ID because the ID has been removed from the webapps 'registry', which means we remove the perms we need removed, but cannot actually verify. I will add an observer for uninstall to be called after removePermissions is complete.

Comment 99

[David Dahl :ddahl]

(In reply to David Dahl :ddahl from comment #98)
\
> I think the tests are failing due to the async behavior, we cannot
> positively identify the app by ID because the ID has been removed from the
> webapps 'registry', which means we remove the perms we need removed, but
> cannot actually verify. I will add an observer for uninstall to be called
> after removePermissions is complete.

No. I am incorrect. The PermissionSettings.get is just not returning anything when i call it during removePermissions():

let permType = PermSettings.get(perms[idx],
                                        null, // TODO: remove arg once bug 770731 is updated
                                        aApp.manifestURL,
                                        aApp.origin,
                                        false);
'permType' is always 'unknown'

Comment 100

[David Dahl :ddahl]

So gdb tells me that the permissions are removed long before removePermissions() is ever called. I added a call to Math.sin and breakpoint right inside of removePermissions(), then checked with sqlite3 via the shell to see that the permissions (except for the permission permission) were already deleted. Another well-placed Math.sin call before uninstall() is called tells me that the permissions were indeed added correctly. Something else is removing the permissions here prematurely.

Comment 101

[David Dahl :ddahl]

http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.h#194

This makes me think the platform is just clearing all permissions on uninstall:

http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#146

A brute force removal of all permissions for the app. @#$#$%$!!

Comment 102

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

Yes. Uninstalling an app removes all associated permissions. But we don't call uninstall on update, right?

Comment 103

[David Dahl :ddahl]

(In reply to Jonas Sicking (:sicking) from comment #102)
> Yes. Uninstalling an app removes all associated permissions. But we don't
> call uninstall on update, right?

No, but we do on uninstall. the only reason I caught this is that the install/uninstall tests started failing when I began testing with a permission that is always set to deny for a web app. It always came back "unknown" after uninstall. I was under the impression that a "deny" permission should always be left in the database, even after an uninstall, is that incorrect?

Comment 104

[David Dahl :ddahl]

After rebasing with the patch from bug 770731 I now get this error:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_permissions.js | an unexpected uncaught JS exception reported through window.onerror - Error: Permission denied to access object at :0

http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/AccessCheck.cpp#35

Comment 105

[David Dahl :ddahl]

Attachment: Patch 18

Ok, this patch is rebased against the latest PermissionSettings patch in bug 770731. Just before the rebase, all tests were passing - with the theory in mind that we don't need to keep any DENY permissions after 'uninstall'. I am clobbering because of the errors above related to the ion monkey merge

Comment 106

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

(In reply to David Dahl :ddahl from comment #103)
> (In reply to Jonas Sicking (:sicking) from comment #102)
> > Yes. Uninstalling an app removes all associated permissions. But we don't
> > call uninstall on update, right?
> 
> No, but we do on uninstall. the only reason I caught this is that the
> install/uninstall tests started failing when I began testing with a
> permission that is always set to deny for a web app. It always came back
> "unknown" after uninstall. I was under the impression that a "deny"
> permission should always be left in the database, even after an uninstall,
> is that incorrect?

Yes, that is incorrect. On explicit user uninstall we should nuke all traces of it everywhere. Including deny records in the permission database.

Updates shouldn't be nuking deny records since it would be unexpected for the user that we all of a sudden start asking about something he explicitly said no to. The fact that uninstalling and reinstalling an app causes prompts to come back is hopefully not surprising to the user.

(As an aside, even if we did keep the deny records on uninstall the app wouldn't get the same AppID on reinstall so the records would be effectively lost anyway)

Comment 107

[David Dahl :ddahl]

(In reply to Jonas Sicking (:sicking) from comment #106)
> (In reply to David Dahl :ddahl from comment #103)

> Yes, that is incorrect. On explicit user uninstall we should nuke all traces
> of it everywhere. Including deny records in the permission database.
> 
OK
> Updates shouldn't be nuking deny records since it would be unexpected for
> the user that we all of a sudden start asking about something he explicitly
> said no to. The fact that uninstalling and reinstalling an app causes
> prompts to come back is hopefully not surprising to the user.
> 
I got it
> (As an aside, even if we did keep the deny records on uninstall the app
> wouldn't get the same AppID on reinstall so the records would be effectively
> lost anyway)
I should have realized this. Sorry for the confusion. The new patch should do all of the above. 

Of course since rebasing against the latest patch in bug 770731 and clobbering, I still see:
 Console message: [JavaScript Error: "Error: Permission denied to access object"]
TEST-INFO | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_permissions.js | Console message: [JavaScript Error: "Error: "]

which may also have something to do with the 'permissions' permission? Gregor said something about fixing something in his patch when he tested this...

Comment 108

[David Dahl :ddahl]

I just noticed one of these checks failed:
TEST-PASS | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_permissions.js | we have a mozApps property
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_permissions.js | mozPermissions is available

So we no longer have access to this during the browser chrome test... here are the relevant bits from the test:

    SpecialPowers.setBoolPref("dom.mozApps.dev_mode", true);
    SpecialPowers.setBoolPref("dom.mozPermissionSettings.enabled", true);
    SpecialPowers.addPermission("permissions", true, browser.contentWindow.document);
    SpecialPowers.addPermission("permissions", true, browser.contentDocument);

      gWindow.focus();
      var nav = XPCNativeWrapper.unwrap(browser.contentWindow.navigator);
      ok(nav.mozApps, "we have a mozApps property");
      var navMozPerms = nav.mozPermissionSettings;
      ok(navMozPerms, "mozPermissions is available");

Comment 109

[Gregor Wagner [:gwagner]]

> 
> Of course since rebasing against the latest patch in bug 770731 and
> clobbering, I still see:
>  Console message: [JavaScript Error: "Error: Permission denied to access
> object"]
> TEST-INFO |
> chrome://mochitests/content/browser/dom/tests/browser/
> browser_webapps_permissions.js | Console message: [JavaScript Error: "Error:
> "]
> 
> which may also have something to do with the 'permissions' permission?
> Gregor said something about fixing something in his patch when he tested
> this...

This is my mistake and it should be fixed once I push bug 792604.

Comment 110

[David Dahl :ddahl]

Attachment: Patch 19

Rebased on latest r+'d patch from bug 770731

(Also, this patch fixes a message manager call, mentioned here: https://bugzilla.mozilla.org/show_bug.cgi?id=792882#c0 ) - I can break this out if need be

Comment 111

[David Dahl :ddahl]

Attachment: Patch 19 again

A few tweaks, removed some braces I added, etc...

Comment 112

[Gregor Wagner [:gwagner]]

># HG changeset patch
># Parent 80fdad48c4b5a7b72c2e38876557cb761654ee9b
>
>diff --git a/dom/apps/src/AppsUtils.jsm b/dom/apps/src/AppsUtils.jsm
>--- a/dom/apps/src/AppsUtils.jsm
>+++ b/dom/apps/src/AppsUtils.jsm
>@@ -12,34 +12,35 @@ const Cr = Components.results;
> Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> Cu.import("resource://gre/modules/Services.jsm");
> 
> // Shared code for AppsServiceChild.jsm and Webapps.jsm
> 
> let EXPORTED_SYMBOLS = ["AppsUtils"];
> 
> function debug(s) {
>-  //dump("-*- AppsUtils.jsm: " + s + "\n");
>+  dump("-*- AppsUtils.jsm: " + s + "\n");

You should remove that.



>+
>+const Ci = Components.interfaces;
>+
>+var EXPORTED_SYMBOLS = ["PermissionsTable",
>+                        "UNKNOWN_ACTION",
>+                        "ALLOW_ACTION",
>+                        "DENY_ACTION",
>+                        "PROMPT_ACTION",
>+                        "AllPossiblePermissions",];

Do you need the extra ","?



>+          for (let idx in AllPossiblePermissions) {
>+            let index = newPerms.indexOf(AllPossiblePermissions[idx]);
>+            if (index == -1) {
>+              // See if the permission was installed previously
>+              let _perm = PermSettings.get(AllPossiblePermissions[idx],
>+                                           aData.app.manifestURL,
>+                                           aData.app.origin,
>+                                           false);
>+              if (_perm == "unknown" || _perm == "deny") {
>+                // All 'deny' permissions should be preserved
>+                continue;
>+              }
>+              // Remove the deprecated permission
>+              PermSettings.set(AllPossiblePermissions[idx],
>+                               aData.app.manifestURL,
>+                               aData.app.origin,
>+                               "unknown",
>+                               false);

Argument order is wrong. The value is the 2nd argument.





>+            }
>+          }
>+        }
>+      }
>+
>+      for (let permName in newManifest.permissions) {
>+        if (!PermissionsTable[permName]) {
>+          Cu.reportError("'" + permName + "'" +
>+                         " is not a valid Webapps permission type. Aborting Webapp installation");
>+          this.uninstall(aData);
>+          return;
>+        }
>+
>+        let perms = expandPermissions(permName,
>+                                      newManifest.permissions[permName].access);
>+        for (let idx in perms) {
>+          // Check to see if the 'webapp' is app/priv/certified
>+          let installType = getAppManifestStatus(newManifest);
>+          let installPermType;
>+          switch (installType) {
>+          case Ci.nsIPrincipal.APP_STATUS_CERTIFIED:
>+            installPermType = "certified";
>+            break;
>+          case Ci.nsIPrincipal.APP_STATUS_PRIVILEGED:
>+            installPermType = "privileged";
>+            break;
>+          case Ci.nsIPrincipal.APP_STATUS_INSTALLED:
>+            installPermType = "app";
>+            break;
>+          default:
>+            // Cannot determine app type, abort install
>+            this.uninstall(aData);
>+            Cu.reportError("Webapps.jsm: Cannot determine app type, install cancelled");
>+            return;
>+          }
>+          let perm = PermissionsTable[permName][installPermType];
>+          let permStr = PERM_TO_STRING[perm];
>+          PermSettings.set(perms[idx],
>+                           aData.app.manifestURL,
>+                           aData.app.origin,
>+                           permStr,
>+                           false);


same here.



>diff --git a/dom/tests/browser/Makefile.in b/dom/tests/browser/Makefile.in
>--- a/dom/tests/browser/Makefile.in
>+++ b/dom/tests/browser/Makefile.in
>@@ -15,11 +15,17 @@ MOCHITEST_BROWSER_FILES := \
>   browser_focus_steal_from_chrome_during_mousedown.js \
>   browser_autofocus_background.js \
>   browser_ConsoleAPITests.js \
>   test-console-api.html \
>   browser_ConsoleStorageAPITests.js \
>   browser_ConsoleStoragePBTest.js \
>   browser_autofocus_preference.js \
>   browser_bug396843.js \
>+	browser_webapps_permissions.js \
>+  browser_webapps_perms_reinstall.js \
>+	test-webapp.webapp \
>+	test-webapp-reinstall.webapp \
>+  test-webapp-original.webapp \
>+	test-webapps-permissions.html \
>   $(NULL)

indentation.

Comment 113

[David Dahl :ddahl]

(In reply to Gregor Wagner [:gwagner] from comment #112)
> ># HG changeset patch
> ># Parent 80fdad48c4b5a7b72c2e38876557cb761654ee9b
> >
> >diff --git a/dom/apps/src/AppsUtils.jsm b/dom/apps/src/AppsUtils.jsm
> >--- a/dom/apps/src/AppsUtils.jsm
> >+++ b/dom/apps/src/AppsUtils.jsm
> >@@ -12,34 +12,35 @@ const Cr = Components.results;
> > Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > Cu.import("resource://gre/modules/Services.jsm");
> > 
> > // Shared code for AppsServiceChild.jsm and Webapps.jsm
> > 
> > let EXPORTED_SYMBOLS = ["AppsUtils"];
> > 
> > function debug(s) {
> >-  //dump("-*- AppsUtils.jsm: " + s + "\n");
> >+  dump("-*- AppsUtils.jsm: " + s + "\n");
> 
> You should remove that.

Whoops, thought I did do that.

> 
> 
> 
> >+
> >+const Ci = Components.interfaces;
> >+
> >+var EXPORTED_SYMBOLS = ["PermissionsTable",
> >+                        "UNKNOWN_ACTION",
> >+                        "ALLOW_ACTION",
> >+                        "DENY_ACTION",
> >+                        "PROMPT_ACTION",
> >+                        "AllPossiblePermissions",];
> 
> Do you need the extra ","?

That is considered good form on the Firefox team

> 
> 
> 
> >+          for (let idx in AllPossiblePermissions) {
> >+            let index = newPerms.indexOf(AllPossiblePermissions[idx]);
> ....
> Argument order is wrong. The value is the 2nd argument.
> 
Ok, got the correct patch from you. can you upload it to the bug?
> >+ PermSettings.set(perms[idx],
> >+                           aData.app.manifestURL,
> >+                           aData.app.origin,
> >+                           permStr,
> >+                           false);
> 
> 
> same here.
> 
fixed it

> 
> 
> >diff --git a/dom/tests/browser/Makefile.in b/dom/tests/browser/Makefile.in
> >--- a/dom/tests/browser/Makefile.in
> >+++ b/dom/tests/browser/Makefile.in
> >@@ -15,11 +15,17 @@ MOCHITEST_BROWSER_FILES := \
> >   browser_focus_steal_from_chrome_during_mousedown.js \
> >   browser_autofocus_background.js \
> >   browser_ConsoleAPITests.js \
> >   test-console-api.html \
> >   browser_ConsoleStorageAPITests.js \
> >   browser_ConsoleStoragePBTest.js \
> >   browser_autofocus_preference.js \
> >   browser_bug396843.js \
> >+	browser_webapps_permissions.js \
> >+  browser_webapps_perms_reinstall.js \
> >+	test-webapp.webapp \
> >+	test-webapp-reinstall.webapp \
> >+  test-webapp-original.webapp \
> >+	test-webapps-permissions.html \
> >   $(NULL)
> 
> indentation.

Are these 2 spaces? Emacs is deceiving me

Comment 114

[David Dahl :ddahl]

Attachment: Patch 20

Ok, everything passes with the latest inbound patch for bug 770731

Comment 115

[Myk Melez [:myk] [@mykmelez]]

(In reply to David Dahl :ddahl from comment #113)
> (In reply to Gregor Wagner [:gwagner] from comment #112)
> > >+var EXPORTED_SYMBOLS = ["PermissionsTable",
> > >+                        "UNKNOWN_ACTION",
> > >+                        "ALLOW_ACTION",
> > >+                        "DENY_ACTION",
> > >+                        "PROMPT_ACTION",
> > >+                        "AllPossiblePermissions",];
> > 
> > Do you need the extra ","?
> 
> That is considered good form on the Firefox team

The reason why a trailing comma is useful is that it enables you to add an item to the list later without modifying the line containing the last item.  But that's only possible if you put the closing bracket on the next line, i.e.:

var EXPORTED_SYMBOLS = [
  "PermissionsTable",
  "UNKNOWN_ACTION",
  "ALLOW_ACTION",
  "DENY_ACTION",
  "PROMPT_ACTION",
  "AllPossiblePermissions",
];

See the definition of initTable in Services.jsm for an example of this formatting: <http://mxr.mozilla.org/mozilla-central/source/toolkit/content/Services.jsm#32>.

Comment 116

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

Comment on attachment 663165 [details] [diff] [review]
Patch 20

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

I'd really like to get Fabrice to look over this since it's so important that we get this right and he knows this code the best. I'd also like another look over since there were a few review comments.

::: dom/apps/src/AppsUtils.jsm
@@ +16,5 @@
>  
>  let EXPORTED_SYMBOLS = ["AppsUtils"];
>  
>  function debug(s) {
> +  // dump("-*- AppsUtils.jsm: " + s + "\n");

Remove this change.

@@ +34,5 @@
>        removable: aApp.removable,
>        localId: aApp.localId,
>        progress: aApp.progress || 0.0,
> +      status: aApp.status || "installed",
> +      permissions: aApp.permissions || {},

You need to do a "clone" of this object, rather than just create another reference to the same object. See how that's done above for the 'receipts' property.

Actually, why do you add a .permissions property on app objects at all? That doesn't seem needed and in fact seems very risky since people should use the information stored in the permission manager, not information from the app object.

::: dom/apps/src/PermissionsTable.jsm
@@ +15,5 @@
> +
> +const UNKNOWN_ACTION = Ci.nsIPermissionManager.UNKNOWN_ACTION;
> +const ALLOW_ACTION = Ci.nsIPermissionManager.ALLOW_ACTION;
> +const DENY_ACTION = Ci.nsIPermissionManager.DENY_ACTION;
> +const PROMPT_ACTION = Ci.nsIPermissionManager.PROMPT_ACTION;

I still think you should use the string values consistently throughout this code. But I guess I can live with this.

::: dom/apps/src/Webapps.jsm
@@ +40,5 @@
> +
> +const PERM_TO_STRING = {
> +  0: "unknown", // Ci.nsIPermissionManager.UNKNOWN_ACTION, etc...
> +  1: "allow",
> +  2: "deny",

If you really want to keep this, at least use Ci.nsIPermissionManager.UNKNOWN_ACTION etc here. I.e:

const PERM_TO_STRING = {
  Ci.nsIPermissionManager.UNKNOWN_ACTION: "unknown",
  Ci.nsIPermissionManager.ALLWO_ACTION: "allow",
  ...
};

@@ +102,5 @@
> +    return Ci.nsIPrincipal.APP_STATUS_PRIVILEGED;
> +  case "certified":
> +    return Ci.nsIPrincipal.APP_STATUS_CERTIFIED;
> +  default:
> +    // XXXddahl: default is Ci.nsIPrincipal.APP_STATUS_INSTALLED or throw?

You should throw here IMO.

@@ +115,5 @@
> + * @returns Array
> + **/
> +function mapSuffixes(aPermName, aSuffixes)
> +{
> +  return aSuffixes.map(function(suf) { return aPermName + "-" + suf; });

Can you export this function from PermissionsTable.jsm instead of duplicating it?

@@ +134,5 @@
> +    throw new Error("Webapps.jsm: App install failed, Unknown Permission: " + aPermName);
> +  }
> +  if (!aAccess && PermissionsTable[aPermName].access) {
> +    Cu.reportError("Webapps.jsm: installPermissions: Invalid Manifest");
> +    throw new Error("Webapps.jsm: App install failed, Invalid Manifest");

You should also fail if aAccess is set and PermissionsTable[aPerfName].access is not set.

You could do this by changing the above test to 

if (!aAccess != !PermissionsTable[aPermName].access) { ... }

but it might be clearer to simply do

if (!aAccess && PermissionsTable[aPermName].access ||
    aAccess && !PermissionsTable[aPermName].access) { ... }

@@ +163,5 @@
> +
> +  let expandedPerms = [];
> +  for (let idx in permArr) {
> +    let perms = mapSuffixes(aPermName, PermissionsTable[aPermName].access);
> +    let index = perms.indexOf(permArr[idx]);

These two lines are somewhat inefficient. Just change it to

let index = PermissionsTable[aPermName].access.indexOf(requestedSuffixes[idx])

@@ +168,5 @@
> +    if (index == -1) {
> +      Cu.reportError("Requested permission access does not exist. " +
> +                   aPermName + ": " + permArr[idx]);
> +      throw new Error("Requested permission access does not exist. " +
> +                      aPermName + ": " + permArr[idx]);

This still seems like it'll make it impossible to get read-write access to settings. permArr will contain "settings-create", but perms won't, which will cause an exception to be thrown.

Simply change this to

if (index != -1) {
  expandedPerms.push(permArr[idx]);
}

Actually, once you've done this and the above comment, you can probably get rid of the 'index' temporary variable. Up to you.

@@ +656,5 @@
>      }
>      aData.mm.sendAsyncMessage("Webapps:Install:Return:KO", aData);
>    },
>  
>    confirmInstall: function(aData, aFromSync, aProfileDir, aOfflineCacheObserver) {

Someone else needs to review the changes to this function. I don't know this code well enough.

@@ +794,5 @@
> +                // All 'deny' permissions should be preserved
> +                continue;
> +              }
> +              // Remove the deprecated permission
> +              PermSettings.set(AllPossiblePermissions[idx],

Please file a followup to turn this into a PermSettings.remove call.

@@ +808,5 @@
> +      for (let permName in newManifest.permissions) {
> +        if (!PermissionsTable[permName]) {
> +          Cu.reportError("'" + permName + "'" +
> +                         " is not a valid Webapps permission type. Aborting Webapp installation");
> +          this.uninstall(aData);

You can just throw here, right?

@@ +818,5 @@
> +        for (let idx in perms) {
> +          // Check to see if the 'webapp' is app/priv/certified
> +          let installType = getAppManifestStatus(newManifest);
> +          let installPermType;
> +          switch (installType) {

Move this to outside of both for-loops.

@@ +831,5 @@
> +            break;
> +          default:
> +            // Cannot determine app type, abort install
> +            this.uninstall(aData);
> +            Cu.reportError("Webapps.jsm: Cannot determine app type, install cancelled");

You can just throw here, right?

@@ +835,5 @@
> +            Cu.reportError("Webapps.jsm: Cannot determine app type, install cancelled");
> +            return;
> +          }
> +          let perm = PermissionsTable[permName][installPermType];
> +          let permStr = PERM_TO_STRING[perm];

Move this outside of the inner-most loop. And I'd call it something like "permValue" or "accessLevel". "perm" sounds like it's the name of a permission, especially when you have a "perms" array which contains names of permissions.

@@ +841,5 @@
> +                           permStr,
> +                           aData.app.manifestURL,
> +                           aData.app.origin,
> +                           false);
> +          aAppObject.permissions[permName] = newManifest.permissions[permName];

I don't think this should be needed. Hopefully we don't need the appObject.permissions property at all.

@@ +1081,5 @@
>        }).bind(this));
>      }
>  
>      if (!found) {
> +      aData.mm.sendAsyncMessage("Webapps:Uninstall:Return:KO", aData);

Someone else has to review this change.

Comment 117

[David Dahl :ddahl]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #115)
> (In reply to David Dahl :ddahl from comment #113)

> > That is considered good form on the Firefox team
> 
> The reason why a trailing comma is useful is that it enables you to add an
> item to the list later without modifying the line containing the last item. 
> But that's only possible if you put the closing bracket on the next line,

Gah! thanks!

Comment 118

[David Dahl :ddahl]

(In reply to Jonas Sicking (:sicking) from comment #116)
> Comment on attachment 663165 [details] [diff] [review]
> Patch 20
> 
> Review of attachment 663165 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd really like to get Fabrice to look over this since it's so important
> that we get this right and he knows this code the best. I'd also like
> another look over since there were a few review comments.
Ok

> 
> ::: dom/apps/src/AppsUtils.jsm
> @@ +16,5 @@
> >  
> >  let EXPORTED_SYMBOLS = ["AppsUtils"];
> >  
> >  function debug(s) {
> > +  // dump("-*- AppsUtils.jsm: " + s + "\n");
> 
> Remove this change.
Done
> 
> @@ +34,5 @@
> >        removable: aApp.removable,
> >        localId: aApp.localId,
> >        progress: aApp.progress || 0.0,
> > +      status: aApp.status || "installed",
> > +      permissions: aApp.permissions || {},
> 
> You need to do a "clone" of this object, rather than just create another
> reference to the same object. See how that's done above for the 'receipts'
> property.
> 
> Actually, why do you add a .permissions property on app objects at all? That
> doesn't seem needed and in fact seems very risky since people should use the
> information stored in the permission manager, not information from the app
> object.
> 
I removed all of the references to permissions in the app object, I don't actually need it anymore, it slipped by

> ::: dom/apps/src/PermissionsTable.jsm
> @@ +15,5 @@
> > +
> > +const UNKNOWN_ACTION = Ci.nsIPermissionManager.UNKNOWN_ACTION;
> > +const ALLOW_ACTION = Ci.nsIPermissionManager.ALLOW_ACTION;
> > +const DENY_ACTION = Ci.nsIPermissionManager.DENY_ACTION;
> > +const PROMPT_ACTION = Ci.nsIPermissionManager.PROMPT_ACTION;
> 
> I still think you should use the string values consistently throughout this
> code. But I guess I can live with this.
> 
> ::: dom/apps/src/Webapps.jsm
> @@ +40,5 @@
> > +
> > +const PERM_TO_STRING = {
> > +  0: "unknown", // Ci.nsIPermissionManager.UNKNOWN_ACTION, etc...
> > +  1: "allow",
> > +  2: "deny",
> 
> If you really want to keep this, at least use
> Ci.nsIPermissionManager.UNKNOWN_ACTION etc here. I.e:
> 
> const PERM_TO_STRING = {
>   Ci.nsIPermissionManager.UNKNOWN_ACTION: "unknown",
>   Ci.nsIPermissionManager.ALLWO_ACTION: "allow",
>   ...
> };

Being an object literal, using a property of another object as a property name in a new object does not work.

> 
> @@ +102,5 @@
> > +    return Ci.nsIPrincipal.APP_STATUS_PRIVILEGED;
> > +  case "certified":
> > +    return Ci.nsIPrincipal.APP_STATUS_CERTIFIED;
> > +  default:
> > +    // XXXddahl: default is Ci.nsIPrincipal.APP_STATUS_INSTALLED or throw?
> 
> You should throw here IMO.
Done

> 
> @@ +115,5 @@
> > + * @returns Array
> > + **/
> > +function mapSuffixes(aPermName, aSuffixes)
> > +{
> > +  return aSuffixes.map(function(suf) { return aPermName + "-" + suf; });
> 
> Can you export this function from PermissionsTable.jsm instead of
> duplicating it?
> 
Good catch

> @@ +134,5 @@
> > +    throw new Error("Webapps.jsm: App install failed, Unknown Permission: " + aPermName);
> > +  }
> > +  if (!aAccess && PermissionsTable[aPermName].access) {
> > +    Cu.reportError("Webapps.jsm: installPermissions: Invalid Manifest");
> > +    throw new Error("Webapps.jsm: App install failed, Invalid Manifest");
> 
> You should also fail if aAccess is set and
> PermissionsTable[aPerfName].access is not set.
> 

> but it might be clearer to simply do
> 
> if (!aAccess && PermissionsTable[aPermName].access ||
>     aAccess && !PermissionsTable[aPermName].access) { ... }
> 
Done

> These two lines are somewhat inefficient. Just change it to
> 
> let index =
> PermissionsTable[aPermName].access.indexOf(requestedSuffixes[idx])
> 

Ok
> @@ +168,5 @@
> > +    if (index == -1) {
> > +      Cu.reportError("Requested permission access does not exist. " +
> > +                   aPermName + ": " + permArr[idx]);
> > +      throw new Error("Requested permission access does not exist. " +
> > +                      aPermName + ": " + permArr[idx]);
> 
> This still seems like it'll make it impossible to get read-write access to
> settings. permArr will contain "settings-create", but perms won't, which
> will cause an exception to be thrown.
> 
> Actually, once you've done this and the above comment, you can probably get
> rid of the 'index' temporary variable. Up to you.
> 
Indeed. Done.

> @@ +656,5 @@
> >      }
> >      aData.mm.sendAsyncMessage("Webapps:Install:Return:KO", aData);
> >    },
> >  
> >    confirmInstall: function(aData, aFromSync, aProfileDir, aOfflineCacheObserver) {
> 
> Someone else needs to review the changes to this function. I don't know this
> code well enough.
> 
I will ask fabrice

> @@ +794,5 @@
> > +                // All 'deny' permissions should be preserved
> > +                continue;
> > +              }
> > +              // Remove the deprecated permission
> > +              PermSettings.set(AllPossiblePermissions[idx],
> 
> Please file a followup to turn this into a PermSettings.remove call.
Done, bug 793204

> @@ +808,5 @@
> > +      for (let permName in newManifest.permissions) {
> > +        if (!PermissionsTable[permName]) {
> > +          Cu.reportError("'" + permName + "'" +
> > +                         " is not a valid Webapps permission type. Aborting Webapp installation");
> > +          this.uninstall(aData);
> 
> You can just throw here, right?
> 
Yep

> @@ +818,5 @@
> > +        for (let idx in perms) {
> > +          // Check to see if the 'webapp' is app/priv/certified
> > +          let installType = getAppManifestStatus(newManifest);
> > +          let installPermType;
> > +          switch (installType) {
> 
> Move this to outside of both for-loops.

ok, the installPermType let statement, right?

> @@ +831,5 @@
> > +            break;
> > +          default:
> > +            // Cannot determine app type, abort install
> > +            this.uninstall(aData);
> > +            Cu.reportError("Webapps.jsm: Cannot determine app type, install cancelled");
> 
> You can just throw here, right?

Yes
> 
> @@ +835,5 @@
> > +            Cu.reportError("Webapps.jsm: Cannot determine app type, install cancelled");
> > +            return;
> > +          }
> > +          let perm = PermissionsTable[permName][installPermType];
> > +          let permStr = PERM_TO_STRING[perm];
> 
> Move this outside of the inner-most loop. And I'd call it something like
> "permValue" or "accessLevel". "perm" sounds like it's the name of a
> permission, especially when you have a "perms" array which contains names of
> permissions.
> 
permStr and perm require installPermType being set directly above in the switch statement to work

> @@ +841,5 @@
> > +                           permStr,
> > +                           aData.app.manifestURL,
> > +                           aData.app.origin,
> > +                           false);
> > +          aAppObject.permissions[permName] = newManifest.permissions[permName];
> 
> I don't think this should be needed. Hopefully we don't need the
> appObject.permissions property at all.
>
OK, Removed.
 
> @@ +1081,5 @@
> >        }).bind(this));
> >      }
> >  
> >      if (!found) {
> > +      aData.mm.sendAsyncMessage("Webapps:Uninstall:Return:KO", aData);
> 
> Someone else has to review this change.

Ok, will ask fabrice for review next

Comment 119

[David Dahl :ddahl]

Attachment: Patch 21

Jonas: is it OK to pass the review over to fabrice now? Otherwise, I can send it your way.

Comment 120

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

> > @@ +835,5 @@
> > > +            Cu.reportError("Webapps.jsm: Cannot determine app type, install cancelled");
> > > +            return;
> > > +          }
> > > +          let perm = PermissionsTable[permName][installPermType];
> > > +          let permStr = PERM_TO_STRING[perm];
> > 
> > Move this outside of the inner-most loop. And I'd call it something like
> > "permValue" or "accessLevel". "perm" sounds like it's the name of a
> > permission, especially when you have a "perms" array which contains names of
> > permissions.
> > 
> permStr and perm require installPermType being set directly above in the
> switch statement to work

No, i mean move the whole switch statement out to where you now have declared installPermType. Nothing in that switch statement changes between the runs.

And then move the two lines out to outside of the inner loop.

Comment 121

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

And get rid of the "installType" temporary.

Comment 122

[David Dahl :ddahl]

(In reply to Jonas Sicking (:sicking) from comment #120)
> No, i mean move the whole switch statement out to where you now have
> declared installPermType. Nothing in that switch statement changes between
> the runs.
> 
> And then move the two lines out to outside of the inner loop.
OK, got it.

> And get rid of the "installType" temporary.
Ok.

Thanks for the feedback/review. Passing over to fabrice now.

Comment 123

[[:fabrice] Fabrice Desré]

Comment on attachment 663442 [details] [diff] [review]
Patch 21

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

Overall that looks good, but I'd love to see if we can move more code out of Webapps.jsm and into PermissionsXXX.jsm
Also, we don't do anything when uninstalling an app here. That should not matter much since we're not reusing appIds, but we should at least file a follow-up to do that.

::: dom/apps/src/AppsUtils.jsm
@@ -79,5 @@
>        if (aApps[id].manifestURL == aManifestURL) {
>          return aApps[id].localId;
>        }
>      }
> -

Nit: remove this change

::: dom/apps/src/PermissionsTable.jsm
@@ +70,5 @@
> +                                      "write",
> +                                      "create"
> +                                     ]
> +                           },
> +                           "device-storage": {

The device-storage permission has been split up in different permissions:
device-storage:apps
device-storage:pictures
device-storage:videos
device-storage:music

@@ +77,5 @@
> +                             certified: ALLOW_ACTION,
> +                             access: ["read",
> +                                      "write",
> +                                      "create"
> +                                     ]

dougt claims that these are not used in device storage

::: dom/apps/src/Webapps.js
@@ +21,5 @@
> +{
> +  if (DEBUG) {
> +    dump("-*- Webapps.js: " + aMsg + "\n");
> +  }
> +}

That looks unused. If you want to keep it, please rewrite as

function debug(aMsg) {
  // dump("-*- Webapps.js: " + aMsg + "\n");
}

::: dom/apps/src/Webapps.jsm
@@ +14,5 @@
> +    }
> +    dump("-*- webapps.jsm: " + output.join(" ") + "\n");
> +  }
> +}
> +

here also, use a simpler debug() method.

@@ +42,5 @@
> +  0: "unknown", // Ci.nsIPermissionManager.UNKNOWN_ACTION, etc...
> +  1: "allow",
> +  2: "deny",
> +  3: "prompt",
> +};

why isn't this an array?
Can you also move it into the only function that uses it?

@@ +104,5 @@
> +    return Ci.nsIPrincipal.APP_STATUS_CERTIFIED;
> +  default:
> +    throw new Error("Webapps.jsm: Undetermined app manifest type");
> +  }
> +}

Actually I would not object to see this being moved out of Webapps.jsm and into AppUtils.jsm

@@ +157,5 @@
> +    }
> +  }
> +  return expandedPerms;
> +}
> +

Could we move this function into PermissionTable.jsm (and probably rename PermissionTable.jsm if we do)? I'm just trying to keep Webapps.jsm size manageable.

@@ +824,5 @@
> +      Cu.reportError(ex);
> +      this.uninstall(aData);
> +    }
> +   },
> +

And this function could also move into the Permissions jsm.

@@ +1413,5 @@
>      return this._localeProp("orientation");
>    },
>  
> +  get permissions() {
> +    return this._localeProp("permissions");

We don't want to localize permissions (the description strings can be l10n, but we're not using them).

::: dom/permission/PermissionSettings.js
@@ +13,5 @@
> +      output.push(arguments[prop]);
> +    }
> +    dump("-*- PermissionSettings: " + output.join(" ") + "\n");
> +  }
> +}

rewrite as:

function debug(aMsg) {
// dump("-*- PermissionSettings.js: " + aMsg + "\n");
}

::: dom/tests/browser/browser_webapps_permissions.js
@@ +23,5 @@
> +
> +const installedPermsToTest = {
> +  "geolocation": "prompt",
> +  "alarm": "allow",
> +  "device-storage-read": "deny",

Apply the device-storage permission change here also.

@@ +29,5 @@
> +
> +const uninstalledPermsToTest = {
> +  "geolocation": "unknown",
> +  "alarm": "unknown",
> +  "device-storage-read": "unknown",

Ditto

::: dom/tests/browser/test-webapp-original.webapp
@@ +6,5 @@
> +    "geolocation": {
> +      "description": "geolocate"
> +    },
> +    "alarm": {
> +      "description": "alarminging"

alarming ?

Comment 124

[David Dahl :ddahl]

(In reply to Fabrice Desré [:fabrice] from comment #123)
> Comment on attachment 663442 [details] [diff] [review]
> Patch 21
> 
> Review of attachment 663442 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall that looks good, but I'd love to see if we can move more code out of
> Webapps.jsm and into PermissionsXXX.jsm
> Also, we don't do anything when uninstalling an app here. That should not
> matter much since we're not reusing appIds, but we should at least file a
> follow-up to do that.

There is native code that removes all permissions for an app on uninstall see above comments

> Could we move this function into PermissionTable.jsm (and probably rename
> PermissionTable.jsm if we do)? I'm just trying to keep Webapps.jsm size
> manageable.

Ok, how about PermissionInstaller.jsm?

Comment 125

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

Regarding the device storage stuff. I believe the plan is to move device storage to support read/write/create as separate permissions. I'm also not entirely sure how to handle the "music" vs. "pictures" vs. "video", so I think we should keep the patch as-is for now and adjust things as needed in followups.

Does this sound ok?

Comment 126

[[:fabrice] Fabrice Desré]

(In reply to David Dahl :ddahl from comment #124)

> There is native code that removes all permissions for an app on uninstall
> see above comments

Great!

> > Could we move this function into PermissionTable.jsm (and probably rename
> > PermissionTable.jsm if we do)? I'm just trying to keep Webapps.jsm size
> > manageable.
> 
> Ok, how about PermissionInstaller.jsm?

That works for me!

Comment 127

[[:fabrice] Fabrice Desré]

(In reply to Jonas Sicking (:sicking) from comment #125)
> Regarding the device storage stuff. I believe the plan is to move device
> storage to support read/write/create as separate permissions. I'm also not
> entirely sure how to handle the "music" vs. "pictures" vs. "video", so I
> think we should keep the patch as-is for now and adjust things as needed in
> followups.
> 
> Does this sound ok?

I trust you on the access stuff, but the media apps already use the music/pictures/video permissions. Eg. the gallery app asks for:

"permissions": [
    "device-storage:pictures",
    "settings"
  ],

Comment 128

[David Dahl :ddahl]

(In reply to Fabrice Desré [:fabrice] from comment #126)
> > > Could we move this function into PermissionTable.jsm (and probably rename
> > > PermissionTable.jsm if we do)? I'm just trying to keep Webapps.jsm size
> > > manageable.
> > 
> > Ok, how about PermissionInstaller.jsm?
> 

Ok, just wasted a bunch of time moving code around, the DOMApplicationManifest object is not recognized as a constructor inside the PermissionsInstall.jsm. Also, getAppManifestStatus would have to be exported as well, which just makes this less elegant and understandable.

In general jsms that import each other seem to do weird things. I think we should file a followup to separate the code. It works now, right?

Comment 129

[[:fabrice] Fabrice Desré]

(In reply to David Dahl :ddahl from comment #128)
> (In reply to Fabrice Desré [:fabrice] from comment #126)
> > > > Could we move this function into PermissionTable.jsm (and probably rename
> > > > PermissionTable.jsm if we do)? I'm just trying to keep Webapps.jsm size
> > > > manageable.
> > > 
> > > Ok, how about PermissionInstaller.jsm?
> > 
> 
> Ok, just wasted a bunch of time moving code around, the
> DOMApplicationManifest object is not recognized as a constructor inside the
> PermissionsInstall.jsm. Also, getAppManifestStatus would have to be exported
> as well, which just makes this less elegant and understandable.

I commented in my review that getAppManifestStatus should be moved into AppsUtils.jsm.
We should also move DOMApplicationManifest there (and rename it) but let's do that in a follow up.

Comment 130

[David Dahl :ddahl]

(In reply to Fabrice Desré [:fabrice] from comment #129)

> I commented in my review that getAppManifestStatus should be moved into
> AppsUtils.jsm.
> We should also move DOMApplicationManifest there (and rename it) but let's
> do that in a follow up.

That's actually the main problem, the jsm <-> jsm dependency makes that object not appear as a constructor, hence: TEST-INFO | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_permissions.js | Console message: [JavaScript Error: "TypeError: DOMApplicationManifest is not a constructor" {file: "resource://gre/modules/Webapps.jsm" line: 436}]

The only way to get past it was to pass in the constructor from where I call installPermissions

Comment 131

[David Dahl :ddahl]

Attachment: non-working Patch 22 - moving things around

Things are regressing here. Ever since I started moving things around - or - since my last hg pull --update PermSettings.set does not work at all, no exceptions no assertions, silent failure. 

In going back to the previous patch, and making any non-invasive changes, the same problem, silent failure. gdb pausing the execution shows no permissions are added, again no exceptions, no assertions. Trying to hg pull --update once more.

Comment 132

[David Dahl :ddahl]

Setting a breakpoint in permissionManager.addFromPrincipal shows that it is never called for anything I send through PermissionSettings.set - the message manager never gets the message? What changed lately in that code?

Comment 133

[Gregor Wagner [:gwagner]]

(In reply to David Dahl :ddahl from comment #132)
> Setting a breakpoint in permissionManager.addFromPrincipal shows that it is
> never called for anything I send through PermissionSettings.set - the
> message manager never gets the message? What changed lately in that code?

PermissionSettings.jsm has to be loaded manually in the test on desktop because of bug 793198.

Comment 134

[David Dahl :ddahl]

Gregor:

Now the tests are leaking the PermissionSettingsModule - this never happened before - did we not have an observer in PermissionSettings that watched for [inner]outer-window-destroyed to remove the permissionssetting property?

error:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_perms_reinstall.js | leaked window property: PermissionSettingsModule

Comment 135

[Gregor Wagner [:gwagner]]

(In reply to David Dahl :ddahl from comment #134)
> Gregor:
> 
> Now the tests are leaking the PermissionSettingsModule - this never happened
> before - did we not have an observer in PermissionSettings that watched for
> [inner]outer-window-destroyed to remove the permissionssetting property?
> 
> error:
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/dom/tests/browser/
> browser_webapps_perms_reinstall.js | leaked window property:
> PermissionSettingsModule

Hm we have a "profile-before-change" observer and remove all registered messages there. What do you mean with removing the permissionssetting property?

Comment 136

[Gregor Wagner [:gwagner]]

Do you have an updated patch? The current one doesn't seem to work.

Comment 137

[David Dahl :ddahl]

Attachment: Patch 23

This is the  latest patch using the original changes to Webapps.jsm. I tried to break things out into a PermissionsInstaller.jsm, but came accross too many problems, one of which is the re-emergence of:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_permissions.js | an unexpected uncaught JS exception reported through window.onerror - Error: Permission denied to access object at :0

No idea what is happening, but things just regressed too much. Agian, the only thing in the way of a working patch is the apparent leak.

Comment 138

[Gregor Wagner [:gwagner]]

I don't see any more leaks if I import it in the test function like:

  var PS = {};
  Components.utils.import("resource://gre/modules/PermissionSettings.jsm", PS);

Comment 139

[David Dahl :ddahl]

(In reply to Gregor Wagner [:gwagner] from comment #138)
> I don't see any more leaks if I import it in the test function like:
> 
>   var PS = {};
>   Components.utils.import("resource://gre/modules/PermissionSettings.jsm",
> PS);

Thanks. Do not understand why the leak goes away, but, whatever.

Comment 140

[David Dahl :ddahl]

Attachment: Patch for review

This patch works, all tests pass, review comments addressed except for code refactor/moves

Comment 141

[[:fabrice] Fabrice Desré]

Comment on attachment 664799 [details] [diff] [review]
Patch for review

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

r=me but I really don't understand why we can't move getAppManifestStatus() to AppsUtils and expandPermissions() to PermissionsTable.jsm

Comment 142

[David Dahl :ddahl]

(In reply to Fabrice Desré [:fabrice] from comment #141)
> Comment on attachment 664799 [details] [diff] [review]
> Patch for review
> 
> Review of attachment 664799 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me but I really don't understand why we can't move getAppManifestStatus()
> to AppsUtils and expandPermissions() to PermissionsTable.jsm

I kept seeing: TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_permissions.js | an unexpected uncaught JS exception reported through window.onerror - Error: Permission denied to access object at :0

Which seems like another black hole of debugging waiting to happen related to the problem in comment 107, I filed bug 794345 to refactor this.

I will want to rebase this patch once the patch in bug 792882 lands on m-c

Comment 143

[David Dahl :ddahl]

oh, another issue for not moving code around now are that 4 or 5 other modules will need to import AppsUtils.jsm

Comment 144

[Gregor Wagner [:gwagner]]

Landing time \o/

Comment 145

[David Dahl :ddahl]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c0e68dceca8

Comment 146

[Ryan VanderMeulen [:RyanVM]]

Sorry, this had to be backed out for test failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2afb3c28c6e

https://tbpl.mozilla.org/php/getParsedLog.php?id=15570668&tree=Mozilla-Inbound

TEST-START | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_perms_reinstall.js
TEST-INFO | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_perms_reinstall.js | Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "http://mochi.test:8888/browser/dom/tests/browser/test-webapps-permissions.html" line: 0}]
TEST-PASS | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_perms_reinstall.js | we have a mozApps property
TEST-PASS | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_perms_reinstall.js | mozPermissions is available
TEST-INFO | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_perms_reinstall.js | Console message: [JavaScript Warning: "ReferenceError: reference to undefined property SpecialPowers.wrap(...).childNodes" {file: "chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_perms_reinstall.js" line: 152}]
TEST-PASS | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_perms_reinstall.js | we have a result: [xpconnect wrapped mozIDOMApplication]
TEST-PASS | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_perms_reinstall.js | install: geolocation is prompt
TEST-PASS | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_perms_reinstall.js | install: alarm is allow
TEST-PASS | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_perms_reinstall.js | install: contacts-read is deny
TEST-PASS | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_perms_reinstall.js | install: contacts-create is deny
TEST-PASS | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_perms_reinstall.js | install: contacts-write is deny
TEST-PASS | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_perms_reinstall.js | install: device-storage:apps is deny
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_perms_reinstall.js | an unexpected uncaught JS exception reported through window.onerror - NS_ERROR_FILE_TARGET_DOES_NOT_EXIST: Component returned failure code: 0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) [nsIFile.copyTo] at chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_perms_reinstall.js:178
Stack trace:
    JS frame :: chrome://mochikit/content/tests/SimpleTest/SimpleTest.js :: simpletestOnerror :: line 980
    native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

TEST-INFO | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_perms_reinstall.js | Console message: [JavaScript Error: "NS_ERROR_FILE_TARGET_DOES_NOT_EXIST: Component returned failure code: 0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) [nsIFile.copyTo]" {file: "chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_perms_reinstall.js" line: 178}]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_perms_reinstall.js | Test timed out
args: ['/home/cltbld/talos-slave/test/build/bin/screentopng']
SCREENSHOT: <see log>
INFO TEST-END | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_perms_reinstall.js | finished in 30080ms

Comment 147

[David Dahl :ddahl]

The test assumes the directory structure is like:   

let parents = ["_tests", "testing", "mochitest", "browser", "dom" , "tests", "browser"];
  newfile = newfile.parent; // up to dist/
  newfile = newfile.parent;// up to obj-dir/

  for (let idx in parents) {
    newfile.append(parents[idx]);
  }

Before over-writing a new webapp manifest - is this not the case on our test/build slaves?

Comment 148

[David Dahl :ddahl]

Ok, fixed the test, the build slaves run them from another path, pushing to try...

Comment 149

[Ryan VanderMeulen [:RyanVM]]

Hopefully that doesn't mean it'll fail if someone attempts to run tests locally.

Comment 150

[David Dahl :ddahl]

(In reply to Ryan VanderMeulen from comment #149)
> Hopefully that doesn't mean it'll fail if someone attempts to run tests
> locally.

I have paths for both. However, it still does not work on try server. 

Here is the latest path code:

http://pastebin.mozilla.org/1845794

Anyone from release engineering know what is wrong here?

Comment 151

[David Dahl :ddahl]

(In reply to David Dahl :ddahl from comment #150)
> (In reply to Ryan VanderMeulen from comment #149)
> > Hopefully that doesn't mean it'll fail if someone attempts to run tests
> > locally.
> 
> I have paths for both. However, it still does not work on try server. 
> 
> Here is the latest path code:
> 
> http://pastebin.mozilla.org/1845794
> 
> Anyone from release engineering know what is wrong here?

jmaher:

ted sez you might be the best authority on how to make this test work. A webapp manifest will change requesting different permissions. The app manifest needs to be overwritten and be served from the same url for this test to work corectly. the above pastebin is how I get this to work locally - and am trying to get the paths correct for the build machines.

Comment 152

[David Dahl :ddahl]

Fabrice: As much as I would hate to do this, I am thinking about disabling this test as it is non-trivial to run on build slaves. How about if I disable it and file a followup bug?

Comment 153

[[:fabrice] Fabrice Desré]

(In reply to David Dahl :ddahl from comment #152)
> Fabrice: As much as I would hate to do this, I am thinking about disabling
> this test as it is non-trivial to run on build slaves. How about if I
> disable it and file a followup bug?

That works for me.

Comment 154

[David Dahl :ddahl]

(In reply to Fabrice Desré [:fabrice] from comment #153)
> > disable it and file a followup bug?
> 
> That works for me.
Ok, running through try one more time, then I will push to inbound

Comment 155

[Mozilla RelEng Bot]

Try run for c087715361d1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c087715361d1
Results (out of 94 total builds):
    exception: 3
    success: 62
    warnings: 17
    failure: 12
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c087715361d1

Comment 156

[David Dahl :ddahl]

Attachment: Unbitrot Fixing Tests Patch r+

Seems to fail on build machines on Mac (maybe more)

Comment 157

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

FWIW, a simpler way to test updates is probably to install the app through a http:// URL pointing to an sjs file. You can then make the sjs file return different files based on state stored in the server. You can use getState/setState in the sjs file to read and write that state. See here for example:

http://mxr.mozilla.org/mozilla-central/source/content/base/test/file_CrossSiteXHR_cache_server.sjs

But let's get this landed first and worry about that after.

Comment 158

[David Dahl :ddahl]

(In reply to Jonas Sicking (:sicking) from comment #157)
> FWIW, a simpler way to test updates is probably to install the app through a
> http:// URL pointing to an sjs file. 

Sounds good. The latest issue may be mac-specific: https://tbpl.mozilla.org/?tree=Try&rev=b31269f27900

###!!! ASSERTION: OBSERVER_KEY not found!: '[userInfo valueForKey:OBSERVER_KEY] != nil', file ../../../../../toolkit/components/alerts/mac/mozNotificationCenterDelegate.mm, line 149

Still waiting on linux & windows

Comment 159

[David Dahl :ddahl]

On windows, all of the checks actually passed and I am seeing the following error:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/dom/tests/browser/browser_webapps_permissions.js | Found an unexpected alert:alert at the end of test run

No idea what that is all about.

Comment 160

[David Dahl :ddahl]

I think the best thing to do to get this landed today is to #ifdef out the tests on mac and win and file a followup to fix both platforms and a bug to re-write these tests using marionette - if possible.

Comment 161

[Mounir Lamouri (:mounir)]

(In reply to David Dahl :ddahl from comment #160)
> I think the best thing to do to get this landed today is to #ifdef out the
> tests on mac and win and file a followup to fix both platforms and a bug to
> re-write these tests using marionette - if possible.

What would be better in Marionette here?

Comment 162

[David Dahl :ddahl]

(In reply to Mounir Lamouri (:mounir) from comment #161)
> (In reply to David Dahl :ddahl from comment #160)
> > I think the best thing to do to get this landed today is to #ifdef out the
> > tests on mac and win and file a followup to fix both platforms and a bug to
> > re-write these tests using marionette - if possible.
> 
> What would be better in Marionette here?

I thought we were not supporting Mac and Win until later anyway.

Comment 163

[David Dahl :ddahl]

Attachment: Patch for inbound Disabled tests on Mac/Win

for gwagner to test on mac

Comment 164

[David Dahl :ddahl]

Attachment: Patch Latest - updated with inbound

Comment 165

[David Dahl :ddahl]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0153fec641e9

Comment 166

[David Dahl :ddahl]

Attachment: Patch landed on inbound

Comment 167

[Mozilla RelEng Bot]

Try run for 33a51487efb3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=33a51487efb3
Results (out of 22 total builds):
    success: 11
    warnings: 11
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-33a51487efb3

Comment 168

[Mozilla RelEng Bot]

Try run for 33a51487efb3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=33a51487efb3
Results (out of 22 total builds):
    success: 11
    warnings: 11
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-33a51487efb3

Comment 169

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0153fec641e9

Comment 170

[Jason Smith [:jsmith]]

Unassigning myself - someone else is being ramped up to do permissions testing for apps now.

Comment 171

[Mark Giffin [:markg]]

Added documentation for permissions field to this MDN page:

https://developer.mozilla.org/en-US/docs/Apps/Manifest#Fields