Bug 238068 – Remove push subscriptions when permissions are reset

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

[Ben Nham]

Remove push subscriptions when permissions are reset

[Ben Nham]

Created attachment 455073 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/90475499>

[Ben Nham]

Created attachment 455136 [details]
rebase

[Ben Nham]

Created attachment 455586 [details]
rebase

[youenn fablet]

Comment on attachment 455586 [details]
rebase

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

> Source/WebCore/ChangeLog:8
> +        Avoid firing the push event if it has no chance of actually displaying a user notification.

It seems to be late to do this after having started the service worker.
Can we do that check directly in UIProcess?

[Ben Nham]

I think I want to get the message to at least NetworkProcess, because in the event that we lack permissions to display the notification, then I probably want to either trigger the silent push logic or remove the subscription entirely. Both require communicating with webpushd, which requires going through NetworkProcess.

But yes, I think it's a good idea to avoid having NetworkProcess spawn a WebProcess that just early-exits with the permissions error. Let me look into moving the logic up one level to NetworkProcess.

[Ben Nham]

Created attachment 455740 [details]
Patch

[Ben Nham]

Created attachment 455765 [details]
Patch

[youenn fablet]

Comment on attachment 455765 [details]
Patch

Overall, looks good to me.
Some suggestions below and an interrogation about whether we need to special case denied vs. prompt.

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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2284
> +    //    topics list in webpushd to prevent unnecessary pushes from being received at all.

I am not sure why we should be more tolerant with denied than with prompt.
If user sets it to deny, I would expect it to stay like this for a while if not forever, in which case deleting the subscription seems good, no?

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2292
> +        if (permissionState == PushPermissionState::Prompt) {

Can we do this check in NetworkProcessProxy.
Then if denied, we bail out early and directly call callback(false).
If prompt, we could either reuse an existing IPC message if we have the right granularity with NetworkProcessProxy::deleteWebsiteDataForOrigins or create a new message (clearPushSubscriptionsForOrigin maybe?).

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1775
> +    auto permissions = WebNotificationManagerProxy::sharedServiceWorkerManager().notificationPermissions();

It is not great we have to create an entirely new map just to query one value.
Could we introduce a permission getter, something like WebNotificationManagerProxy::sharedServiceWorkerManager().permissionForOrigin(origin)?
That will also allow to centralise the default value there.

> Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.cpp:310
> +        removePushSubscriptionsForOrigins(securityOrigins);

We are creating securityOrigins only for one of the code path. Should we contemplate doing only if needed?

> Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.cpp:311
> +        WebProcessPool::sendToAllRemoteWorkerProcesses(Messages::WebNotificationManager::DidRemoveNotificationDecisions(originStrings));

From a style prospective, we tend to return early in that case.

[Ben Nham]

(In reply to youenn fablet from comment #9)
> Comment on attachment 455765 [details]
> Patch
> 
> Overall, looks good to me.
> Some suggestions below and an interrogation about whether we need to special
> case denied vs. prompt.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455765&action=review
> 
> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2284
> > +    //    topics list in webpushd to prevent unnecessary pushes from being received at all.
> 
> I am not sure why we should be more tolerant with denied than with prompt.
> If user sets it to deny, I would expect it to stay like this for a while if
> not forever, in which case deleting the subscription seems good, no?

Right, so another option is that if you set to deny, then eventually the subscription gets deleted (e.g. via the incrementSilentPushCount logic).

I wrote it this way because in the ideal case, changing the permission state to Denied would not increment silent push count or delete the push sub. Instead it would just move the associated push topic to the ignore list, which essentially puts the subscription in a paused state which causes the server to not deliver us any pushes for that topic. We could then remove that topic from the ignore list when the user toggles the preference back to on.

However, the state synchronization between the browser process and System Preferences for the allow/deny state change is pretty wonky right now and I don't feel super comfortable about committing that ideal change.

So maybe the right thing to do for now is to have the Denied case increment the silent push count. Let me see what Brady thinks as well.

> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2292
> > +        if (permissionState == PushPermissionState::Prompt) {
> 
> Can we do this check in NetworkProcessProxy.
> Then if denied, we bail out early and directly call callback(false).
> If prompt, we could either reuse an existing IPC message if we have the
> right granularity with NetworkProcessProxy::deleteWebsiteDataForOrigins or
> create a new message (clearPushSubscriptionsForOrigin maybe?).

I will look into this depending on whether we decide to make the Denied case increment the silent push count or not.

> > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1775
> > +    auto permissions = WebNotificationManagerProxy::sharedServiceWorkerManager().notificationPermissions();
> 
> It is not great we have to create an entirely new map just to query one
> value.
> Could we introduce a permission getter, something like
> WebNotificationManagerProxy::sharedServiceWorkerManager().
> permissionForOrigin(origin)?
> That will also allow to centralise the default value there.

That's a good idea. I might want to do it in a different patch since it requires an API change and I'm not sure how involved that is yet.

[Ben Nham]

After thinking it through a bit more, I think we can get the Denied case to do the right thing, although it is a bit more of an involved patch involving some state sync between the browser process and webpushd. I think it's best for me to do that in a separate patch since it will be semi-involved. I will change the ChangeLog comment though to explain the new thinking there.

In terms of how we want to handle the Denied case in processPushMessage, I think I will keep the handling of that branch in NetworkProcess rather than NetworkProcessProxy since what we'll eventually do in that branch is send a message to webpushd to move the associated push topic to the ignore list. I'll add a FIXME to that branch in this patch.

I think we should also update the notification manager to allow for more granular querying of permission state, although again I'll probably do that in a separate patch.

[Ben Nham]

Created attachment 455778 [details]
patch feedback

[youenn fablet]

Comment on attachment 455778 [details]
patch feedback

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

> Source/WebKit/ChangeLog:20
> +        back on. We want the subscription object to stay the same after the preference is toggled

I am not 100% sure about this policy but I can live with it.
As a user, I sometimes change back from Allow to Prompt some permissions.
I would not expect Prompt to remove the subscriptions while Denied would keep them.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1779
> +        permission = it->value ? PushPermissionState::Granted : PushPermissionState::Denied;

In case permission is denied, I would exit early, it does not seem worth to start the network process proxy for no good reason.

> Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.cpp:278
> +        if (!networkProcess && dataStore.isPersistent()) {

It is a bit weird to do this only for the first persistent session, can you explain?
I would be tempted to do this for all session IDs.

> Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.cpp:288
> +        networkProcess->deletePushAndNotificationRegistration(sessionID, origin, [originString = origin.toString()](auto&&) { });

Should we add a deletePushAndNotificationRegistration taking a vector of origins to remove the multiple IPC dance?
Also, why do we need originString in the lambda?

> Source/WebKit/webpushd/WebPushDaemon.mm:502
> +    connection->enqueueAppBundleRequest(makeUnique<AppBundleDeletionRequest>(*connection, originString, [this, originString = String { originString }, replySender = WTFMove(replySender), bundleIdentifier = connection->hostAppCodeSigningIdentifier()](const String& result) mutable {

s/const String&/auto/

[Ben Nham]

(In reply to youenn fablet from comment #13)
> Comment on attachment 455778 [details]
> patch feedback
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455778&action=review
> 
> > Source/WebKit/ChangeLog:20
> > +        back on. We want the subscription object to stay the same after the preference is toggled
> 
> I am not 100% sure about this policy but I can live with it.
> As a user, I sometimes change back from Allow to Prompt some permissions.
> I would not expect Prompt to remove the subscriptions while Denied would
> keep them.

I agree it's a bit confusing. We did discuss this at the very beginning in the xfunc and we decided to this way because it mirrors what we do when a user toggles notifications on/off for native apps. But let me bring it up again in the next xfunc to make sure that everyone is on board with this behavior.

> > Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.cpp:278
> > +        if (!networkProcess && dataStore.isPersistent()) {
> 
> It is a bit weird to do this only for the first persistent session, can you
> explain?
> I would be tempted to do this for all session IDs.

We're basically just using NetworkProcess as a proxy to webpushd. Any persistent NetworkProcess session for a given UIProcess looks the same to webpushd, so we just arbitrarily choose the first one. In the case that there is no persistent session when this logic runs (which doesn't happen with our current browser implementation, but could happen in theory), we can fall back on the failsafe logic elsewhere in this patch that removes a subscription at push reception time if the notification policy doesn't exist.

> > Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.cpp:288
> > +        networkProcess->deletePushAndNotificationRegistration(sessionID, origin, [originString = origin.toString()](auto&&) { });
> 
> Should we add a deletePushAndNotificationRegistration taking a vector of
> origins to remove the multiple IPC dance?
> Also, why do we need originString in the lambda?

I originally wrote another IPC that took a vector of origins. But it seems like our browser only removes one origin at a time in practice, so I decided to remove that and reuse the existing deletePushAndNotificationRegistration IPC.

[Ben Nham]

Created attachment 456096 [details]
Patch for landing

[EWS]

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

[Ben Nham]

Created attachment 456154 [details]
Patch for landing

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 456154 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Ben Nham]

Created attachment 456192 [details]
Patch for landing

[EWS]

Committed r292158 (249065@main): <https://commits.webkit.org/249065@main>

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