Bug 233088 – Add support for onpushsubscriptionchange event handler

• Component: New Bugs
• Status: RESOLVED FIXED

[Ben Nham]

Add support for onpushsubscriptionchange event handler

[Ben Nham]

Created attachment 444145 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/85378345>

[Ben Nham]

Created attachment 444146 [details]
Patch

[Ben Nham]

Comment on attachment 444146 [details]
Patch

I can't repro the timeout in the ios-wk2 tests locally. Maybe there's something inherently flaky in the way I wrote the test that I'm not seeing yet.

I'd still like to get some feedback, so setting the r? flag.

[youenn fablet]

Comment on attachment 444146 [details]
Patch

Some comments below.
As of the test failures for iOS, it seems all spin tests have this issue on iOS simulator.
This might be worth some investigations.

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

> Source/WebCore/testing/ServiceWorkerInternals.cpp:85
> +void ServiceWorkerInternals::schedulePushSubscriptionChangeEvent(RefPtr<PushSubscription> newSubscription, RefPtr<PushSubscription> oldSubscription)

I don't think this is good to directly use PushSubscription objects in SWContextManager::firePushSubscriptionChangeEvent.
PushSubscription should be created and live in worker thread. For instance it is refcounted, not thread safe refcounted.
And it has a ref to an ActiveDOMObject.

What can be done is to hop to the main thread with the necessary data to reconstruct PushSubscription objects at the time we fire the event.
This will also make SWContextManager::firePushSubscriptionChangeEvent easier to use when network process will send IPC messages.

One model is to reuse what we do for ServiceWorkerRegistration:
- Add a PushSubscriptionData similarly to ServiceWorkerRegistrationData. It should be encodable/decodable through IPC, and also have a cross thready copier.
- Pass PushSubscriptionData for old and new to SWContextManager::firePushSubscriptionChangeEvent, hop to worker thread and create PushSubscriptions from PushSubscriptionData.
- PushSubscription is fired on a service worker so I would guess the old registration has no ServiceWorkerRegistration and the new registration if not null would have the registration of the given worker, so no need to pass any info for the registration.

> Source/WebCore/testing/ServiceWorkerInternals.h:59
> +    void schedulePushSubscriptionChangeEvent(RefPtr<PushSubscription> newSubscription, RefPtr<PushSubscription> oldSubscription);

I would tend to use RefPtr<>&& instead of RefPtr<>

[Ben Nham]

(In reply to youenn fablet from comment #5)
> Comment on attachment 444146 [details]
> Patch
> 
> Some comments below.
> As of the test failures for iOS, it seems all spin tests have this issue on
> iOS simulator.
> This might be worth some investigations.

I've been looking into it, but I can only repro it locally right now if I run all the layout tests, not when I just run the tests in http/wpt/service-workers. This makes it pretty challenging to investigate.

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444146&action=review
> 
> > Source/WebCore/testing/ServiceWorkerInternals.cpp:85
> > +void ServiceWorkerInternals::schedulePushSubscriptionChangeEvent(RefPtr<PushSubscription> newSubscription, RefPtr<PushSubscription> oldSubscription)
> 
> I don't think this is good to directly use PushSubscription objects in
> SWContextManager::firePushSubscriptionChangeEvent.
> PushSubscription should be created and live in worker thread. For instance
> it is refcounted, not thread safe refcounted.
> And it has a ref to an ActiveDOMObject.
> 
> What can be done is to hop to the main thread with the necessary data to
> reconstruct PushSubscription objects at the time we fire the event.
> This will also make SWContextManager::firePushSubscriptionChangeEvent easier
> to use when network process will send IPC messages.
>
> One model is to reuse what we do for ServiceWorkerRegistration:
> - Add a PushSubscriptionData similarly to ServiceWorkerRegistrationData. It
> should be encodable/decodable through IPC, and also have a cross thready
> copier.
> - Pass PushSubscriptionData for old and new to
> SWContextManager::firePushSubscriptionChangeEvent, hop to worker thread and
> create PushSubscriptions from PushSubscriptionData.
> - PushSubscription is fired on a service worker so I would guess the old
> registration has no ServiceWorkerRegistration and the new registration if
> not null would have the registration of the given worker, so no need to pass
> any info for the registration.

I see, that makes sense. I already created a PushSubscriptionData in a previous patch for cross-thread copying so I'll use that instead.

[Ben Nham]

Created attachment 444518 [details]
Patch

[youenn fablet]

Comment on attachment 444518 [details]
Patch

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

> Source/WebCore/Modules/push-api/PushSubscription.h:70
>      String m_endpoint;

Would it make sense to store a PushSubscriptionData in PushSubscription?
Then we could have a data() getter, and constructor would take less parameters.

> Source/WebCore/Modules/push-api/PushSubscriptionData.h:47
> +    WEBCORE_EXPORT static PushSubscriptionData create(const PushSubscription&);

No longer needed if we add a const PushSubscriptionData& data() const (or  PushSubscriptionData data() const) to PushSubscription.

> Source/WebCore/Modules/push-api/PushSubscriptionData.h:55
> +Ref<PushSubscription> createPushSubscriptionFromData(RefPtr<ServiceWorkerRegistration>, PushSubscriptionData&&);

If we pass to the constructor/create a PushSubscriptionData, we no longer need that method.

> Source/WebCore/testing/ServiceWorkerInternals.cpp:86
> +void ServiceWorkerInternals::schedulePushSubscriptionChangeEvent(RefPtr<PushSubscription>&& newSubscription, RefPtr<PushSubscription>&& oldSubscription)

Can we just take PushSubscription* parameters?

> Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:252
> +            oldSubscription = createPushSubscriptionFromData(nullptr, WTFMove(*oldSubscriptionData));

Seems a bit strange to have nullptr here.
I guess we could have a PushSubscription::create(PushSubscriptionData&&, RefPtr<>&& = nulltr);
Also, we should be able to call unsubscribe on the new subscription, which probably requires to pass a ServiceWorkerRegistration.
In that case, we should be able to get it from the ServiceWorkerGlobalScope.

> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:328
> +        oldSubscriptionData = oldSubscriptionData->isolatedCopy();

I am not sure this is needed. If the issue is about std::optional, we can hopefully use crossThreadCopy.
Something like:
thread().runLoop().postTask([this, protectedThis = Ref { *this }, newSubscriptionData = crossThreadCopy(WTFMove(newSubscriptionData)), ...

[Ben Nham]

Created attachment 444534 [details]
Patch

[Ben Nham]

Created attachment 444597 [details]
Patch

[Ben Nham]

Created attachment 444615 [details]
Patch

[Ben Nham]

Comment on attachment 444615 [details]
Patch

I think I addressed all the feedback. One difference is that I kept two PushSubscription constructors rather than using a default parameter. This is mainly because Internals calls the constructor and calling the constructor with a RefPtr<ServiceWorkerRegistration> parameter would require us to export ServiceWorkerRegistration due to the way RefPtr works. It seems cleaner to not export that object purely for testing purposes (although i could do that instead if you prefer).

[youenn fablet]

Comment on attachment 444615 [details]
Patch

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

> Source/WebCore/Modules/push-api/PushSubscription.cpp:50
> +    , m_serviceWorkerRegistration(WTFMove(registration))

Let's have just one constructor, taking a RefPtr<>

> Source/WebCore/Modules/push-api/PushSubscription.h:70
> +    PushSubscription(PushSubscriptionData&&, Ref<ServiceWorkerRegistration>&&);

I am not sure why we want to limit exporting.
If we want that, I would tend to have a dedicated create method:
WEBCORE_EXPORT static Ref<PushSubscription> create(PushSubscriptionData&&) instead of two constructors.

> Source/WebCore/Modules/push-api/PushSubscriptionData.cpp:31
> +#include "ServiceWorkerRegistration.h"

No longer needed?

> Source/WebCore/Modules/push-api/PushSubscriptionData.cpp:-41
> -

No longer needed?

> Source/WebCore/Modules/push-api/PushSubscriptionData.h:32
> +#include <wtf/Forward.h>

No longer needed?

> Source/WebCore/Modules/push-api/PushSubscriptionData.h:39
> +

No longer needed?

> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:327
> +    thread().runLoop().postTask([this, protectedThis = Ref { *this }, newSubscriptionData = crossThreadCopy(newSubscriptionData), oldSubscriptionData = crossThreadCopy(oldSubscriptionData)](auto&) mutable {

We would write it as newSubscriptionData = crossThreadCopy(WTFMove(newSubscriptionData)), in case we want to have an optimised isolatedCopy() && on PushSubscriptionData in the future.

[Ben Nham]

Created attachment 444694 [details]
patch for landing

[Ben Nham]

Created attachment 444714 [details]
rebased patch for landing

[EWS]

Committed r286044 (244431@main): <https://commits.webkit.org/244431@main>

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