Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
saschanaz opened this issue Apr 17, 2024 · 5 comments

Comments

@saschanaz
Copy link
Member

saschanaz commented Apr 17, 2024

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
Copy link
Member

annevk commented Apr 17, 2024

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
Copy link
Member Author

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
Copy link
Member

annevk commented Apr 18, 2024

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

@saschanaz
Copy link
Member Author

Should we add a note?

@annevk
Copy link
Member

annevk commented Apr 18, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants