Refactor Notification::permission to follow the same code path as Permissions::query #2982

RupinMittal · 2022-08-03T17:32:49Z

`92a17b9`

Refactor Notification::permission to follow the same code path as Permissions::query
https://bugs.webkit.org/show_bug.cgi?id=243502

Reviewed by NOBODY (OOPS!).

There are two functions for obtaining the permission state of the Notifications API.
We need to provide support for both to ensure compatibility with web standards. This
patch ensures that they follow the same code path since having two different paths is
unnecessary and hurts maintainability. This code path goes to the UIProcess and then
to the client to get the permission state.

This patch also removes the now uneeded cache in WebNotificationManager that
Notification::permission was relying on and enables the WebProcess cache in
WebPermissionController to be used.

* LayoutTests/http/tests/notifications/service-worker-notification-permission-expected.txt: Added.
* LayoutTests/http/tests/notifications/service-worker-notification-permission.html: Added.
* LayoutTests/http/tests/notifications/service-worker-notification-permission.js: Added.
(self.onmessage):
* LayoutTests/http/tests/push-api/subscribe-deny-permissions-on-prompt-expected.txt:
* LayoutTests/http/tests/push-api/subscribe-deny-permissions-on-prompt.html:
* LayoutTests/platform/mac-wk1/TestExpectations:
* Source/WebCore/Headers.cmake:
* Source/WebCore/Modules/permissions/PermissionController.h:
* Source/WebCore/Modules/permissions/Permissions.cpp:
(WebCore::Permissions::sourceFromContext):
(WebCore::sourceFromContext): Deleted.
* Source/WebCore/Modules/permissions/Permissions.h:
* Source/WebCore/WebCore.xcodeproj/project.pbxproj:
* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::queryPermission):
(WebKit::WebPageProxy::clearNotificationPermissionState):
* Source/WebKit/UIProcess/WebPermissionControllerProxy.cpp:
(WebKit::WebPermissionControllerProxy::querySync):
* Source/WebKit/UIProcess/WebPermissionControllerProxy.h:
* Source/WebKit/UIProcess/WebPermissionControllerProxy.messages.in:
* Source/WebKit/WebProcess/Notifications/NotificationPermissionRequestManager.cpp:
(WebKit::NotificationPermissionRequestManager::startRequest):
(WebKit::NotificationPermissionRequestManager::permissionLevel): Deleted.
* Source/WebKit/WebProcess/Notifications/NotificationPermissionRequestManager.h:
* Source/WebKit/WebProcess/Notifications/WebNotificationManager.cpp:
(WebKit::WebNotificationManager::initialize):
(WebKit::WebNotificationManager::didUpdateNotificationDecision):
(WebKit::WebNotificationManager::didRemoveNotificationDecisions):
(WebKit::WebNotificationManager::removeAllPermissionsForTesting):
(WebKit::WebNotificationManager::policyForOrigin const): Deleted.
* Source/WebKit/WebProcess/Notifications/WebNotificationManager.h:
* Source/WebKit/WebProcess/WebCoreSupport/WebNotificationClient.cpp:
(WebKit::WebNotificationClient::requestPermission):
(WebKit::WebNotificationClient::checkPermission):
(WebKit::WebNotificationClient::clearNotificationPermissionState): Deleted.
* Source/WebKit/WebProcess/WebCoreSupport/WebNotificationClient.h:
* Source/WebKit/WebProcess/WebCoreSupport/WebPermissionController.cpp:
(WebKit::WebPermissionController::query):
(WebKit::WebPermissionController::querySync):
(WebKit::WebPermissionController::webPageProxyIdentifier):
(WebKit::WebPermissionController::removeEntryFromCache):
(WebKit::WebPermissionController::clearCache):
(WebKit::WebPermissionController::tryProcessingRequests):
* Source/WebKit/WebProcess/WebCoreSupport/WebPermissionController.h:
(isType):
* Source/WebKit/WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::clearNotificationPermissionState): Deleted.
* Source/WebKit/WebProcess/WebPage/WebPage.h:
* Source/WebKit/WebProcess/WebPage/WebPage.messages.in:
* Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:
(WTR::TestRunner::removeAllWebNotificationPermissions):
* Tools/WebKitTestRunner/TestController.cpp:
(WTR::TestController::removeAllWebNotificationPermissions):
* Tools/WebKitTestRunner/TestController.h:
* Tools/WebKitTestRunner/TestInvocation.cpp:
(WTR::TestInvocation::didReceiveSynchronousMessageFromInjectedBundle):

92a17b9

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	❌ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

Source/WebCore/Modules/permissions/PermissionController.h

Source/WebKit/UIProcess/WebPageProxy.messages.in

Source/WebKit/WebProcess/WebCoreSupport/WebNotificationClient.cpp

Source/WebKit/WebProcess/WebCoreSupport/WebPermissionController.h

Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp

Tools/WebKitTestRunner/TestController.cpp

szewai

I think this needs to be be done after adding Worker support for Permissions API, and is better be done after change event is supported (or making web process able to cache the state to avoid sync messages).

Source/WebCore/Modules/permissions/PermissionController.h

Source/WebKit/WebProcess/WebCoreSupport/WebNotificationClient.cpp

Source/WebKit/WebProcess/WebCoreSupport/WebPermissionController.cpp

Source/WebKit/WebProcess/WebCoreSupport/WebNotificationClient.cpp

webkit-early-warning-system · 2022-08-25T02:06:42Z

3c2f221

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	🛠 🧪 win
🧪 bindings	🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	❌ 🧪 api-gtk
	🛠 tv	✅ 🧪 mac-wk1
🧪 services	✅ 🛠 tv-sim	❌ 🧪 mac-wk2
	🛠 watch	❌ 🧪 mac-AS-debug-wk2	🛠 jsc-mips
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress	🧪 jsc-mips-tests

webkit-early-warning-system · 2022-08-25T02:06:43Z

EWS run on previous version of this PR (hash 3c2f221)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	❌ 🛠 ios	❌ 🛠 mac	❌ 🛠 wpe	❌ 🛠 🧪 win
❌ 🧪 bindings	❌ 🛠 ios-sim	✅ 🛠 mac-debug	❌ 🛠 gtk	❌ 🛠 wincairo
❌ 🧪 webkitperl	✅ 🧪 ios-wk2	❌ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	❌ 🧪 api-gtk
	❌ 🛠 tv	✅ 🧪 mac-wk1
	❌ 🛠 tv-sim	❌ 🧪 mac-wk2
	❌ 🛠 watch	❌ 🧪 mac-AS-debug-wk2
	❌ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-08-25T06:10:03Z

7e0bfbe

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
🧪 style	⏳ 🛠 ios	⏳ 🛠 mac	🛠 wpe	🛠 🧪 win
⏳ 🧪 bindings	🛠 ios-sim	⏳ 🛠 mac-debug	⏳ 🛠 gtk	🛠 wincairo
⏳ 🧪 webkitperl	🧪 ios-wk2	⏳ 🛠 mac-AS-debug	⏳ 🧪 gtk-wk2
	🧪 api-ios	⏳ 🧪 api-mac	⏳ 🧪 api-gtk
🛠 🧪 jsc	⏳ 🛠 tv	⏳ 🧪 mac-wk1	⏳ 🛠 jsc-armv7
🧪 services	⏳ 🛠 tv-sim	⏳ 🧪 mac-wk2	⏳ 🧪 jsc-armv7-tests
	⏳ 🛠 watch	⏳ 🧪 mac-AS-debug-wk2	⏳ 🛠 jsc-mips
	⏳ 🛠 watch-sim	⏳ 🧪 mac-wk2-stress	⏳ 🧪 jsc-mips-tests

webkit-early-warning-system · 2022-08-25T06:10:05Z

EWS run on previous version of this PR (hash 7e0bfbe)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe
	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	❌ 🧪 api-gtk
✅ 🛠 🧪 jsc	✅ 🛠 tv	❌ 🧪 mac-wk1	✅ 🛠 jsc-armv7
	✅ 🛠 tv-sim	❌ 🧪 mac-wk2	✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch	❌ 🧪 mac-AS-debug-wk2	✅ 🛠 jsc-mips
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 jsc-mips-tests

szewai

Test failure looks relevant, please fix them

Source/WebKit/WebProcess/WebCoreSupport/WebPermissionController.cpp

Source/WebKit/WebProcess/WebCoreSupport/WebNotificationClient.cpp

…missions::query https://bugs.webkit.org/show_bug.cgi?id=243502 Reviewed by NOBODY (OOPS!). There are two functions for obtaining the permission state of the Notifications API. We need to provide support for both to ensure compatibility with web standards. This patch ensures that they follow the same code path since having two different paths is unnecessary and hurts maintainability. This code path goes to the UIProcess and then to the client to get the permission state. This patch also removes the now uneeded cache in WebNotificationManager that Notification::permission was relying on and enables the WebProcess cache in WebPermissionController to be used. * LayoutTests/http/tests/notifications/service-worker-notification-permission-expected.txt: Added. * LayoutTests/http/tests/notifications/service-worker-notification-permission.html: Added. * LayoutTests/http/tests/notifications/service-worker-notification-permission.js: Added. (self.onmessage): * LayoutTests/http/tests/push-api/subscribe-deny-permissions-on-prompt-expected.txt: * LayoutTests/http/tests/push-api/subscribe-deny-permissions-on-prompt.html: * LayoutTests/platform/mac-wk1/TestExpectations: * Source/WebCore/Headers.cmake: * Source/WebCore/Modules/permissions/PermissionController.h: * Source/WebCore/Modules/permissions/Permissions.cpp: (WebCore::Permissions::sourceFromContext): (WebCore::sourceFromContext): Deleted. * Source/WebCore/Modules/permissions/Permissions.h: * Source/WebCore/WebCore.xcodeproj/project.pbxproj: * Source/WebKit/UIProcess/WebPageProxy.cpp: (WebKit::WebPageProxy::queryPermission): (WebKit::WebPageProxy::clearNotificationPermissionState): * Source/WebKit/UIProcess/WebPermissionControllerProxy.cpp: (WebKit::WebPermissionControllerProxy::querySync): * Source/WebKit/UIProcess/WebPermissionControllerProxy.h: * Source/WebKit/UIProcess/WebPermissionControllerProxy.messages.in: * Source/WebKit/WebProcess/Notifications/NotificationPermissionRequestManager.cpp: (WebKit::NotificationPermissionRequestManager::startRequest): (WebKit::NotificationPermissionRequestManager::permissionLevel): Deleted. * Source/WebKit/WebProcess/Notifications/NotificationPermissionRequestManager.h: * Source/WebKit/WebProcess/Notifications/WebNotificationManager.cpp: (WebKit::WebNotificationManager::initialize): (WebKit::WebNotificationManager::didUpdateNotificationDecision): (WebKit::WebNotificationManager::didRemoveNotificationDecisions): (WebKit::WebNotificationManager::removeAllPermissionsForTesting): (WebKit::WebNotificationManager::policyForOrigin const): Deleted. * Source/WebKit/WebProcess/Notifications/WebNotificationManager.h: * Source/WebKit/WebProcess/WebCoreSupport/WebNotificationClient.cpp: (WebKit::WebNotificationClient::requestPermission): (WebKit::WebNotificationClient::checkPermission): (WebKit::WebNotificationClient::clearNotificationPermissionState): Deleted. * Source/WebKit/WebProcess/WebCoreSupport/WebNotificationClient.h: * Source/WebKit/WebProcess/WebCoreSupport/WebPermissionController.cpp: (WebKit::WebPermissionController::query): (WebKit::WebPermissionController::querySync): (WebKit::WebPermissionController::webPageProxyIdentifier): (WebKit::WebPermissionController::removeEntryFromCache): (WebKit::WebPermissionController::clearCache): (WebKit::WebPermissionController::tryProcessingRequests): * Source/WebKit/WebProcess/WebCoreSupport/WebPermissionController.h: (isType): * Source/WebKit/WebProcess/WebPage/WebPage.cpp: (WebKit::WebPage::clearNotificationPermissionState): Deleted. * Source/WebKit/WebProcess/WebPage/WebPage.h: * Source/WebKit/WebProcess/WebPage/WebPage.messages.in: * Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp: (WTR::TestRunner::removeAllWebNotificationPermissions): * Tools/WebKitTestRunner/TestController.cpp: (WTR::TestController::removeAllWebNotificationPermissions): * Tools/WebKitTestRunner/TestController.h: * Tools/WebKitTestRunner/TestInvocation.cpp: (WTR::TestInvocation::didReceiveSynchronousMessageFromInjectedBundle):

webkit-early-warning-system · 2022-08-26T23:33:46Z

92a17b9

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	🛠 ios	⏳ 🛠 mac	⏳ 🛠 wpe	⏳ 🛠 🧪 win
🧪 bindings	⏳ 🛠 ios-sim	⏳ 🛠 mac-debug	⏳ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	⏳ 🛠 mac-AS-debug	⏳ 🧪 gtk-wk2
	⏳ 🧪 api-ios	⏳ 🧪 api-mac	⏳ 🧪 api-gtk
⏳ 🛠 🧪 jsc	⏳ 🛠 tv	⏳ 🧪 mac-wk1	⏳ 🛠 jsc-armv7
🧪 services	⏳ 🛠 tv-sim	⏳ 🧪 mac-wk2	⏳ 🧪 jsc-armv7-tests
	⏳ 🛠 watch	⏳ 🧪 mac-AS-debug-wk2	⏳ 🛠 jsc-mips
	⏳ 🛠 watch-sim	⏳ 🧪 mac-wk2-stress	⏳ 🧪 jsc-mips-tests

webkit-early-warning-system · 2022-08-26T23:33:47Z

EWS run on previous version of this PR (hash 07ac0dc)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	❌ 🛠 ios	⏳ 🛠 mac	⏳ 🛠 wpe	❌ 🛠 🧪 win
❌ 🧪 bindings	❌ 🛠 ios-sim	⏳ 🛠 mac-debug	⏳ 🛠 gtk	⏳ 🛠 wincairo
❌ 🧪 webkitperl	❌ 🧪 ios-wk2	⏳ 🛠 mac-AS-debug	⏳ 🧪 gtk-wk2
	❌ 🧪 api-ios	⏳ 🧪 api-mac	⏳ 🧪 api-gtk
⏳ 🛠 🧪 jsc	⏳ 🛠 tv	⏳ 🧪 mac-wk1	⏳ 🛠 jsc-armv7
⏳ 🧪 services	❌ 🛠 tv-sim	⏳ 🧪 mac-wk2	⏳ 🧪 jsc-armv7-tests
	❌ 🛠 watch	⏳ 🧪 mac-AS-debug-wk2
	⏳ 🛠 watch-sim	⏳ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-08-26T23:33:49Z

92a17b9

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	❌ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

szewai · 2022-08-27T00:05:07Z

LayoutTests/http/tests/push-api/subscribe-deny-permissions-on-prompt.html

-                reject('Test requires internals.');
-                return;
-            }
+        if (!window.internals)


Can you explain in commit message that why this change is needed? It's not clear to me why pushManager would return different results after this patch

WebNotificationClient::checkPermission (and thus, Notification::permission) used to get the permission state from a cache in WebNotificationManager. This cache did not get updated by calling TestController::denyNotificationPermissionOnPrompt and then WebNotificationClient::requestPermission (which is what happens in this test). So when request2 occurred, WebNotificationClient::checkPermission would return default, then fail the transient activation check and reject the promise with a "user gesture" error.

This patch removes this cache and refactors WebNotificationClient::checkPermission (and thus, Notification::permission) to get the permission state from the client (which is TestController in the case of this test). The cache in TestController that stores the permissions state does get updated by calling TestController::denyNotificationPermissionOnPrompt and then WebNotificationClient::requestPermission. So now, when request2 occurs, WebNotificationClient::checkPermission returns denied and rejects the promise with a "user denied push permission" error before reaching the transient activation check.

I spoke with @bnham about this and learned that the point of this layout test is to check that given a user gesture, the first call to PushManager::subscribe would consume the transient activation and that the second call would not have a transient activation. Due to the change described above, this check on the second call was being circumvented. We can ensure that this check occurs by altering the test the way I did in this patch.

You're right, the reason for change is not clear just by looking at the test, so I'll update the commit message with the above explanation once the bots finish and I make any other suggested changes.

From what's described above, It seems the right thing is to update WebKitTestRunner, making it cache the request result like a real client would do, instead of changing the test, since you don't intend to change the tested behavior (that first subscribe request fails due to user activation and second fails due to permission request).

Our current design for permissions is that each query and request goes to client, which means client is responsible to cache the result if needed.
Specifically, you might call setPermission on WebNotificationProvider in TestController::decidePolicyForNotificationPermissionRequest. (to simulate user denies the prompt and browser remembers the choice).

I believe WebKitTestRunner already caches the result. TestController::decidePolicyForNotificationPermissionRequest. already calls setPermission on WebNotificationProvider. denyOnPrompt means that if you request/prompt for permission, the state will be set to denied. The test was expecting that after denyOnPrompt is called, then first request does a prompt, the second request will get that the permission state is default? But that doesn't seem to make sense. The permission state should be denied, no? The reason that the test was passing before this patch was because it was relying on the fact that WebKitTestRunner wasn't updating the cache in WebNotificationManager. Now that this cache is gone, and checkPermission actually goes to the client, and testRunner returns the result based on its cache in WebNotificationProvider, the second request correctly gets back that the permission state is denied.

szewai · 2022-08-27T00:37:52Z

Source/WebKit/WebProcess/Notifications/WebNotificationManager.cpp

+    auto& webPermissionController = downcast<WebPermissionController>(PermissionController::shared());
+    for (auto const& [originString, allowed] : parameters.notificationPermissions) {
+        auto topOrigin = SecurityOrigin::createFromString(originString)->data();
+        auto permissionState = allowed ? PermissionState::Granted : PermissionState::Denied;
+        webPermissionController.updateCache(ClientOrigin { topOrigin, SecurityOriginData { } }, PermissionDescriptor { PermissionName::Notifications }, permissionState);
+    }


Why this change? I thought we only want one implementation for both APIs, i.e. we will only use queryPermission to pull notification's permission.
This makes the cache dependent on WKNotificationManager: e.g. if client implements queryPermission but not WKNotificationManager, the result will be wrong (because cache will not be updated).

This patch contains other code changes that ensure that calling queryPermission will also update the cache (if the permission state being queried is the state of the Notifications API.

The reason for this change was that in a previous version of this patch, the EWS bots detected a API test failure on GTK. The failure was that Notification::permission was incorrectly returning "default" instead of "granted". It turns out that the test client did not implement queryPermission and the test had been succeeding because of the cache in WebNotificationManager. Since this patch removes that cache and refactors Notification::permission to use queryPermission, the result was coming back as default. There were two options to fix this:

Implement queryPermission in the GTK test client

Make the not-working WebProcess cache in WebPermissionController work for notifications.

The second option seemed easier and more beneficial since we'll have to do this for the on change event later on anyways. I wasn't sure how to test if this will fix the API test since it was a GTK failure, so I guess we'll have to wait for the bots to check. :(

This patch contains other code changes that ensure that calling queryPermission will also update the cache (if the permission state being queried is the state of the Notifications API.

Let's say client implements queryPermission but not WKNotificationManager, and user sets allowed notification (e.g. in per-site settings in Safari) for a site.
For the first permission::query, client returns allowed, and WebPermissionController caches the result. Then user decides to change the setting to denied (no longer wants to receive notification from the site), and tells WebKit client. For the second permission query, what would be returned with your patch?

IIUC, before this patch, notification.permission relies on WKNotificationManager and permission.query relies on queryPermission. After this patch, both rely on either WKNotificationManager or queryPermission + WKNotificationManager, which does not seem like a progress to me.

The second option seemed easier and more beneficial since we'll have to do this for the on change event later on anyways.

The reason we implement this before change event is because we think this change would work on its own. If this change relies on cache to work, then I think we should implement change event first, since the cache might not be in this form (an origin-to-permission map). You can imagine having WebPermissionController track PermissionStatus (or PermissionObserver) objects which hold the state, then you will need to revamp the cache.

Source/WebKit/WebProcess/Notifications/NotificationPermissionRequestManager.cpp

cdumez · 2022-08-29T14:50:43Z

Source/WebKit/WebProcess/Notifications/WebNotificationManager.cpp

+    for (auto const& [originString, allowed] : parameters.notificationPermissions) {
+        auto topOrigin = SecurityOrigin::createFromString(originString)->data();
+        auto permissionState = allowed ? PermissionState::Granted : PermissionState::Denied;
+        webPermissionController.updateCache(ClientOrigin { topOrigin, SecurityOriginData { } }, PermissionDescriptor { PermissionName::Notifications }, permissionState);


Same comment, your ClientOrigin seems wrong.

cdumez · 2022-08-29T14:52:08Z

Source/WebKit/WebProcess/Notifications/WebNotificationManager.cpp

+    auto& webPermissionController = downcast<WebPermissionController>(PermissionController::shared());
+    auto topOrigin = SecurityOrigin::createFromString(originString)->data();
+    auto permissionState = allowed ? PermissionState::Granted : PermissionState::Denied;
+    webPermissionController.updateCache(ClientOrigin { topOrigin, SecurityOriginData { } }, PermissionDescriptor { PermissionName::Notifications }, permissionState);


Again, bad ClientOrigin.

cdumez · 2022-08-29T14:52:23Z

Source/WebKit/WebProcess/Notifications/WebNotificationManager.cpp

    for (auto& originString : originStrings)
-        m_permissionsMap.remove(originString);
+        webPermissionController.removeEntryFromCache(ClientOrigin { SecurityOrigin::createFromString(originString)->data(), SecurityOriginData { } });


cdumez · 2022-08-29T14:53:09Z

Source/WebKit/WebProcess/Notifications/WebNotificationManager.cpp

 void WebNotificationManager::removeAllPermissionsForTesting()
 {
 #if ENABLE(NOTIFICATIONS)
-    m_permissionsMap.clear();
+    auto& webPermissionController = downcast<WebPermissionController>(PermissionController::shared());
+    webPermissionController.clearCache();


Well, this seems wrong isn't it? We used to clear only notification permissions, now we clear ALL permissions?

Yeah, this is a mistake. I'll fix this in the next upload of the patch.

cdumez · 2022-08-29T14:55:53Z

Source/WebKit/WebProcess/WebCoreSupport/WebNotificationClient.cpp

-    NotificationClient::Permission resultPermission;
-    callOnMainRunLoopAndWait([&resultPermission, origin = origin->data().toString().isolatedCopy()] {
-        resultPermission = WebProcess::singleton().supplement<WebNotificationManager>()->policyForOrigin(origin);
+    auto* page = m_page ? m_page->corePage() : nullptr;


m_page is a main thread object and is thus not safe to access off the main thread like you're doing here.

cdumez · 2022-08-29T14:56:22Z

Source/WebKit/WebProcess/WebCoreSupport/WebNotificationClient.cpp

+        if (!permissionState)
+            return;
+
+        switch (*permissionState) {


Seems like we should have a helper function to convert one enum type to the other.

cdumez · 2022-08-29T14:59:30Z

Source/WebKit/WebProcess/WebCoreSupport/WebPermissionController.h

@@ -74,3 +79,7 @@ class WebPermissionController final : public CanMakeWeakPtr<WebPermissionControl
 };

 } // namespace WebCore
+
+SPECIALIZE_TYPE_TRAITS_BEGIN(WebKit::WebPermissionController)
+    static bool isType(const WebCore::PermissionController&) { return true; }


How could this possibly be correct? You're doing zero type validation?

cdumez · 2022-08-29T15:04:29Z

LayoutTests/http/tests/push-api/subscribe-deny-permissions-on-prompt-expected.txt

 PASS: service worker permissionState was prompt
 PASS: document permissionState was prompt
 PASS: service worker subscribe was error: NotAllowedError
 PASS: document subscribe without user gesture was error: NotAllowedError
-PASS: document subscribe with user gesture was error: NotAllowedError
-PASS: document subscribe with consumed user gesture failed with user gesture error
+PASS: document subscribe with user gesture consumed the user gesture


The difference in behavior here is worrisome. This is meant to be a refactoring that maintain pre-existing behavior. Mixing behavior changes and refactoring in the same patch is usually a recipe for having a bad time later.

RupinMittal requested review from cdumez and JonWBedard as code owners August 3, 2022 17:32

RupinMittal self-assigned this Aug 3, 2022

RupinMittal added New Bugs Unclassified bugs are placed in this component until the correct component can be determined. WebKit Nightly Build labels Aug 3, 2022

RupinMittal requested review from szewai and geoffreygaren August 3, 2022 17:33

cdumez requested changes Aug 3, 2022

View reviewed changes

RupinMittal force-pushed the eng/Bug-243501 branch from 5971c5d to 3c2f221 Compare August 3, 2022 20:02

RupinMittal requested a review from cdumez August 3, 2022 20:05

szewai requested changes Aug 3, 2022

View reviewed changes

cdumez requested changes Aug 3, 2022

View reviewed changes

Source/WebKit/WebProcess/WebCoreSupport/WebNotificationClient.cpp Outdated Show resolved Hide resolved

webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label Aug 4, 2022

RupinMittal closed this Aug 4, 2022

RupinMittal reopened this Aug 25, 2022

RupinMittal removed the merging-blocked Applied to prevent a change from being merged label Aug 25, 2022

RupinMittal force-pushed the eng/Bug-243501 branch from 3c2f221 to 7e0bfbe Compare August 25, 2022 06:09

RupinMittal requested review from szewai and cdumez August 25, 2022 06:16

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 25, 2022

szewai requested changes Aug 25, 2022

View reviewed changes

RupinMittal removed the merging-blocked Applied to prevent a change from being merged label Aug 26, 2022

RupinMittal force-pushed the eng/Bug-243501 branch from 7e0bfbe to 07ac0dc Compare August 26, 2022 23:21

RupinMittal force-pushed the eng/Bug-243501 branch from 07ac0dc to 92a17b9 Compare August 26, 2022 23:29

RupinMittal requested a review from szewai August 26, 2022 23:37

szewai reviewed Aug 27, 2022

View reviewed changes

cdumez requested changes Aug 29, 2022

View reviewed changes

xeenon removed the WebKit Nightly Build label Nov 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Notification::permission to follow the same code path as Permissions::query #2982

Refactor Notification::permission to follow the same code path as Permissions::query #2982

RupinMittal commented Aug 3, 2022 •

edited by webkit-early-warning-system

szewai left a comment

webkit-early-warning-system commented Aug 25, 2022 •

edited

webkit-early-warning-system commented Aug 25, 2022 •

edited

webkit-early-warning-system commented Aug 25, 2022 •

edited

webkit-early-warning-system commented Aug 25, 2022 •

edited

szewai left a comment

webkit-early-warning-system commented Aug 26, 2022

webkit-early-warning-system commented Aug 26, 2022

webkit-early-warning-system commented Aug 26, 2022 •

edited

szewai Aug 27, 2022

RupinMittal Aug 27, 2022

szewai Aug 29, 2022

RupinMittal Aug 29, 2022

szewai Aug 27, 2022

RupinMittal Aug 27, 2022 •

edited

szewai Aug 29, 2022

cdumez Aug 29, 2022

cdumez Aug 29, 2022

cdumez Aug 29, 2022

cdumez Aug 29, 2022

RupinMittal Aug 29, 2022

cdumez Aug 29, 2022

cdumez Aug 29, 2022

cdumez Aug 29, 2022

cdumez Aug 29, 2022

Refactor Notification::permission to follow the same code path as Permissions::query #2982

Are you sure you want to change the base?

Refactor Notification::permission to follow the same code path as Permissions::query #2982

Conversation

RupinMittal commented Aug 3, 2022 • edited by webkit-early-warning-system

szewai left a comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Aug 25, 2022 • edited

webkit-early-warning-system commented Aug 25, 2022 • edited

webkit-early-warning-system commented Aug 25, 2022 • edited

webkit-early-warning-system commented Aug 25, 2022 • edited

szewai left a comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Aug 26, 2022

webkit-early-warning-system commented Aug 26, 2022

webkit-early-warning-system commented Aug 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RupinMittal Aug 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RupinMittal commented Aug 3, 2022 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Aug 25, 2022 •

edited

webkit-early-warning-system commented Aug 25, 2022 •

edited

webkit-early-warning-system commented Aug 25, 2022 •

edited

webkit-early-warning-system commented Aug 25, 2022 •

edited

webkit-early-warning-system commented Aug 26, 2022 •

edited

RupinMittal Aug 27, 2022 •

edited