search

whatwg / notifications

Notifications API Standard
https://notifications.spec.whatwg.org/
Other
139 stars 49 forks source link

Permission check happens first on both Gecko and Blink for showNotification #207

Open saschanaz opened 7 months ago

saschanaz commented 7 months ago

What is the issue with the Notifications API Standard?

The current spec in https://notifications.spec.whatwg.org/#dom-serviceworkerregistration-shownotification creates a notification in step 5 and do the permission check in step 6 where checks for options bag happen. But in reality:

  1. Gecko checks permission first: https://searchfox.org/mozilla-central/rev/b94e479d0b79b157029379832d05229df646e134/dom/notification/Notification.cpp#2210-2234
  2. Blink checks permission first too: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/notifications/service_worker_registration_notifications.cc;l=61-74;drc=08bac9d4c75f023df86a07be1986884f760dab94
  3. WebKit too: https://searchfox.org/wubkat/rev/25be8d6371093c7bc40042867d31fcb62659346c/Source/WebCore/workers/service/ServiceWorkerRegistration.cpp#300-311 (although there's no renotify/vibrate support there to actually make a difference)

This does not change any visible exception order as both throw TypeError, but maybe worth changing the spec to reflect reality?

annevk commented 7 months ago

Are you talking about https://notifications.spec.whatwg.org/#dom-serviceworkerregistration-shownotification?

I think the difference is somewhat observable as the permission check TypeError comes from a task. And I think that's somewhat important as we wouldn't want to require synchronous IPC for a permission check.

saschanaz commented 7 months ago

Hmm, it seems both Blink and WebKit indeed tries to synchronously ping something outside of the current thread, although I don't think Gecko does that (as we propagate permissions beforehand). I'm not sure anyone would be actually interested to fix that, but maybe the spec shouldn't force the sync behavior either.

annevk commented 7 months ago

Yeah, I don't think we should force synchronizing permissions whenever we can avoid it.

saschanaz commented 7 months ago

Should we add a note?

annevk commented 7 months ago

Maybe, but we're not terribly consistent about this at the moment. E.g., Notification.permission does require active synchronization.

There might be some ways to get rid of that (at some point there was a proposal to always return "default" there), but thus far nobody has.