Closed Bug 1233387 Opened 9 years ago Closed 9 years ago

Removing website from the list of websites allowed to send notifications doesn't have any effect

Categories

(Core :: DOM: Push Subscriptions, defect)

Product:
Core
Component:
DOM: Push Subscriptions
Version:
44 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox46 --- affected

People

(Reporter: marco, Unassigned)

Details

Steps to reproduce: 1) Open https://serviceworke.rs/push-subscription-management_demo.html 2) Click on subscribe 3) You'll start receiving notifications every ~5 seconds 4) Open about:preferences#content 5) Remove https://serviceworke.rs/ from the list of websites allowed to send notifications I'd expect to stop receiving notifications, but I'm still receiving them. I'd expect the 'pushsubscriptionchange' event to be triggered, but it isn't. I'd expect the push service to return an error when the server sends the notification, but it doesn't.
Oops, I didn't click "Save Changes"...
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
In Firefox 46 all my expectations are satisfied. In Firefox 45, the 'pushsubscriptionchange' event isn't triggered and the push service doesn't return an error. So I guess the subscription is still active, but notifications are disabled. Can we backport the fix to Firefox 45?
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Version: 46 Branch → 45 Branch
Sorry, Developer Edition is still 44.
Version: 45 Branch → 44 Branch
Flags: needinfo?(kcambridge)
Ouch. `pushsubscriptionchange` received quite an overhaul (bug 1205109, bug 1219063, bug 1223202), so I'm nervous about uplifting all those patches to 44. That said, they do include test coverage, they've been on Nightly for about a month, and you verified they're working for you. I'm not sure what the right call is here. What do you think, Ritu?
Flags: needinfo?(kcambridge) → needinfo?(rkothari)
Kit: I haven't looked at the patches on these bugs, but it's good to see that fixes for bug 1205109 and bug 1219063 have landed in Nightly since 6 weeks. Bug 1223202 hasn't made it into Nightly yet. Kit, Matt, Edwin: Are these changes planned for 44? I know that push notifications was planned for FF44 but were these bugs Kit listed in the scope? Do you think it makes sense to uplift to 44? We could take it if it helps improve usability/functionality/stability of this feature as we are only just starting out on the beta44 cycle.
Flags: needinfo?(rkothari)
Flags: needinfo?(edwin)
Flags: needinfo?(MattN+bmo)
It can be confusing to developers (I've got an email from https://www.pushwoosh.com/ developers that were asking how Firefox handles unsubscribing), so I think it's useful if the risk is low.
We talked about these patches a few weeks ago at our standup, and decided to let them ride the trains unless developers ran into issues. Per Marco's comment, it sounds like that's the case. Handling `pushsubscriptionchange` correctly is tricky, especially without dev tools support. A buggy implementation doesn't help. I attached new patches to those bugs, and requested uplifts to 44. The order is bug 1205109, bug 1219063, and bug 1223202. Patch #4 (bug 1223202) needed some tweaks to apply cleanly.
Flags: needinfo?(edwin) → needinfo?(edwong)
:kit this sounds good to me. Let's uplift if possible.
Flags: needinfo?(edwong)
To test on Beta, the `dom.push.enabled` and `dom.webnotifications.serviceworker.enabled` prefs need to be set in `about:config`. Once the blockers are resolved, we'll enable these prefs by default in bug 1234054.
Flags: needinfo?(MattN+bmo)
The patches have been uplifted. Thank you!
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.