824421 - Cannot request access to the desktop notification webapi in the browser - no permission prompt is seen, webapi can't be used

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Jason Smith [:jsmith]]

Build: B2G18 12/23/2012
Device: Unagi

Steps:

1. Go to http://jds2501.github.com/webapi-permissions-tests/webapi_permissions_test.html in the browser
2. Select create notification

Expected:

A permission prompt should appear asking the user if they would like to grant access to the desktop notification webapi.

Actual:

Nothing happens. When I install a hosted app that calls out for desktop notification permissions, that same test works fine. So the permission prompt is probably failing to show in a browser context.

Relevant code from test case:

  var notification = navigator.mozNotification.createNotification(
		"Sample title", "Sample description", "http://jds2501.github.com/webapi-permissions-tests/qalogo.png");
  notification.show();

Comment 1

[Jason Smith [:jsmith]]

And when I mean "works fine" in the actual results I mean I get implicit access to desktop notification webapi and as a result, the desktop notification is fired with the correct name, description, and icon.

However, the permission prompt won't even show up in the browser. And I don't even get access to the webapi in the browser at all, even though the permissions matrix says otherwise.

Comment 2

[Lawrence Mandel [:lmandel] (use needinfo)]

Andrew, can you comment on the expected use of the notification API in the browser context? Should the user see a prompt? Is this API supported in this context?

Comment 3

[Jason Smith [:jsmith]]

Data comparison:

Permissions matrix states the following:

* Web Content - PROMPT_ACTION
* Hosted Web App - ALLOW_ACTION
* Privileged Web App - ALLOW_ACTION
* Certified Web App - ALLOW_ACTION

Permissions Installer code states the following:

"desktop-notification": {
  app: ALLOW_ACTION,
  privileged: ALLOW_ACTION,
  certified: ALLOW_ACTION
},

Current behavior from testing states the following:

* Web Content - DENY_ACTION
* Hosted Web App - ALLOW_ACTION
* Privileged Web App - ALLOW_ACTION
* Certified Web App - ALLOW_ACTION

Comment 4

[Andrew Overholt [:overholt]]

Doug says he'll handle this.  It's definitely something that should be usable from web content.

Comment 5

[William Chen [:wchen]]

Attachment: Added prompt for notification permission requests from web content.

Someone who know more about permissions should take a look in case there is a more appropriate way to hook up permissions for web content. The only other permissions that are exposed to web content are geolocation (currently being treated as a special case) and fmradio (currently not implemented).

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 696403 [details] [diff] [review]
Added prompt for notification permission requests from web content.

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

gregor, can you also look at this?

::: b2g/components/ContentPermissionPrompt.js
@@ +55,1 @@
>        permissionManager.addFromPrincipal(aPrincipal,

This logic is getting a bit complex.  Maybe we should create a new function that takes the permission name and returns a bool:

function shouldAllowPermission(aPermission, aPerm, aPrincipal) {
  let type = permissionManager.testExactPermissionFromPrincipal(aPrincipal, aPerm);

  if (type == Ci.nsIPermissionManager.PROMPT_ACTION) {
    return true;
  }

  if (type != Ci.nsIPermissionManager.UNKNOWN_ACTION) {
    return false;
  }

  if (PROMPT_FOR_UNKNOWN.indexOf(aPermission) >= 0) {
    return true;
  }
  
  if (PROMPT_FOR_WEB_CONTENT.indexOf(aPermission) >= 0) {
    return aPrincipal.isInBrowserElement;
  }

  return false;
}

I think that this is a bit more verbose but more readable.  What do you think?

Comment 7

[Gregor Wagner [:gwagner]]

I think we should combine this PROMPT_FOR_UNKNOWN and PROMPT_FOR_WEB_CONTENT since they mean the same. I will post a patch soon.

Comment 8

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 9

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 10

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66de0c58bdad

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/66de0c58bdad

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/36960406b74b

Comment 13

[Jason Smith [:jsmith]]

Well, I'm getting the perm prompt, I can deny perms, but accepting perms blows up:

01-09 12:28:58.921: E/GeckoConsole(109): [JavaScript Error: "Desktop-notification message app-notification-send from a content process with no desktop-notification privileges." {file: "chrome://browser/content/shell.js" line: 747}]
01-09 12:28:58.931: I/Gecko(109): ###!!! [Parent][AsyncChannel] Error: Channel error: cannot send/recv

Gregor mentions that he's going to fix this as part of bug 823974.

Comment 14

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

Hmm.. I'm not convinced that bug 823974 will fix that problem. But let's verify after that bug is fixed.

Comment 15

[Jason Smith [:jsmith]]

(In reply to Jonas Sicking (:sicking) from comment #14)
> Hmm.. I'm not convinced that bug 823974 will fix that problem. But let's
> verify after that bug is fixed.

Andrea - Were you able to reproduce what I hit in comment 13 with your patch applied from bug 823974?

Comment 16

[Andrea Marchesini [:baku]]

Yes, I see the prompt dialog. I cannot reproduce this bug.

Comment 17

[Andrea Marchesini [:baku]]

Sorry guys, it still crashes.

Comment 18

[Jason Smith [:jsmith]]

Basic flows looks okay. Marking as verified.