Closed Bug 1216684 Opened 4 years ago Closed 4 years ago

Notification permissions cannot be granted/denied in private windows

Categories

(Toolkit :: Notifications and Alerts, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 --- unaffected
firefox44 --- fixed

People

(Reporter: MattN, Assigned: jaws)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

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.
We should consider a private window only allow option for this scenario or automatically deny the request (which could have web compat issues).
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.
(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.
Attached patch Patch (obsolete) — Splinter Review
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).
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #8676615 - Flags: review?(MattN+bmo)
Attached patch Patch (qref'd) (obsolete) — Splinter Review
(I guess it would help if I qrefreshed the patch before uploading)
Attachment #8676615 - Attachment is obsolete: true
Attachment #8676615 - Flags: review?(MattN+bmo)
Attachment #8676616 - Flags: review?(MattN+bmo)
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.
Attachment #8676616 - Flags: review?(MattN+bmo) → feedback+
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.
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.
Attached patch Patch v1.1Splinter Review
Attachment #8676616 - Attachment is obsolete: true
Attachment #8677038 - Flags: review?(MattN+bmo)
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".
Attachment #8677038 - Flags: review?(MattN+bmo) → review+
(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.
https://hg.mozilla.org/integration/fx-team/rev/0393e5088a20f01fbc0bef806005d6c3126b64ac
Bug 1216684 - Notification permissions cannot be granted/denied in private windows. r=MattN
https://hg.mozilla.org/mozilla-central/rev/0393e5088a20
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Flags: in-qa-testsuite+
You need to log in before you can comment on or make changes to this bug.