Bug 236863 – Enforce silent push quota

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

[Ben Nham]

Enforce silent push quota

[Ben Nham]

Created attachment 452588 [details]
Patch

[Ben Nham]

Created attachment 452592 [details]
Patch

[Ben Nham]

*** Bug 236543 has been marked as a duplicate of this bug. ***

[Ben Nham]

Created attachment 452602 [details]
Patch

[Chris Dumez]

Comment on attachment 452592 [details]
Patch

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

> Source/WebCore/workers/service/ServiceWorkerGlobalScope.h:92
> +    bool hasPendingSilentPushEvent() { return m_hasPendingSilentPushEvent; }

nit: should be const.

> Source/WebCore/workers/service/ServiceWorkerGlobalScope.h:117
> +    bool m_hasPendingSilentPushEvent;

Missing initialization?

> Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:227
> +        serviceWorkerGlobalScope->setHasPendingSilentPushEvent(true);

I don't fully understand the logic here. What if there is more than 1 push event being dispatched one after the other. Won't that boolean already be true?
Also, won't the first event to be dispatched reset this boolean to false even though there is still another pending event?

> Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:245
> +                RELEASE_LOG(ServiceWorker, "ServiceWorkerThread::queueTaskToFirePushEvent failed to process push event (rejectedPromise = %d, showedNotification = %d)", hasRejectedAnyPromise, showedNotification);

RELEASE_LOG_ERROR()

> Source/WebKit/Shared/WebPushDaemonConstants.h:31
> +static constexpr int maxSilentPushCount = 3;

Probably don't need the static.

> Source/WebKit/webpushd/PushService.mm:491
> +                        RELEASE_LOG(Push, "IncrementSilentPushRequest couldn't remove subscription for topic %{sensitive}s: %{public}s code: %lld)", topic.utf8().data(), error.domain.UTF8String ?: "none", static_cast<int64_t>(error.code));

RELEASE_LOG_ERROR ?

> Tools/TestWebKitAPI/TestNotificationProvider.h:43
> +    WKDictionaryRef notificationPermissions();

Can this be const?

[Ben Nham]

Created attachment 452616 [details]
Patch

[Ben Nham]

(In reply to Chris Dumez from comment #5)
> Comment on attachment 452592 [details]
> Patch
> > Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:227
> > +        serviceWorkerGlobalScope->setHasPendingSilentPushEvent(true);
> 
> I don't fully understand the logic here. What if there is more than 1 push
> event being dispatched one after the other. Won't that boolean already be
> true?
> Also, won't the first event to be dispatched reset this boolean to false
> even though there is still another pending event?

I mentioned this in the ChangeLog. We are only dispatching one push event at a time, and we don't dispatch the next one until the current one finishes.

I actually brought up this concern to Brady earlier (who suggested this approach) and he said not to worry about it due to that. But obviously we could change it to track actual in-flight events instead of a boolean flag.

[Ben Nham]

Created attachment 452619 [details]
Patch feedback

[Ben Nham]

Created attachment 452750 [details]
EWS

[Ben Nham]

<rdar://87409042>

[youenn fablet]

Comment on attachment 452750 [details]
EWS

LGTM overall, some nits below.

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

> Source/WebKit/ChangeLog:9
> +           then we not increment the silent push count associated with that origin.

s/not//

> Source/WebCore/Modules/push-api/PushDatabase.cpp:524
> +        WorkQueue::main().dispatch([completionHandler = WTFMove(completionHandler), topics = crossThreadCopy(WTFMove(topics))]() mutable {

completeOnMainQueue?

> Source/WebCore/Modules/push-api/PushDatabase.cpp:530
> +void PushDatabase::incrementSilentPushCount(const String& bundleID, const String& securityOrigin, CompletionHandler<void(int)>&& completionHandler)

should probably an unsigned, not an int.

> Source/WebCore/Modules/push-api/PushDatabase.cpp:557
> +                completeOnMainQueue(WTFMove(completionHandler), 0);

We could use scope Exit here, see below.

> Source/WebCore/Modules/push-api/PushDatabase.cpp:622
> +                completeOnMainQueue(WTFMove(completionHandler), Vector<PushRecord> { });

You might want to use scopeExit to return early without calling completeOnMainQueue explicitly.
Something like:
auto scope = makeScopeExit([this, &completionHandler] { completeOnMainQueue(WTFMove(completionHandler), Vector<PushRecord> { }); })
Then, use release() before calling
completeOnMainQueue(WTFMove(completionHandler), WTFMove(removedRecords));

> Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:232
> +        pushEvent->whenAllExtendLifetimePromisesAreSettled([weakThis = WTFMove(weakThis), serviceWorkerGlobalScope, eventCreationTime = pushEvent->timeStamp(), callback = WTFMove(callback)](auto&& extendLifetimePromises) mutable {

We are not using weakThis here apparently, can we remove it?

> Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:241
> +            bool showedNotification = !serviceWorkerGlobalScope->hasPendingSilentPushEvent();

This seems correct to me to check hasPendingSilentPushEvent when the event wait promises are settled.
The spec is not super clear about userVisibleOnly being treated this way. Are other browsers detecting this in this manner?

> Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:245
> +                RELEASE_LOG_ERROR(ServiceWorker, "ServiceWorkerThread::queueTaskToFirePushEvent failed to process push event (rejectedPromise = %d, showedNotification = %d)", hasRejectedAnyPromise, showedNotification);

RELEASE_LOG_ERROR_IF

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2272
> +            if (!result && (session = networkSession(sessionID))) {

We are incrementing the counter in case of web process crash. Crash should be fairly limited hopefully but this might trigger some potential overprompting.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2274
> +                    .incrementSilentPushCount(WTFMove(origin), [callback = WTFMove(callback), result](int) mutable {

Why doing this incrementation?

> Source/WebKit/webpushd/PushService.mm:489
> +                m_connection->unsubscribe(topic, record.serverVAPIDPublicKey, [topic](bool unsubscribed, NSError* error) {

unsubscribe should notify the service worker so that the service worker knows it has been unregistered through pushsubscriptionchange.
Do we have test coverage for this?
Are we sure current websites handle this unregistration code path correctly? If not, we could unregister the whole service worker registration to make sure things would restart with a clean state.

> Source/WebKit/webpushd/PushService.mm:491
> +                        RELEASE_LOG_ERROR(Push, "IncrementSilentPushRequest couldn't remove subscription for topic %{sensitive}s: %{public}s code: %lld)", topic.utf8().data(), error.domain.UTF8String ?: "none", static_cast<int64_t>(error.code));

RELEASE_LOG_ERROR_IF(Push, !unsubscribed, ...).

> Source/WebKit/webpushd/WebPushDaemon.h:36
> +#include <WebCore/SecurityOriginData.h>

We could forward declare SecurityOriginData

[Ben Nham]

Comment on attachment 452750 [details]
EWS

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

>> Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:241
>> +            bool showedNotification = !serviceWorkerGlobalScope->hasPendingSilentPushEvent();
> 
> This seems correct to me to check hasPendingSilentPushEvent when the event wait promises are settled.
> The spec is not super clear about userVisibleOnly being treated this way. Are other browsers detecting this in this manner?

Exactly how to treat userVisibleOnly isn't mentioned in the spec. In practice, both Firefox and Chrome apply some penalty if you send too many silent pushes. This patch implements a quota system which is pretty similar to what Firefox implements (they also unsubscribe once you hit your quota).

I think the two main differences we have from Firefox's implementation from my quick look are that:

1. They check that a notification is shown some fixed interval after the push event is fired (e.g. N seconds afterwards), whereas we are checking when the push event's promises settle. I think checking when the promises settle seems reasonable so I chose that.
2. Their quota is higher than ours.

Chrome's penalty is a bit different, as they make silent pushes consume some sort of budget based on how you've handled the last N messages. If too many push event handlers fail to show a notification, then they automatically show a notification along the lines of "$DOMAIN is doing work in the background". There was significant pushback against showing a notification with text like that so I didn't implement that.

There is some more discussion here: https://github.com/w3c/push-api/issues/313.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2274
>> +                    .incrementSilentPushCount(WTFMove(origin), [callback = WTFMove(callback), result](int) mutable {
> 
> Why doing this incrementation?

This is where we're incrementing the silent push count if the push event handler failed to fulfill all promises or show a notification.

I guess if WebContent crashes during pushEvent handling become a big issue I can add more code in the future to deal with that scenario.

>> Source/WebKit/webpushd/PushService.mm:489
>> +                m_connection->unsubscribe(topic, record.serverVAPIDPublicKey, [topic](bool unsubscribed, NSError* error) {
> 
> unsubscribe should notify the service worker so that the service worker knows it has been unregistered through pushsubscriptionchange.
> Do we have test coverage for this?
> Are we sure current websites handle this unregistration code path correctly? If not, we could unregister the whole service worker registration to make sure things would restart with a clean state.

For this initial version of the feature I am thinking of not emitting the pushsubscriptionchange event unless people strongly want it:

1. Only Firefox emits this event right now (even though it's been mentioned in the spec for years), and it's actually not even the current version of the event that has the oldSubscription and newSubscription objects. So this event handler is unlikely to be widely deployed on the current web.

2. We would probably not want to emit the pushsubscriptionchange event instantly, since the most likely response would be for the pushsubsriptionchange event handler to simply re-subscribe to push, which would make this whole heuristic pretty pointless. Instead, we'd probably to do something like persisting some bit in the service worker RegistrationDatabase that says that the next user-facing visit to this origin would result in a pushsubscriptionchange event firing on the service worker. As far as I could tell from experimenting, this is what Firefox does when you run out of quota and get unsubscribed.

(2) makes the event more complicated to implement (although obviously very tractable), and (1) makes it so that work is likely not very useful on the real web, so I didn't implement it.

[Ben Nham]

Created attachment 453328 [details]
Patch

[youenn fablet]

> >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2274
> >> +                    .incrementSilentPushCount(WTFMove(origin), [callback = WTFMove(callback), result](int) mutable {
> > 
> > Why doing this incrementation?

Sorry, I was meaning indentation.

[Brady Eidson]

Comment on attachment 453328 [details]
Patch

I'm fine with all of this.

[Ben Nham]

Created attachment 453807 [details]
Patch for landing

[EWS]

Committed r290815 (248051@main): <https://commits.webkit.org/248051@main>

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