Bug 236543 – Track silent push counts

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: New Bugs

[Ben Nham]

Track silent push counts

[Ben Nham]

Created attachment 451797 [details]
Patch

[Ben Nham]

Created attachment 451835 [details]
Patch

[Ben Nham]

Created attachment 451857 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/88886050>

[youenn fablet]

Comment on attachment 451857 [details]
Patch

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

> Source/WebCore/ChangeLog:19
> +        e.g. making push subscriptions associated with this oirgin non-waking or removing the push

s/oirgin/origin.
It seems like we should add some callback API to inform the application each time the counter got increased.
That would be useful for API tests as well.

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:424
> +    Vector<SecurityOriginData> removedPushMetadata;

Maybe we could simplify and consider that an updatedPushMetadata with a counter of 0 is a removed push metadata?

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:761
> +        callOnMainThread([protectedThis = Ref { *this }, data = data.isolatedCopy()]() mutable {

WTFMove(data).isolatedCopy().

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:762
> +            protectedThis->addPushMetadataToStore(WTFMove(data));

Maybe we could build the vector/map, and do just one call to add the metadata to store.

> Source/WebCore/workers/service/server/RegistrationStore.cpp:127
> +    ASSERT(!data.origin.protocol.isEmpty() && !data.origin.host.isEmpty());

Preexisting pattern, but I wonder whether this is needed here.
Ditto for removePushMetadata

> Source/WebCore/workers/service/server/RegistrationStore.cpp:142
> +    m_updatedPushMetadata.set(origin, std::nullopt);

Why using std::nullopt, and not a counter of 0?

> Source/WebCore/workers/service/server/SWServer.cpp:193
> +void SWServer::addPushMetadataFromStore(PushMetadataData &&data)

&& data.

> Source/WebCore/workers/service/server/SWServer.cpp:195
> +    m_originToPushMetadata.set(data.origin, WTFMove(data));

ASSERT(!m_originToPushMetadata.contains(data.origin));
m_originToPushMetadata.add(...);
But we could also just have just one big init by setting m_originToPushMetadata directly.

> Source/WebCore/workers/service/server/SWServer.cpp:598
> +void SWServer::flushPendingRegistrationsForTesting(CompletionHandler<void()>&& handler)

Could tests reuse storeRegistrationsOnDisk instead?

> Source/WebCore/workers/service/server/SWServer.cpp:1264
> +    m_originToPushMetadata.set(origin, WTFMove(data));

We can probably simplify with something like:
m_originToPushMetadata.add(origin, { origin, 0 }).iterator->value++;
m_registrationStore->updatePushMetadata(data);

> Source/WebCore/workers/service/server/SWServer.cpp:1289
> +        if (auto it = m_originToPushMetadata.find(origin); it != m_originToPushMetadata.end()) {

s/it/iterator.
Also, what is the difference between std::nullopt and 0?
Maybe we can simply do:
completionHandler(m_originToPushMetadata.get(origin).silentPushCount).

> Source/WebCore/workers/service/server/SWServer.cpp:1338
> +                    weakThis->incrementSilentPushCount(origin);

This is probably wrong in case of third party service worker. Do we have tests covering this area for third-party service workers?

> Source/WebCore/workers/service/server/SWServer.h:309
> +    HashMap<SecurityOriginData, PushMetadataData> m_originToPushMetadata;

Should we simplify to HashMap<SecurityOriginData, unsigned>?

> Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp:198
> +        server().resetSilentPushCount(registration->key().topOrigin());

I am not sure we want this.
There could be no controlling registration for the given URL even though there might be a related service worker.
For instance, a web site might have a unique push service worker but this service worker is only used for push and not for controlling any page. 
Why not simply calling server().resetSileintPushCount for any top level navigation load?

[Ben Nham]

(In reply to youenn fablet from comment #5)
> Comment on attachment 451857 [details]
> Patch
> 
> > Source/WebCore/workers/service/server/RegistrationDatabase.cpp:424
> > +    Vector<SecurityOriginData> removedPushMetadata;
> 
> Maybe we could simplify and consider that an updatedPushMetadata with a
> counter of 0 is a removed push metadata?

The reason for this is because I am thinking we may eventually need to store other stuff in this PushMetadata object. So the value of the updatedPushMetadata isn't an integer but rather the whole PushMetadata object. Perhaps a premature optimization.

> WTFMove(data).isolatedCopy().
> 
> > Source/WebCore/workers/service/server/RegistrationDatabase.cpp:762
> > +            protectedThis->addPushMetadataToStore(WTFMove(data));
> 
> Maybe we could build the vector/map, and do just one call to add the
> metadata to store.

I originally did that, but then decided to follow the existing pattern. I can change it to just build up one vector and hop to the main thread just once.

I think would have to be passed in a vector since I don't think HashMap implements a cross-thread copier.

> > Source/WebCore/workers/service/server/RegistrationStore.cpp:142
> > +    m_updatedPushMetadata.set(origin, std::nullopt);
> 
> Why using std::nullopt, and not a counter of 0?

Same reason as above, it's so that we can add more columns to PushMetadata in the future without having to rewrite this.

> > Source/WebCore/workers/service/server/SWServer.cpp:1338
> > +                    weakThis->incrementSilentPushCount(origin);
> 
> This is probably wrong in case of third party service worker. Do we have
> tests covering this area for third-party service workers?

I need to investigate this.

> > Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp:198
> > +        server().resetSilentPushCount(registration->key().topOrigin());
> 
> I am not sure we want this.
> There could be no controlling registration for the given URL even though
> there might be a related service worker.
> For instance, a web site might have a unique push service worker but this
> service worker is only used for push and not for controlling any page. 
> Why not simply calling server().resetSileintPushCount for any top level
> navigation load?

I'm going to remove the call to resetSilentPushCount for now since Brady wants a further discussion on when or if we should be doing that.

[Chris Dumez]

HashMap appears to have a cross thread copier:
https://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/CrossThreadCopier.h#L160

[Ben Nham]

Went with another approach. Duping to the newer patch.

*** This bug has been marked as a duplicate of bug 236863 ***