1972120 - Do not open origin URL when service worker doesn't exist

Status: NEW

(Core :: DOM: Notifications, enhancement)

Description

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

Context: comments in https://phabricator.services.mozilla.com/D249801

Basically the intention is to prevent user confusion when there's no service worker to open the tab.

But given that:

1. We are fixing the major bug that service worker should now be available to take responsibility to respond on clicks
2. Bug 1972117 covers potential dangling notifications after site data cleanup

The situation to open the origin URL will become significantly lesser, and that brings the question - why do we still need to keep the current behavior?

Thus this bug is to discuss about removing that behavior.

Comment 1

[:Gijs (he/him)]

From phab, on how we can have notifications without serviceworkers:

1. cleanup-on-shutdown, Clearing SW do not clear notifications and such notifications won't have corresponding SW. One could argue that we should also clear notifications, but that would be bigger change with more discussion needed.
2. If we migrate from SW scope to SW registration ID, an ID mismatch. In case somehow a website drops SW or re-register SW somehow.

I'm not that familiar with service workers or notifications.

At first glance, I'd expect that (1) should be fixed by clearing the notifications: if the user explicitly asks to remove traces of a site (or all sites), then that should include notifications, which will largely be things that the user does not otherwise directly control (that is, the user will not typically use browser UI to create a site notification, unlike bookmarks/downloads/saved-credentials where we presume the user has a level of control and we do not delete those when the relevant origin has site data cleared). I guess bug 1972117 covers this. It's framed as a question in the summary there - I think the answer should be "yes" :-)

I don't really understand (2). Can you spell it out a bit more for me (theoretical STR, doesn't need an actual live example) ?

And are there any other cases where this can happen?

On the whole, agreed that user-visible notifications where clicking "does nothing" could be bad. Then again, websites also use notifications where "click" is not that meaningful / not normally a unique way for the user to get somewhere. If the fallback to origin isn't costing us much in terms of code maintenance etc. and isn't likely to be abused, I'd be fine with keeping it - but perhaps there's some other trade-off I'm missing?

Hopefully that helps?

Comment 2

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

(In reply to :Gijs (he/him) from comment #1)

At first glance, I'd expect that (1) should be fixed by clearing the notifications: if the user explicitly asks to remove traces of a site (or all sites), then that should include notifications, which will largely be things that the user does not otherwise directly control (that is, the user will not typically use browser UI to create a site notification, unlike bookmarks/downloads/saved-credentials where we presume the user has a level of control and we do not delete those when the relevant origin has site data cleared). I guess bug 1972117 covers this. It's framed as a question in the summary there - I think the answer should be "yes" :-)

Thanks for the input! My main concern was that this isn't exactly what Firefox manages - it's managed by the OS. That said, if we consider the feature should "remove traces of a site" then it should not be surprising to remove notifications. Not 100% sure that clearing "Cookies and Site Data" implies notifications, but technically site notification is also site "data"...

I don't really understand (2). Can you spell it out a bit more for me (theoretical STR, doesn't need an actual live example) ?

This is not a thing yet, it depends on bug 1881812. The STR would be something like:

await navigator.serviceWorker.register("sw.js");
let registration = await navigator.serviceWorker.ready;

await registration.showNotification("hello");
await registration.unregister();
// -> Now there's a notification but no service worker to respond
// NOTE: but we should probably also clear notifications at this point?

await navigator.serviceWorker.register("sw.js")
// -> Now there's a service worker but with a different registration ID.
//
// Currently clicking notification would trigger the new registration,
// but we want to change that in bug 1881812 so that only a SW with the
// matching ID would run.
//
// Updating service worker script would normally use update() instead
// (which should not change the ID), and doing unregister and register
// probably implies breaking change - incompatibility with older
// notification data.
// 
// (e.g. the site completely drops service worker usage, and months later
// registers a totally different service worker, while a few users may
// still have months-old notifications in their rarely used devices.)

And are there any other cases where this can happen?

That's all I can think of. Perhaps we should somehow strictly bind the notification lifetime with the service worker registration lifetime and then we won't have to deal with this kind of edge case. That's the purpose of bug 1881812 anyway.

On the whole, agreed that user-visible notifications where clicking "does nothing" could be bad. Then again, websites also use notifications where "click" is not that meaningful / not normally a unique way for the user to get somewhere. If the fallback to origin isn't costing us much in terms of code maintenance etc. and isn't likely to be abused, I'd be fine with keeping it - but perhaps there's some other trade-off I'm missing?

On top of Andrew's concerns I also see a potential problem that this code path would be called less and less in the wild that the only real callers would be security researchers. Ideally we should not enter this code path at all, so might as well just remove it when we deal with the known ways to enter the path.

Hopefully that helps?

👍🏻

Comment 3

[Nick Rishel [:nrishel]]

Fallback to origin is also used for handling non-Service Worker notifications, which have nothing to invoke when restarting Firefox. Part of the motivation for that behavior was to create a best effort attempt at doing something useful when notifications are clicked; this behavior matches that of Chrome. We'll need to keep those code path to support that, so I assume to exclude Service Workers would be new logic? My guess is it's simpler not to special case it, and keep the fallback path if for some reason we don't clean up the notification when the Service Worker is removed..

Comment 4

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

Yup, the topic here is only for service-worker-bound notifications. It's already sorta special-cased, while not too complex: https://searchfox.org/mozilla-central/rev/e22715d304f4dcf18c58667b7db3535cfd828f24/dom/notification/NotificationParent.cpp#62-63

if no SW scope
  then open window
else
  ping service worker
  if that failed then open window  // <- removing this is the topic

Comment 5

[Nick Rishel [:nrishel]]

In that case removing this fallback once Bug 1972117 is resolved makes sense to me.

Comment 6

[:Gijs (he/him)]

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

(In reply to :Gijs (he/him) from comment #1)

I don't really understand (2). Can you spell it out a bit more for me (theoretical STR, doesn't need an actual live example) ?

This is not a thing yet, it depends on bug 1881812. The STR would be something like:

await navigator.serviceWorker.register("sw.js");
let registration = await navigator.serviceWorker.ready;

await registration.showNotification("hello");
await registration.unregister();
// -> Now there's a notification but no service worker to respond
// NOTE: but we should probably also clear notifications at this point?

await navigator.serviceWorker.register("sw.js")
// -> Now there's a service worker but with a different registration ID.
//
// Currently clicking notification would trigger the new registration,
// but we want to change that in bug 1881812 so that only a SW with the
// matching ID would run.
//
// Updating service worker script would normally use update() instead
// (which should not change the ID), and doing unregister and register
// probably implies breaking change - incompatibility with older
// notification data.
// 
// (e.g. the site completely drops service worker usage, and months later
// registers a totally different service worker, while a few users may
// still have months-old notifications in their rarely used devices.)

So I think whether or not we need to care about this comes down to "are sites likely to do this [and expect notifications to continue to work]". I don't know what the answer to that question is, I'd defer to folks more familiar with notifications / service workers. If we aren't sure, we could check what other browsers do? It would be unfortunate if we removed this and then we found out that $somepopularsite re-registers its service workers every 5 minutes (or something), for [stupid-reason], and we have to revert.

Comment 7

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

Hmm, so I followed this STR for Chrome:

1. Open https://tests.peter.sh/notification-generator/
2. Trigger a notification
3. Reopen Chrome (otherwise step 4 will clear the notification, which we should also do as part of bug 1881812)
4. Clear site data for tests.peter.sh
5. Now click the notification
6. See that Chrome does nothing

I don't think Safari ever opens origin page, that means nobody does anything for disconnected SW-triggered notification. That probably means we can safely follow what others do.

Comment 8

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

await registration.unregister();
// -> Now there's a notification but no service worker to respond
// NOTE: but we should probably also clear notifications at this point?

I do think we should be clearing at this point, yes. Especially since we use the same unregister flow for user-initiated privacy-related clearings as well.