Closed Bug 770731 Opened 12 years ago Closed 12 years ago

Expose JS API for modifying app permissions

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: gwagner, Assigned: gwagner)

References

Details

(Keywords: feature, Whiteboard: [WebAPI:P1], [LOE:S], [qa-])

Attachments

(1 file, 12 obsolete files)

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)
Why do we need origin? The app manifest already contains it.

Also, I'm not sure what "prompt-remember" means.
OS: Mac OS X → All
Hardware: x86 → All
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.
(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.
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.
(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.
Why do you prefer to avoid addPermission(exampleApp, "app://example.org")? When it comes to security, explicit is always better.
(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()?
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
Assignee: nobody → anygregor
Gregor:
Let me know if you need any help to get this bug moved forward.
Attached patch WiP (obsolete) — Splinter Review
Attached patch patch (obsolete) — Splinter Review
Attachment #643949 - Attachment is obsolete: true
Attachment #643983 - Flags: review?(jonas)
Attached patch patch (obsolete) — Splinter Review
And with the right packaging.
Attachment #643983 - Attachment is obsolete: true
Attachment #643983 - Flags: review?(jonas)
Attachment #643994 - Flags: review?(jonas)
What is "aBrowserFlag"?

+  set: function set(aPermission, aManifestURL, aOrigin, aValue, aBrowserFlag) {
(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.
jonas ping :)
Attached patch patch (obsolete) — Splinter Review
update
Attachment #643994 - Attachment is obsolete: true
Attachment #643994 - Flags: review?(jonas)
Attachment #648838 - Flags: review?(jonas)
Attachment #648838 - Attachment is patch: true
Attached patch patch (obsolete) — Splinter Review
now with appID
Attachment #648838 - Attachment is obsolete: true
Attachment #648838 - Flags: review?(jonas)
Attachment #648858 - Flags: review?(jonas)
Attached patch patch (obsolete) — Splinter Review
update
Attachment #648858 - Attachment is obsolete: true
Attachment #648858 - Flags: review?(jonas)
Attachment #649327 - Flags: review?(jonas)
Blocks: 758269
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.
Attached patch patch (obsolete) — Splinter Review
Attachment #649327 - Attachment is obsolete: true
Attachment #649327 - Flags: review?(jonas)
Attachment #650698 - Flags: review?(jonas)
No longer blocks: basecamp-security
Attached patch ddahl-tweaked-patch (obsolete) — Splinter Review
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.
Attachment #654244 - Flags: review?(anygregor)
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 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.
Attached patch ddahl-tweaked-patch-unbitrot (obsolete) — Splinter Review
For testing and feedback
Attachment #654244 - Attachment is obsolete: true
Attachment #654244 - Flags: review?(anygregor)
Attached patch ddahl-tweaked-patch-unbitrot (obsolete) — Splinter Review
forgot to update the idl changeing PRUint32 to uint32_2
Attachment #655664 - Attachment is obsolete: true
Attachment #655710 - Flags: feedback?(doug.turner)
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>
(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.
Attached patch Unbitrot 8-31, ddahl (obsolete) — Splinter Review
Gregor: unbitrot patch while working on bug 758269
Attachment #655710 - Attachment is obsolete: true
Attachment #655710 - Flags: feedback?(doug.turner)
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?
Summary: Expose JS API for settings app permissions → Expose JS API for modifying app permissions
Whiteboard: [WebAPI:P3]
(In reply to Chris Jones [:cjones] [:warhammer] from comment #28)
> philikon, is the mm helper ready yet?

Almost. Bug 776832.
Keywords: feature
Updating "priority" to reflect the fact that it blocks a bug of previously-higher priority.
Whiteboard: [WebAPI:P3] → [WebAPI:P1]
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.
Attachment #650698 - Flags: review?(jonas) → review+
Whiteboard: [WebAPI:P1] → [WebAPI:P1][LOE:S]
Attached patch patch (obsolete) — Splinter Review
I created a new module and don't reuse the permissionPromptHelper.
Attachment #650698 - Attachment is obsolete: true
Attachment #662221 - Flags: review?(jonas)
Attached patch patchSplinter Review
update
Attachment #662221 - Attachment is obsolete: true
Attachment #662221 - Flags: review?(jonas)
Attachment #662288 - Flags: review?(jonas)
Comment on attachment 662288 [details] [diff] [review]
patch

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

\o/
Attachment #662288 - Flags: review?(jonas) → review+
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
Is that why it turned mochitest-3 orange on all major platforms?
(In reply to Bill Gianopoulos [:WG9s] from comment #37)
> Is that why it turned mochitest-3 orange on all major platforms?

No
Whiteboard: [WebAPI:P1][LOE:S] → [WebAPI:P1], [LOE:S], [qa-]
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
Depends on: 792604
https://hg.mozilla.org/mozilla-central/rev/aa121eb77862
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
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
(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(...)
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
Depends on: 792882
Depends on: 792896
No longer depends on: 792896
Depends on: 793198
(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?
Attachment #657422 - Attachment is obsolete: true
(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.
Flags: sec-review?(curtisk)
Flags: sec-review?(curtisk) → sec-review+
Depends on: 866441
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: