1216684 - Notification permissions cannot be granted/denied in private windows

Status: RESOLVED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect)

Description

[Matthew N. [:MattN]]

Attachment: Screen Shot of the permission doorhanger with no buttons

Because we removed the session option in bug 1192458, there are no actions remaining for private windows since we can't set persistent permissions from them.

Comment 1

[Matthew N. [:MattN]]

We should consider a private window only allow option for this scenario or automatically deny the request (which could have web compat issues).

Comment 2

[Bill Maggs (bmaggs)]

I don't understand. Don't we prevent Service Workers from working in PBM? It's no good to grant permission if they can't receive a SW-based Notification anyway.

Comment 3

[Lina Butler [:lina]]

(In reply to Bill Maggs (bmaggs) from comment #2)
> I don't understand. Don't we prevent Service Workers from working in PBM?

We do, but scripts and (IIRC) web workers can still show vanilla notifications. With that restriction in mind, I think it makes sense to have an "allow" option for private windows.

Anecdotally, I use a private window for my personal Gmail, and a non-private one for work. That way, I can stay signed in and get new message notifications for both. Probably not a common scenario, though.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

This patch adds the "Allow for session" but only on private windows. I can write a test for this but I wanted to get your feedback on it first before I write the test.

Note that I had to define SitePermissions.UNKNOWN as one of the states to get "Always Ask" to appear in the dropdowns of control center, even though the documentation within SitePermissions doesn't make this obvious (likely a bug).

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch (qref'd)

(I guess it would help if I qrefreshed the patch before uploading)

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8676616 [details] [diff] [review]
Patch (qref'd)

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

What happens if we leave out the SitePermissions.jsm changes? Since that's complicated enough and probably affects other permissions in PB we may want to move that to a follow-up that the FxPrivacy team can handle. Otherwise it seems like we should probably meet about the JSM changes.

::: browser/modules/SitePermissions.jsm
@@ +166,5 @@
>    },
>  
>    "desktop-notification": {
> +    get states() {
> +      const Cc = Components.classes, Ci = Components.interfaces;

You should probably just put the standard destructuring assignment version with all 4 properties at the top of this file?

@@ +169,5 @@
> +    get states() {
> +      const Cc = Components.classes, Ci = Components.interfaces;
> +      let wm = Cc["@mozilla.org/appshell/window-mediator;1"]
> +                  .getService(Ci.nsIWindowMediator);
> +      let win = wm.getMostRecentWindow("navigator:browser");

Services.wm.

@@ +170,5 @@
> +      const Cc = Components.classes, Ci = Components.interfaces;
> +      let wm = Cc["@mozilla.org/appshell/window-mediator;1"]
> +                  .getService(Ci.nsIWindowMediator);
> +      let win = wm.getMostRecentWindow("navigator:browser");
> +      if (PrivateBrowsingUtils.isWindowPrivate(win)) {

This feels wrong to me since we don't know that the most recent window is the one calling this getter. In the case of the control center panel this is likely correct but not always.

@@ +171,5 @@
> +      let wm = Cc["@mozilla.org/appshell/window-mediator;1"]
> +                  .getService(Ci.nsIWindowMediator);
> +      let win = wm.getMostRecentWindow("navigator:browser");
> +      if (PrivateBrowsingUtils.isWindowPrivate(win)) {
> +        return [ SitePermissions.SESSION, SitePermissions.UNKNOWN ];

I think SitePermissions.SESSION is only really meant for cookies and that may be part of the problem. It seems like it may be useful for SitePermissions.jsm to know about EXPIRE_SESSION (not to be confused with ACCESS_SESSION which should probably be replaced with usage of EXPIRE_SESSION)

@@ +173,5 @@
> +      let win = wm.getMostRecentWindow("navigator:browser");
> +      if (PrivateBrowsingUtils.isWindowPrivate(win)) {
> +        return [ SitePermissions.SESSION, SitePermissions.UNKNOWN ];
> +      }
> +      return [ SitePermissions.ALLOW, SitePermissions.BLOCK, SitePermissions.UNKNOWN ];

I suspect at least one of the other permissions in this file allow permission manager leaks via changing to allow/block from control center. An alternative solution would be teach SitePermissions about EXPIRE_SESSION and have the control center toggle allow toggling between allow and block in PB but retains the EXPIRE_SESSION value present in the DB.

Note that there are plans to replace the dropdown with an [X] to remove in the control center at some point. Not sure if that's on a roadmap though.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

The SitePermissions.jsm changes are necessary for Control Center to show the "Allow for Session" and "Always Ask" options.

If we just land the nsBrowserGlue.js and browser.properties changes, Private Window doorhangers will have the "Allow for Session" and "Always Ask" options but control center will have "Always", "Block", and "Always Ask". When "Allow for Session" is granted in a PB window, non-PB window control center for same domain will show "Allow" as the selected option and PB window will show "Allow". Further, the Notification Permissions popup in preferences will show "Allow" in the row for the domain.

If we are OK with control center and the preferences showing "Allow" instead of "Allow for session" then we could land this change and file the SitePermissions.jsm issue as a follow-up.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

From discussion today during a product meeting, bmaggs said he was fine with the "Allow for Session" in PB windows.

Let's move forward with the patch minus the SitePermissions.jsm changes.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8677038 [details] [diff] [review]
Patch v1.1

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

r+ with fixes

::: browser/components/nsBrowserGlue.js
@@ +2630,5 @@
> +    var browser = this._getBrowserForRequest(aRequest);
> +    var chromeWin = browser.ownerDocument.defaultView;
> +    // Only show "allow for session" in PB mode, we don't
> +    // support "allow for session" in non-PB mode.
> +    if (PrivateBrowsingUtils.isWindowPrivate(chromeWin)) {

isBrowserPrivate avoids the CPOW with e10s

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +380,1 @@
>  webNotifications.alwaysReceive=Always Receive Notifications

This should be "Receive for this session" to match the new wording. Please update the ID too and drop the "2".

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Matthew N. [:MattN] from comment #10)
> Comment on attachment 8677038 [details] [diff] [review]
> Patch v1.1
> 
> Review of attachment 8677038 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with fixes
> 
> ::: browser/components/nsBrowserGlue.js
> @@ +2630,5 @@
> > +    var browser = this._getBrowserForRequest(aRequest);
> > +    var chromeWin = browser.ownerDocument.defaultView;
> > +    // Only show "allow for session" in PB mode, we don't
> > +    // support "allow for session" in non-PB mode.
> > +    if (PrivateBrowsingUtils.isWindowPrivate(chromeWin)) {
> 
> isBrowserPrivate avoids the CPOW with e10s

isBrowserPrivate does the exact same thing that this code is doing. I'll make the change, but I don't see how that would avoid a CPOW if this has a CPOW. Since this is getting the chrome window and not the content window there shouldn't be a CPOW.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/0393e5088a20f01fbc0bef806005d6c3126b64ac
Bug 1216684 - Notification permissions cannot be granted/denied in private windows. r=MattN

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0393e5088a20

Comment 14

[Florencia Di Ciocco, NI to vbalducci]

Attachment: Snag_36f22d6.png

Hi,
On macOs Big Sur version 11.0.1, on Beta 84.0b6 this still happens. On the site https://serviceworke.rs/push-simple_demo.html in a private browser, I do not get the notification warning.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

This bug was fixed 5 years ago. Please open a new bug for the issue you're seeing.

Comment 16

[Florencia Di Ciocco, NI to vbalducci]

Thanks Ryan, I opened the bug 1680540.