Bug 231064

Summary: Add push registration stubs
Product: WebKit Reporter: Ben Nham <nham>
Component: New BugsAssignee: Ben Nham <nham>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, annulen, beidson, benjamin, calvaris, cdumez, dvpdiner2, esprehn+autocc, ews-watchlist, gyuyoung.kim, kondapallykalyan, nham, ryuan.choi, sergio, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Ben Nham 2021-09-30 22:31:22 PDT
Add push registration stubs
Comment 1 Ben Nham 2021-09-30 22:42:58 PDT
Created attachment 439823 [details]
Patch
Comment 2 youenn fablet 2021-09-30 23:32:02 PDT
Comment on attachment 439823 [details]
Patch

A couple of nits below.
I like your idea to use a specific folder in modules, I'll update my patch accordingly.
I think I prefer Source/WebCore/Modules/push-api to better match the name of the spec and name of the related WPT test folder.

View in context: https://bugs.webkit.org/attachment.cgi?id=439823&action=review

> Source/WebCore/ChangeLog:8
> +        Description to follow.

We should be able to rebase some WPT push-api tests within this patch

> Source/WTF/wtf/PlatformEnableCocoa.h:473
> +#if !defined(ENABLE_PUSH_API) && (PLATFORM(MAC) || PLATFORM(IOS))

I would hope SERVICE_WORKER to be sufficient here.

> Source/WebCore/Modules/push/PushPermissionState.h:32
> +enum class PushPermissionState {

It is a bit sad that there is a push specific enum for that, but the spec is like that so seems ok.

> Source/WebCore/Modules/push/PushSubscription.cpp:39
> +String PushSubscription::endpoint() const

const String&

> Source/WebCore/Modules/push/PushSubscription.h:46
> +    WTF_MAKE_ISO_ALLOCATED(PushSubscription);

It seems we might want a static create method to create those objects and a private constructor.
If we cannot exercise them right now, we could add an Internals API to create one of these objects to get some early testing.

> Source/WebCore/Modules/push/PushSubscriptionOptions.h:40
> +    WTF_MAKE_ISO_ALLOCATED(PushSubscriptionOptions);

Probably want a create/private constructor as well here.
Comment 3 Ben Nham 2021-10-04 11:06:40 PDT
Created attachment 440078 [details]
Patch
Comment 4 youenn fablet 2021-10-04 11:23:08 PDT
Comment on attachment 440078 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440078&action=review

> Source/WebCore/Modules/push-api/PushManager.cpp:55
> +    return PushService::singleton().supportedContentEncodings();

Is that thread safe?

> Source/WebCore/Modules/push-api/PushManager.cpp:60
> +    PushService::singleton().subscribe(m_serviceWorkerRegistrationKey, WTFMove(options), [promise = WTFMove(promise)](auto result) mutable {

Ditto, it seems we should have a push service connection specific for workers and for main threads, especially if we capture objects like promises.
I would tend to reuse ServiceWorkerRegistration infrastructure.

> Source/WebCore/Modules/push-api/PushManager.h:53
> +    PushManager(const ServiceWorkerRegistrationKey&);

explicit.
Maybe the PushManager should have a WeakPtr<ServiceWorkerRegistration> or a Ref to its ServiceWorkerContainer, or a Ref to a SWClientConnection, that will do the worker/main thread handling.
Comment 5 Radar WebKit Bug Importer 2021-10-07 22:32:17 PDT
<rdar://problem/84014097>
Comment 6 Ben Nham 2021-10-08 23:28:04 PDT
Created attachment 440700 [details]
Patch
Comment 7 Ben Nham 2021-10-09 16:33:57 PDT
Created attachment 440722 [details]
Patch
Comment 8 Ben Nham 2021-10-09 23:00:46 PDT
Created attachment 440724 [details]
Patch
Comment 9 youenn fablet 2021-10-10 06:10:38 PDT
Comment on attachment 440724 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440724&action=review

> Source/WebCore/Modules/push-api/PushManager.cpp:55
> +    static const auto encodings = makeNeverDestroyed(Vector<String> { "aesgcm"_s, "aes128gcm"_s });

I don't think this is threadsafe, we could ref and deref these strings from various threads.

> Source/WebCore/Modules/push-api/PushManager.cpp:61
> +    if (!scriptExecutionContext.isSecureContext()) {

I am not sure this is needed, PushManager is probably not exposed in non secret context.
An ASSERT might be fine.

> Source/WebCore/Modules/push-api/PushManager.cpp:66
> +    if (!options || !options->userVisibleOnly) {

This makes sense to me but is not as per standard. Is it compatible with existing sites, given default is (sadly) false?

> Source/WebCore/Modules/push-api/PushManager.cpp:77
> +    KeyDataResult keyDataResult = WTF::switchOn(*options->applicationServerKey, [](RefPtr<JSC::ArrayBuffer>& value) -> KeyDataResult {

auto

> Source/WebCore/Modules/push-api/PushManager.cpp:109
> +    if (!m_serviceWorkerRegistration || !m_serviceWorkerRegistration->active()) {

I would think m_serciceWorkerRegistration should always be available if PushManager.
For instance, even if the serviceWorkerRegistration gets garbage collected, push manager should still be able to work fine.

One way to avoid a ref cycle would be the following:
ServiceWorkerRegistration owns a unique_ptr<PushManager>, lazily constructed.
PushManager has a ServiceWorkerRegistration&.
PushManager implements ref/deref methods by calling its ServiceWorkerRegistration ref/deref methods.

> Source/WebCore/Modules/push-api/ServiceWorkerRegistrationPushAPI.cpp:49
> +RefPtr<PushManager> ServiceWorkerRegistrationPushAPI::pushManager()

I guess it should either be a PushManager& or a Ref<PushManager>, since attribute value is not nullable.

> LayoutTests/ChangeLog:12
> +        * http/wpt/push-api/pushManager-serviceworker.js: Added.

You might be able to simplify a bit the tests by doing the following:
- Rename pushManager-test.js in pushManager.any.js (plus remove testSubscribe(), directly call test/promise_test)
- Add an empty file named http/wpt/push-api/pushManager.any.serviceworker.html
- Remove http/wpt/push-api/pushManager-serviceworker.https.htm and http/wpt/push-api/pushManager-serviceworker.js
WPT server will provide templated test files that will make use of your test.
You can see pushEvent.any.worker.js as an example.

> LayoutTests/http/wpt/push-api/pushManager-test.js:38
> +    promise_test(async (test) => {

It would be nice if we could run these tests in window environment as well.
Comment 10 Ben Nham 2021-10-10 16:40:30 PDT
Created attachment 440742 [details]
Patch
Comment 11 Ben Nham 2021-10-10 16:41:49 PDT
The latest patch should address your comments.

Although the standard doesn't require userVisibleOnly to be true, Chrome requires that (it throws otherwise) and we plan to do so as well.
Comment 12 youenn fablet 2021-10-11 00:18:23 PDT
Comment on attachment 440742 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440742&action=review

> Source/WebCore/Modules/push-api/PushManager.cpp:78
> +        promise.reject(Exception { NotSupportedError, "Subscribing for push requires userVisibleOnly to be true"_s });

Chrome and Firefox seem to use NotAllowedError, can we align?
Comment 13 youenn fablet 2021-10-11 00:27:32 PDT
(In reply to Ben Nham from comment #11)
> The latest patch should address your comments.
> 
> Although the standard doesn't require userVisibleOnly to be true, Chrome
> requires that (it throws otherwise) and we plan to do so as well.

Sounds good (although I wonder why we even have the userVisibleOnly=false option).
I filed https://github.com/w3c/push-api/issues/339 as I think it would be nice to mention this in the spec (and where/how to do this check).
Comment 14 youenn fablet 2021-10-11 00:34:46 PDT
Comment on attachment 440742 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440742&action=review

> LayoutTests/http/wpt/push-api/pushManager.any.js:37
> +        registration = await navigator.serviceWorker.register("pushManager-worker.js");

It is probably worth unregistering in window environment as the last test as some sort of clean-up.
Comment 15 Ben Nham 2021-10-11 09:55:36 PDT
Created attachment 440801 [details]
Patch
Comment 16 EWS 2021-10-11 11:38:34 PDT
Committed r283916 (242785@main): <https://commits.webkit.org/242785@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440801 [details].