770731 - Expose JS API for modifying app permissions

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gregor Wagner [:gwagner]]

We want to create an API that looks like
DOMString ["allow", "prompt", "deny", "prompt-remember"] get (DOMString permission, DOMString manifestURI, DOMString origin)

void set (DOMString permission, DOMString manifestURI, DOMString origin, DOMString value)

Comment 1

[Mounir Lamouri (:mounir)]

Why do we need origin? The app manifest already contains it.

Also, I'm not sure what "prompt-remember" means.

Comment 2

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

Permissions are per origin+app, no? I.e. "google.com within the facebook app has access to geolocation and camera". Remember that this is a low-level API which we can build various UIs on top of.

"prompt-remember" means the same thing as "prompt", except when we display the prompt dialog, the "remember this decision" checkbox is checked by default.

Comment 3

[Mounir Lamouri (:mounir)]

(In reply to Jonas Sicking (:sicking) from comment #2)
> Permissions are per origin+app, no? I.e. "google.com within the facebook app
> has access to geolocation and camera". Remember that this is a low-level API
> which we can build various UIs on top of.

Indeed. We could allow not specifying the origin to give permissions to the app's origin though.

Comment 4

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

Given that we want to allow many types of UIs I don't think we should limit the API in that way. For example it's useful to be able to display in the UI that the user has given "google.com" permission to use geolocation inside of the facebook.com app.

Comment 5

[Mounir Lamouri (:mounir)]

(In reply to Mounir Lamouri (:mounir) from comment #3)
> (In reply to Jonas Sicking (:sicking) from comment #2)
> > Permissions are per origin+app, no? I.e. "google.com within the facebook app
> > has access to geolocation and camera". Remember that this is a low-level API
> > which we can build various UIs on top of.
> 
> Indeed. We could allow not specifying the origin to give permissions to the
> app's origin though.

Oh... actually, we might *NEED* to make a difference between addPermission(app, origin) and addPermission(app) even when orinig == app's origin. For trusted app, if we keep the "app://" plan, we might want to make a difference between addPermission(exampleApp) and addPermission(exampleApp, "http://example.org"). Though, you could require using addPermission(exampleApp, app://example.org") but I would prefer to avoid that.

Comment 6

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

Why do you prefer to avoid addPermission(exampleApp, "app://example.org")? When it comes to security, explicit is always better.

Comment 7

[Mounir Lamouri (:mounir)]

(In reply to Jonas Sicking (:sicking) from comment #6)
> Why do you prefer to avoid addPermission(exampleApp, "app://example.org")?
> When it comes to security, explicit is always better.

I wouldn't say that addPermission(exampleApp, "app://example.org") is more explicit than addPermission(exampleApp). If you say "I want to give permissions to that app to do Foo", you are more explicit that "I want to give permission to that origin in that app to do Foo, where the origin is actually the app's origin".
One hint would be the UI we could build. What is the more understandable:
- "Do you want to allow Example App to access your camera?"
- "Do you want to allow app://example.org from Example App to access your camera?"

Maybe we could simply have two methods: addPermissionToApp() and addPermissionToOriginInApp()?

Comment 8

[David Dahl :ddahl]

Gregor:
Let me know if you need any help to get this bug moved forward.

Comment 9

[Gregor Wagner [:gwagner]]

Attachment: WiP

Comment 10

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 11

[Gregor Wagner [:gwagner]]

Attachment: patch

And with the right packaging.

Comment 12

[David Dahl :ddahl]

What is "aBrowserFlag"?

+  set: function set(aPermission, aManifestURL, aOrigin, aValue, aBrowserFlag) {

Comment 13

[Gregor Wagner [:gwagner]]

(In reply to David Dahl :ddahl from comment #12)
> What is "aBrowserFlag"?
> 
> +  set: function set(aPermission, aManifestURL, aOrigin, aValue,
> aBrowserFlag) {

AFAIK: This indicates if you ask for permission for a site in the browser or as an app.
Facebook for example can be a standalone app or opened in the browser.

Comment 14

[Gregor Wagner [:gwagner]]

jonas ping :)

Comment 15

[Gregor Wagner [:gwagner]]

Attachment: patch

update

Comment 16

[Gregor Wagner [:gwagner]]

Attachment: patch

now with appID

Comment 17

[Gregor Wagner [:gwagner]]

Attachment: patch

update

Comment 18

[Gregor Wagner [:gwagner]]

I can't set a permission in the child process. I will rewrite the patch to do perform a sync set via the parent process.

Comment 19

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 20

[David Dahl :ddahl]

Attachment: ddahl-tweaked-patch

Gregor: I am trying to get the PermissionSettings interface to operate when loading via Cc[...]

I have tweaked the patch, and the test does have permissions, but now the set() method is failing as it seems to misinterpret Ci.nsIPermissionManager.ALLOW_ACTION as a non-valid permission type. I will post my latest patch to bug 758269 as well.

Comment 21

[David Dahl :ddahl]

Comment on attachment 650698 [details] [diff] [review]
patch

I have been using this patch in my work on bug 758269, here is some feedback:

>diff -r a96d79dd9d2c dom/permission/PermissionPromptHelper.jsm

One thing I am unsure of here is why is this a "jsm singleton" instead of an xpcom service? don't we need native code callers as well? 

>--- a/dom/permission/PermissionPromptHelper.jsm	Thu Aug 09 11:34:57 2012 -0700
>+++ b/dom/permission/PermissionPromptHelper.jsm	Thu Aug 09 15:24:44 2012 -0700
>@@ -41,16 +41,17 @@ XPCOMUtils.defineLazyGetter(this, "ppmm"
> var permissionManager = Cc["@mozilla.org/permissionmanager;1"].getService(Ci.nsIPermissionManager);
> var secMan = Cc["@mozilla.org/scriptsecuritymanager;1"].getService(Ci.nsIScriptSecurityManager);
> var appsService = Cc["@mozilla.org/AppsService;1"].getService(Ci.nsIAppsService);
> 
> let PermissionPromptHelper = {
>   init: function() {
>     debug("Init");
>     ppmm.addMessageListener("PermissionPromptHelper:AskPermission", this);
>+    ppmm.addMessageListener("PermissionPromptHelper:AddPermission", this);
>     Services.obs.addObserver(this, "profile-before-change", false);
>   },
> 
>   askPermission: function(aMessage, aCallbacks) {
>     let msg = aMessage.json;
>     debug("askPerm: " + JSON.stringify(aMessage.json));
>     let uri = Services.io.newURI(msg.origin, null, null);
>     let principal = secMan.getAppCodebasePrincipal(uri, msg.appID, msg.browserFlag);
>@@ -64,34 +65,71 @@ let PermissionPromptHelper = {
>         aCallbacks.cancel();
>         return;
>     }
> 
>   // FIXXMEE PROMPT MAGIC! Bug 773114.
>   // We have to diplay the prompt here.

See the patch here for my take on how to handle the prompting inside of b2g/components/ContentPermissionPrompt.js, which I based on an email description from dougt: https://bugzilla.mozilla.org/attachment.cgi?id=653928&action=diff#a/b2g/components/ContentPermissionPrompt.js_sec2

>   },
> 
>+  addPermission: function(aData, aCallbacks) {
>+    let uri = Services.io.newURI(aData.origin, null, null);
>+    let appID = appsService.getAppLocalIdByManifestURL(aData.manifestURL);
>+    let principal = secMan.getAppCodebasePrincipal(uri, appID, aData.browserFlag);
>+
>+    let action;
>+    switch (aData.value)
>+    {
>+      case "unknonwn":

spelling issue: "unknown"


>   receiveMessage: function(aMessage) {
>     debug("PermissionPromptHelper::receiveMessage " + aMessage.name);
>     let mm = aMessage.target.QueryInterface(Ci.nsIFrameMessageManager);
>     let msg = aMessage.data;
> 

I was under the impression we could get away with just using mozContentEvent and mozChromeEvent for all of the back and forth - perhaps at least for b2g? Is this required for XUL fennec?

>diff -r a96d79dd9d2c dom/permission/PermissionSettings.js
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/dom/permission/PermissionSettings.js	Thu Aug 09 15:24:44 2012 -0700

I found that I needed to use this interface directly in order to test permissions in this chrome mochitest: https://bugzilla.mozilla.org/attachment.cgi?id=654245&action=diff#a/dom/tests/mochitest/webapps/jshelper.js_sec2

This of course, does not fire the init() method, which sets the window that establishes the principal for using the permissions manager, stranding the API in the test.

I attempted a hack around this in my patch uploaded here: https://bugzilla.mozilla.org/attachment.cgi?id=654244&action=diff#a/dom/permission/PermissionSettings.js_sec2  see line 56

Comment 22

[Mounir Lamouri (:mounir)]

Comment on attachment 650698 [details] [diff] [review]
patch

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

::: dom/interfaces/permission/nsIDOMPermissionSettings.idl
@@ +1,5 @@
> +/* 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/. */
> +
> +#include "domstubs.idl"

#include "nsISupports.idl"

@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "domstubs.idl"
> +
> +interface nsIDOMDOMRequest;

You don't use that, do you?

@@ +10,5 @@
> +interface nsIDOMPermissionSettings : nsISupports
> +{
> +  DOMString get(in DOMString permission, in DOMString access, in DOMString manifestURI, in DOMString origin, in bool browserFlag);
> +
> +  void set(in DOMString permission, in DOMString access, in DOMString manifestURI, in DOMString origin, in DOMString value, in bool browserFlag);

Why |origin| and |browserFlag|? Don't you just want to set/get permissions for an app? Why do you want to do that for anything else?

::: dom/permission/PermissionPromptHelper.jsm
@@ +70,5 @@
>    // We have to diplay the prompt here.
>    },
>  
> +  addPermission: function(aData, aCallbacks) {
> +    let uri = Services.io.newURI(aData.origin, null, null);

I don't think you need to ask for the origin, you can get the origin of the app.

@@ +72,5 @@
>  
> +  addPermission: function(aData, aCallbacks) {
> +    let uri = Services.io.newURI(aData.origin, null, null);
> +    let appID = appsService.getAppLocalIdByManifestURL(aData.manifestURL);
> +    let principal = secMan.getAppCodebasePrincipal(uri, appID, aData.browserFlag);

If you want to set a permission for an app, why do you want to use the browserFlag?

::: dom/permission/PermissionSettings.js
@@ +37,5 @@
> +
> +PermissionSettings.prototype = {
> +  get: function get(aPermission, aAccess, aManifestURL, aOrigin, aBrowserFlag) {
> +    debug("Get called with: " + aPermission + ", " + aAccess + ", " + aManifestURL + ", " + aOrigin + ", " + aBrowserFlag);
> +    if (this.hasPrivileges) {

if (!this.hasPrivileges) {
  dump(" blah ");
  return;
}

@@ +38,5 @@
> +PermissionSettings.prototype = {
> +  get: function get(aPermission, aAccess, aManifestURL, aOrigin, aBrowserFlag) {
> +    debug("Get called with: " + aPermission + ", " + aAccess + ", " + aManifestURL + ", " + aOrigin + ", " + aBrowserFlag);
> +    if (this.hasPrivileges) {
> +      let uri = Services.io.newURI(aOrigin, null, null);

app's origin should be enough

@@ +41,5 @@
> +    if (this.hasPrivileges) {
> +      let uri = Services.io.newURI(aOrigin, null, null);
> +      let appID = appsService.getAppLocalIdByManifestURL(aManifestURL);
> +      let principal = secMan.getAppCodebasePrincipal(uri, appID, aBrowserFlag);
> +      let result = permissionManager.testExactPermissionFromPrincipal(principal, aPermission);

no need for aBrowserFlag

@@ +66,5 @@
> +  },
> +
> +  set: function set(aPermission, aAccess, aManifestURL, aOrigin, aValue, aBrowserFlag) {
> +    debug("Set called with: " + aPermission + ", " + aAccess + ", " + aManifestURL + ", " + aOrigin + ",  " + aValue + ", " + aBrowserFlag);
> +    if (this.hasPrivileges) {

ditto

::: dom/permission/tests/Makefile.in
@@ +16,5 @@
> +
> +include $(topsrcdir)/config/rules.mk
> +
> +_TEST_FILES = \
> +  test_permission_basics.html \

we don't do that anymore!

Use MOCHITEST_FILES.

::: dom/permission/tests/test_permission_basics.html
@@ +48,5 @@
> +    SimpleTest.finish();
> +  }
> +}
> +
> +var gPermissionssEnabled = SpecialPowers.getBoolPref("dom.mozPermissionSettings.enabled");

Given that you set the pref before... why do you do this? or did you wanted to reset the pref and re-call permissionTest() so both cases are tested?

::: netwerk/base/public/nsIPermissionManager.idl
@@ +49,5 @@
>    const PRUint32 ALLOW_ACTION = 1;
>    const PRUint32 DENY_ACTION = 2;
>  
> +  const PRUint32 PROMPT_ACTION = 3;
> +  const PRUint32 PROMPT_REMEMBER_ACTION = 4;

Can you explain why we want that?
This has always been done depending on the context. For example, UNKNOWN_ACTION could be used to know we have to prompt. Depending on other things, we know if we should show a remember option or not.

Comment 23

[David Dahl :ddahl]

Attachment: ddahl-tweaked-patch-unbitrot

For testing and feedback

Comment 24

[David Dahl :ddahl]

Attachment: ddahl-tweaked-patch-unbitrot

forgot to update the idl changeing PRUint32 to uint32_2

Comment 25

[David Dahl :ddahl]

In attempting to fix bug 758269, I am running into some issues with bugs from the patch here.

1. checking this.hasPrivileges in PermissionSettings.js always returns false in my tests - I have tried Mochitest-plain and now am trying to test things via Mochitest browser chrome. I have resorted to temp. setting it to true. I have wasted way too much time trying to do things like:

    Services.prefs.setBoolPref("dom.mozPermissionSettings.enabled", true);
    SpecialPowers.addPermission("permissions", true, browser.contentWindow.document); 

etc... to no avail. The latest patch will be uploaded to bug 758269

2. I am seeing that this API appears to mismatch the 'appID' on set() and then get():

// debugging output logs:

// set the permission:
-*- Permission Prompt Helper component: PermissionPromptHelper::receiveMessage PermissionPromptHelper:AddPermission
-*- Permission Prompt Helper component: addPermission
-*- Permission Prompt Helper component: addPermisssion appID: 0
//////////////////////////////////////////// app id is "zero" ^ ///////////////////////
-*- Permission Prompt Helper component: principal: [xpconnect wrapped (nsISupports, nsIPrincipal, nsISerializable) @ 0x2b8409d40c80 (native @ 0x2b8409d40c10)]
-*- Permission Prompt Helper component: principal.URL: http://mochi.test:8888/
-*- Permission Prompt Helper component: aData.value: allow
-*- Permission Prompt Helper component: aData.type: sms-read
-*- Permission Prompt Helper component: ACTION: 1
-*- Permission Prompt Helper component: TYPE: sms-read
-*- Permission Prompt Helper component: VALUE: allow
-*- Permission Prompt Helper component: add: http://mochi.test:8888 0 1 sms-read allow

// GET the permission to verify it is installed correctly

-*- PermissionSettings: Get called with: sms-read, null, http://mochi.test:8888/browser/dom/tests/browser/test-webapp.webapp, http://mochi.test:8888/, true
-*- PermissionSettings: uri: http://mochi.test:8888/
-*- PermissionSettings: appID: 6
/////////////////////// appID is 6 now? WTF? ///////////////////////////////////////////
-*- PermissionSettings: principal: http://mochi.test:8888
-*- PermissionSettings: permission: sms-read
-*- PermissionSettings: result: 0

I assume this has to do with AppService.getAppLocalIdByManifestURL in dom/apps/src/AppService

tl;dr; PLEASE Comment each method of your code and follow the style guide
<rant>
Upon inspection of the tests in dom/tests/webapps I see no tests that verify this app ID and other properties of apps. This Apps code is not tested thoroughly to say the least - as well as next to no method and argument comments throughout the source of the Apps code. I am seeing this more and more in b2g and dom and it is doing us a disservice.

The style guide is here for reference: https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style?redirectlocale=en-US&redirectslug=Mozilla_Coding_Style_Guide#JavaScript_Practices

For coding style best practices in action, refer to the devtools code: 
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/highlighter/highlighter.jsm
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/HUDService.jsm
</rant>

Comment 26

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

(In reply to David Dahl :ddahl from comment #25)
> Upon inspection of the tests in dom/tests/webapps I see no tests that verify
> this app ID and other properties of apps. This Apps code is not tested
> thoroughly to say the least - as well as next to no method and argument
> comments throughout the source of the Apps code.

I got the same sense, but those tests are hard to read, which makes them hard to enhance, so over in bug 785545 I'm unrefactoring them (and making other changes) for maximum readability (and some improved reliability).  Once that patch lands, I'll be happy to look into any cases of missing test coverage you point me at.


> I am seeing this more and
> more in b2g and dom and it is doing us a disservice.

I understand the frustration, but B2G is at a very different stage of its lifespan than the mature Firefox product for which Mozilla's coding style guide and software development practices are designed.  It is a new product, its first version hasn't yet shipped, its market viability is unknown, and it has a limited window of opportunity to prove itself.

So it makes perfect sense for it to adopt practices that maximize its short-term ability to ship over code hygiene and its long-term ability to evolve, which can include writing code that violates conventions, isn't sufficiently documented, and isn't well-enough tested automatically.

The original developers of Firefox did exactly the same thing.  And the much larger team of us who spent years rewriting their code after the success of Firefox 1.0 often grumbled about its quality.  But they did what they needed to do to finish Firefox in time for it to succeed and give us the luxury of being able to kvetch about it later.

Comment 27

[David Dahl :ddahl]

Attachment: Unbitrot 8-31, ddahl

Gregor: unbitrot patch while working on bug 758269

Comment 28

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Haven't read the impl here, but this needs cross-process permissions checks.  Otherwise, all the lock-down work we've done is in vain, because a misbehaving app process can just grant itself permissions for everything.

philikon, is the mm helper ready yet?

Comment 29

[Philipp von Weitershausen [:philikon]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #28)
> philikon, is the mm helper ready yet?

Almost. Bug 776832.

Comment 30

[Andrew Overholt [:overholt]]

Updating "priority" to reflect the fact that it blocks a bug of previously-higher priority.

Comment 31

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

Comment on attachment 650698 [details] [diff] [review]
patch

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

r=me with that fixed. Though I don't quite understand why you are letting the permissions prompt helper handle the parent process part of this. Seems a bit hacky to me.

::: dom/permission/PermissionPromptHelper.jsm
@@ +72,5 @@
>  
> +  addPermission: function(aData, aCallbacks) {
> +    let uri = Services.io.newURI(aData.origin, null, null);
> +    let appID = appsService.getAppLocalIdByManifestURL(aData.manifestURL);
> +    let principal = secMan.getAppCodebasePrincipal(uri, appID, aData.browserFlag);

This is correct. We don't want the origin of the app. This is intended to be a JS API to the permission manager as it is.

This code looks fine.

@@ +90,5 @@
> +      case "prompt":
> +        action = Ci.nsIPermissionManager.PROMPT_ACTION;
> +        break;
> +      case "prompt-remember":
> +        action = Ci.nsIPermissionManager.PROMPT_REMEMBER_ACTION;

Remove this one.

@@ +125,5 @@
> +          }
> +        });
> +        break;
> +      case "PermissionPromptHelper:AddPermission":
> +        this.addPermission(msg);

You need to add checks here to ensure that a hacked process can't get access to this API. But maybe that is blocked on bug 776832?

::: dom/permission/PermissionSettings.js
@@ +37,5 @@
> +
> +PermissionSettings.prototype = {
> +  get: function get(aPermission, aAccess, aManifestURL, aOrigin, aBrowserFlag) {
> +    debug("Get called with: " + aPermission + ", " + aAccess + ", " + aManifestURL + ", " + aOrigin + ", " + aBrowserFlag);
> +    if (this.hasPrivileges) {

This shouldn't be needed at all. See below.

@@ +41,5 @@
> +    if (this.hasPrivileges) {
> +      let uri = Services.io.newURI(aOrigin, null, null);
> +      let appID = appsService.getAppLocalIdByManifestURL(aManifestURL);
> +      let principal = secMan.getAppCodebasePrincipal(uri, appID, aBrowserFlag);
> +      let result = permissionManager.testExactPermissionFromPrincipal(principal, aPermission);

This is correct.

@@ +54,5 @@
> +          return "deny";
> +        case Ci.nsIPermissionManager.PROMPT_ACTION:
> +          return "prompt";
> +        case Ci.nsIPermissionManager.PROMPT_REMEMBER_ACTION:
> +          return "prompt-remember";

Remove prompt-remember.

@@ +66,5 @@
> +  },
> +
> +  set: function set(aPermission, aAccess, aManifestURL, aOrigin, aValue, aBrowserFlag) {
> +    debug("Set called with: " + aPermission + ", " + aAccess + ", " + aManifestURL + ", " + aOrigin + ",  " + aValue + ", " + aBrowserFlag);
> +    if (this.hasPrivileges) {

Same here.

@@ +87,5 @@
> +    if (!Services.prefs.getBoolPref("dom.mozPermissionSettings.enabled"))
> +      return null;
> +
> +    let perm = Services.perms.testExactPermissionFromPrincipal(aWindow.document.nodePrincipal, "permissions");
> +    this.hasPrivileges = perm == Ci.nsIPermissionManager.ALLOW_ACTION;

If this app doesn't have access to permissions, then you should just return null here instead. That both makes it easier for an app to see if it can use this API, and it means that there's only one place where we have to get the security checks right. That way you can also remove this.hasPrivileges.

Comment 32

[Gregor Wagner [:gwagner]]

Attachment: patch

I created a new module and don't reuse the permissionPromptHelper.

Comment 33

[Gregor Wagner [:gwagner]]

Attachment: patch

update

Comment 34

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

Comment on attachment 662288 [details] [diff] [review]
patch

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

\o/

Comment 35

[Gregor Wagner [:gwagner]]

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

Comment 36

[David Dahl :ddahl]

This was landed with a misspelling of a variable:

    1.47 +    switch (aData.value)
    1.48 +    {
    1.49 +      case "unknonwn":
    1.50 +        action = Ci.nsIPermissionManager.UNKNOWN_ACTION;
    1.51 +        break;

https://hg.mozilla.org/integration/mozilla-inbound/diff/5f3887536eef/dom/permission/PermissionSettings.jsm#l1.49

Comment 37

[Bill Gianopoulos [:WG9s]]

Is that why it turned mochitest-3 orange on all major platforms?

Comment 38

[Gregor Wagner [:gwagner]]

Backout
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a218efe1e3a

Comment 39

[Gregor Wagner [:gwagner]]

(In reply to Bill Gianopoulos [:WG9s] from comment #37)
> Is that why it turned mochitest-3 orange on all major platforms?

No

Comment 40

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa121eb77862

Comment 41

[David Dahl :ddahl]

After rebasing bug 758269 with this patch 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 42

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/aa121eb77862

Comment 43

[David Dahl :ddahl]

With the latest patch I am getting this error, I think on uninstall:

JavaScript error: resource://gre/modules/Webapps.jsm, line 878: TypeError: aData.mm.broadcastMessage is not a function

Comment 44

[[:fabrice] Fabrice Desré]

(In reply to David Dahl :ddahl from comment #43)
> With the latest patch I am getting this error, I think on uninstall:
> 
> JavaScript error: resource://gre/modules/Webapps.jsm, line 878: TypeError:
> aData.mm.broadcastMessage is not a function

Yes, this one went unnoticed when refactoring Webapps.jsm. Can you file a bug? This should be aData.mm.sendAsyncMessage(...)

Comment 45

[David Dahl :ddahl]

I think there is another bug now as well - my install checks all fail, and yet when i fire up the debugger and check for the values in the database, they are there. It seems like the onsuccess DOMRequest event is being fired to early, you can test this with the patch in bug 758269

Put a call to Math.sin() and a breakpoint in gdb (break js::math_sin) in the test (dom/tests/browser/browser_webapps_permissions.js) right before uninstallApp() is called, of course, you also have to fix the issue in the above comment

Comment 46

[David Dahl :ddahl]

(In reply to David Dahl :ddahl from comment #45)
> I think there is another bug now as well - my install checks all fail, and

gwagner and I figured this out on irc.

Btw: was the final patch attached to this bug yet? Was it landed on m-c?

Comment 47

[Gregor Wagner [:gwagner]]

(In reply to David Dahl :ddahl from comment #46)
> (In reply to David Dahl :ddahl from comment #45)
> > I think there is another bug now as well - my install checks all fail, and
> 
> gwagner and I figured this out on irc.
> 
> Btw: was the final patch attached to this bug yet? Was it landed on m-c?

Yes, everything should be in m-c now.