896748 - Electrolysis: Notification request box doesn't open

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Tom S [:evilpie]]

Notification.requestPermission() throws an exception and doesn't throw an exception.

Comment 1

[Tom S [:evilpie]]

Wow. I meant "and doesn't show a permission request"

Comment 2

[Tom S [:evilpie]]

Attachment: v1

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 783195 [details] [diff] [review]
v1

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js

>+      var requestingWindow = aRequest.window.top;
>+      chromeWin = this._getChromeWindow(requestingWindow).wrappedJSObject;
>+      browser = chromeWin.gBrowser.getBrowserForDocument(requestingWindow.document);
>+      if (!browser) {
>+        // find the requesting browser or iframe
>+        browser = requestingWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>+                                    .getInterface(Ci.nsIWebNavigation)
>+                                    .QueryInterface(Ci.nsIDocShell)
>+                                    .chromeEventHandler;
>+      }

Not your code, but if you're changing this you might as well fix it: we should just always use the chromeEventHandler trick, rather than first trying getBrowserForDocument. I can't think of any cases where it would give a different answer, and chromeEventHandler works in more cases.

>diff --git a/dom/ipc/AppProcessChecker.cpp b/dom/ipc/AppProcessChecker.cpp

> AssertAppProcess(PBrowserParent* aActor,
>                  AssertAppProcessType aType,
>                  const char* aCapability)
> {
>+#ifndef MOZ_WIDGET_GONK
>+// We currently don't want to check permissions of
>+// content processes on Desktop.
>+  return true;
>+#endif

Why not? This seems like a change that merits wider discussion.

>diff --git a/toolkit/modules/PopupNotifications.jsm b/toolkit/modules/PopupNotifications.jsm

>+  _isActiveBrowser: function (browser) {
>+    return browser.docShell
>+      ? browser.docShell.isActive
>+      : (this.window.gBrowser.selectedBrowser == browser);
>+      // Todo: e10s should use something more like docShell.isActive.

The comment should probably specify that the entire reason why this helper exists is for the e10s case, and that otherwise we'd just use .isActive.

Comment 4

[Tom S [:evilpie]]

>Why not? This seems like a change that merits wider discussion.
Browsers are not apps, from my understanding they implement a very different model of how permissions are implemented. This is not the right time to think about the possible security benefits of explicitly checking the security privileges. This is something for the sandboxing era. We already made somewhat similar decision in bug 891954.

I totally agree to the rest.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 783195 [details] [diff] [review]
v1

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

Looks alright, needs an update with the comments from Gavin. And as I understood, the AppProcessChecker.cpp change will be removed and moved to bug 900707, right?

::: browser/components/nsBrowserGlue.js
@@ +1754,3 @@
>  
> +    try {
> +      /* This is only supposed to work with remote tabs. */

what is only supposed to work with remote tabs? is it the existence of aRequest.element?

Comment 6

[Tom S [:evilpie]]

Attachment: v2

Addressed feedback and added better comments. I moved the permission stuff to Bug 900707.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 789128 [details] [diff] [review]
v2

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

::: browser/components/nsBrowserGlue.js
@@ +1764,3 @@
>  
> +    try {
> +      // This is only works in e10s mode, because of "element".

s/is//

@@ +1766,5 @@
> +      // This is only works in e10s mode, because of "element".
> +      browser = aRequest.element;
> +      chromeWin = browser.ownerDocument.defaultView;
> +    } catch (e) {}
> +

chromeWin will now be undefined in non-e10s mode, but it's still used in the function.

I think you can just move the chromeWin = browser... line after the if () below
and not use _getChromeWindow

that means that also you probably don't need to wrap the .element in a try catch, unless that throws in non-e10s?  But please keep the comment saying that it's only used for e10s

Comment 8

[Tom S [:evilpie]]

Attachment: v3

Sorry about not testing a normal build. This is nice then before I think.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7ca66a4c4c5e