Closed
Bug 1216684
Opened 9 years ago
Closed 9 years ago
Notification permissions cannot be granted/denied in private windows
Categories
(Toolkit Graveyard :: Notifications and Alerts, defect)
Toolkit Graveyard
Notifications and Alerts
Tracking
(firefox43 unaffected, firefox44 fixed)
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox43 | --- | unaffected |
firefox44 | --- | fixed |
People
(Reporter: MattN, Assigned: jaws)
References
Details
(Keywords: regression)
Attachments
(3 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.
Reporter | ||
Comment 1•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
(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.
Assignee | ||
Comment 4•9 years ago
|
||
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 | ||
Comment 5•9 years ago
|
||
(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)
Reporter | ||
Comment 6•9 years ago
|
||
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+
Assignee | ||
Comment 7•9 years ago
|
||
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.
Assignee | ||
Comment 8•9 years ago
|
||
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.
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8676616 -
Attachment is obsolete: true
Attachment #8677038 -
Flags: review?(MattN+bmo)
Reporter | ||
Comment 10•9 years ago
|
||
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+
Assignee | ||
Comment 11•9 years ago
|
||
(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.
Assignee | ||
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/0393e5088a20f01fbc0bef806005d6c3126b64ac
Bug 1216684 - Notification permissions cannot be granted/denied in private windows. r=MattN
Comment 13•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Updated•6 years ago
|
Flags: in-qa-testsuite+
Comment 14•4 years ago
|
||
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.
Updated•4 years ago
|
status-firefox84:
--- → affected
Comment 15•4 years ago
|
||
This bug was fixed 5 years ago. Please open a new bug for the issue you're seeing.
status-firefox84:
affected → ---
Flags: needinfo?(fdiciocco)
Comment 16•4 years ago
|
||
Thanks Ryan, I opened the bug 1680540.
No longer blocks: 1680540
Flags: needinfo?(fdiciocco)
Updated•1 year ago
|
Product: Toolkit → Toolkit Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•