1881812 - ServiceWorkerRegistration.getNotifications is based on the scope rather than on the registration

Status: NEW

(Core :: DOM: Notifications, defect)

Description

[Kagami Rosylight [:saschanaz] (they/them)]

Unregistering should dissociate the notifications (and also close them) but it does not.

Comment 1

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1881812 - Add a test for SWR assocation with notifications r=asuth

Comment 2

[Kagami Rosylight [:saschanaz] (they/them)]

Hmm, but Safari doesn't do that, which makes 2:1 situation. I'll just add a tentative test in that case.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

There is an r+ patch which didn't land and no activity in this bug for 2 weeks.
:saschanaz, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Comment 4

[Kagami Rosylight [:saschanaz] (they/them)]

Good bot.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7b9a590cbbb
Add a test for SWR assocation with notifications r=asuth

Comment 6

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/46592 for changes under testing/web-platform/tests

Comment 7

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/d7b9a590cbbb

Comment 8

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 9

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 10

[Kagami Rosylight [:saschanaz] (they/them)]

We landed a test but the behavior is still the same.

Comment 11

[Kagami Rosylight [:saschanaz] (they/them)]

And I'm not sure how to proceed here with https://github.com/w3c/push-api/pull/393, which now wants to pair the push subscriptions with scopes instead of SWRs. Do we now want to pair notifications to scopes in the spec too?

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #11)

And I'm not sure how to proceed here with https://github.com/w3c/push-api/pull/393, which now wants to pair the push subscriptions with scopes instead of SWRs. Do we now want to pair notifications to scopes in the spec too?

I think specs defining their data in terms of specific registrations seems desirable if that data can't exist independent of the registration. In particular, I think it results in implicit data clearing/removal when the registration is removed, although arguably specs should probably be defining behavior about removal and adding tests like you landed here. For example the cookie-store incubation proposal defines the CookieStoreManager in terms of registrations by sorta monkey-patching cookie change subscription list onto the registration.

In the push API PR above, the spec text around "Subscription deactivation" handles the clearing appropriately and there's a good reason to define the the subscriptions in this way at the cost of additional spec text. And in particular, if https://github.com/w3c/ServiceWorker/issues/1512 happens to allow non-scope identifiers for registrations, push will need even more spec text to handle this.

Currently it does seem like notifications defines list of notifications in such a way that either they need to be redefined to operate in terms of the registration or specific clearing steps are necessary when a service worker registration is cleared. Currently this is where we trigger the push clearing in Gecko and I believe CookieStoreManager will hook at that location shortly too. Of course, it's fine for the spec to be that the notifications are stored on the registration but we in fact store them separately, keyed by scope, and that we purge them based on scope in our clear registration implementation.

Restating context: It looks like https://github.com/w3c/push-api/pull/368#issuecomment-2241315574 is where I started having opinions in this space and that issue ended up as the linked https://github.com/w3c/push-api/pull/393

Comment 13

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: WIP: Bug 1881812