Remove push subscriptions when permissions are reset
Created attachment 455073 [details] Patch
<rdar://problem/90475499>
Created attachment 455136 [details] rebase
Created attachment 455586 [details] rebase
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?
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.
Created attachment 455740 [details] Patch
Created attachment 455765 [details] Patch
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.
(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.
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.
Created attachment 455778 [details] patch feedback
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/
(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.
Created attachment 456096 [details] Patch for landing
ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.
Created attachment 456154 [details] Patch for landing
Tools/Scripts/svn-apply failed to apply attachment 456154 [details] to trunk. Please resolve the conflicts and upload a new patch.
Created attachment 456192 [details] Patch for landing
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].