Implement ServiceWorkerRegistration.showNotification()

[Alexey Proskuryakov]

This is the same as HTML5 Window.showNotification() (which we don't implement yet): <http://www.whatwg.org/specs/web-apps/current-work/#dom-shownotification>.

[Alexey Proskuryakov]

<rdar://problem/6425808>

[Brady Eidson]

Lets reuse this for the modern bit left that's not supported yet, that's about to be.

[Brady Eidson]

Created attachment 451771 [details]
Patch for EWS

[Brady Eidson]

Created attachment 451772 [details]
EWS v2

[Brady Eidson]

Created attachment 451789 [details]
EWS v3

[Brady Eidson]

Created attachment 451790 [details]
EWS v4

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Brady Eidson]

Created attachment 451794 [details]
EWS v5

[Brady Eidson]

Created attachment 451795 [details]
EWS v6

[Brady Eidson]

Created attachment 451798 [details]
EWS v7

[Brady Eidson]

Filed https://bugs.webkit.org/show_bug.cgi?id=236545 to finish this off with getNotifications()

[Alex Christensen]

Comment on attachment 451798 [details]
EWS v7

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

> Source/WebKit/UIProcess/API/glib/WebKitNotificationProvider.cpp:122
> +        // I won't make something up for them.

probably not necessary.  Unclear who "I" is to a future reader.

> Source/WebKit/WebProcess/Notifications/WebNotificationManager.cpp:126
> +template<typename U> bool WebNotificationManager::sendNotificationMessage(U&& message, Notification& notification, WebPage* page)

nit: Could we pass in a std::variant<WebPage&, SWContextManager&>? or maybe std::reference_wrapper?

> Tools/WebKitTestRunner/WebNotificationProvider.cpp:158
> +    WKRetain(m_permissions.get());

Where's the corresponding release?

> Tools/WebKitTestRunner/WebNotificationProvider.h:56
> +    void setPermission(const String& origin, bool allowed);

Can this have a more descriptive name?

[Chris Dumez]

Comment on attachment 451798 [details]
EWS v7

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

r=me but please add change logs :)

> LayoutTests/http/tests/workers/service/shownotification-allowed.html:2
> +<script src="/resources/js-test-pre.js"></script>

Since you're not including js-test-post.js, seems you should be including js-test.js, not js-test-pre.js.

> LayoutTests/http/tests/workers/service/shownotification-allowed.html:10
> +        testRunner.notifyDone();

We're supposed to call finishJSTest() with js-test instead of notifyDone().

> LayoutTests/http/tests/workers/service/shownotification-allowed.html:14
> +    testRunner.waitUntilDone();

Can use `jsTestIsAsync = true;` since this is a js-test.

> LayoutTests/http/tests/workers/service/shownotification-allowed.html:69
> +            testRunner.notifyDone();

ditto about finishJSTest()

> LayoutTests/http/tests/workers/service/shownotification-denied-expected.txt:3
> +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".

See how we're missing the "TEST COMPLETE" at the end.

> LayoutTests/http/tests/workers/service/shownotification-denied.html:2
> +<script src="/resources/js-test-pre.js"></script>

Same comments apply to this test too.

> Source/WebCore/Modules/notifications/Notification.cpp:193
> +        auto* context = scriptExecutionContext();

I am not convinced that calling scriptExecutionContext() on the main thread here is really safe given that this is a service worker object.

> Source/WebCore/Modules/notifications/Notification.cpp:198
> +        static_cast<ServiceWorkerGlobalScope*>(context)->postTaskToFireNotificationEvent(NotificationEventType::Click, *this, { });

should be a downcast<ServiceWorkerGlobalScope>(context). Then you don't need your RELEASE_ASSERT() above.

> Source/WebCore/Modules/notifications/Notification.cpp:217
> +        static_cast<ServiceWorkerGlobalScope*>(context)->postTaskToFireNotificationEvent(NotificationEventType::Close, *this, { });

ditto.

> Source/WebCore/Modules/notifications/NotificationEvent.cpp:37
> +    auto* notification = init.notification.get();

Given that it is required and not nullable in the IDL, I think it would be good to assert that it is not null here and pass a C++ reference to the constructor to make it clear it is non-null. Sadly, we need to keep using a RefPtr<> (instead of a Ref<>) in the struct because of bindings generator limitations.

> Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp:78
> +    });

Not convinced this needed to be on the next line :)

> Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp:235
> +            auto event = NotificationEvent::create(eventName, protectedNotification.ptr(), action, ExtendableEvent::IsTrusted::Yes);

Ideally we'd pass a C++ reference to create() and drop the `.ptr()` here.

> Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp:236
> +            static_cast<ServiceWorkerGlobalScope&>(scope).dispatchEvent(event);

downcast<ServiceWorkerGlobalScope>(scope)

> Source/WebCore/workers/service/ServiceWorkerRegistration.cpp:301
> +    promise.reject(Exception { TypeError, "ServiceWorkerRegistration.getNotifications not yet implemented"_s });

NotSupportedError may be more appropriate than TypeError here I think.

> Source/WebCore/workers/service/server/SWServer.cpp:1232
> +        LOG(Push, "ServiceWorker import is complete, can handle push messasge now");

Typo: messasge

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2257
> +        LOG(Notifications, "NetworkProcess getting pending push messages for session ID %llu", sessionID.toUInt64());

Aren't we supposed to use PRIu64 to print uint64s to be nice to our Linux ports?

> Source/WebKit/UIProcess/Notifications/ServiceWorkerNotificationHandler.cpp:36
> +    static ServiceWorkerNotificationHandler& handler = *new ServiceWorkerNotificationHandler;

Seems like what we usually use NeverDestroyed<> for?

Also, we probably want an ASSERT(isMainRunLoop()); in here since this is not thread safe.

> Source/WebKit/UIProcess/Notifications/ServiceWorkerNotificationHandler.cpp:40
> +ServiceWorkerNotificationHandler::ServiceWorkerNotificationHandler()

= default; ?

> Source/WebKit/UIProcess/Notifications/ServiceWorkerNotificationHandler.cpp:64
> +    m_notificationToSessionMap.set(data.notificationID, data.sourceSession);

Can the key already exist in the map? If not, add() would be more efficient.

> Source/WebKit/UIProcess/Notifications/ServiceWorkerNotificationHandler.h:35
> +class ServiceWorkerNotificationHandler : public NotificationManagerMessageHandler {

Class could be marked as final.

> Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.cpp:54
> +    static WebNotificationManagerProxy* sharedManager = new WebNotificationManagerProxy(nullptr);

Could use NeverDestroyed<>

> Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.cpp:135
>      globalNotificationIDs.reserveCapacity(m_globalNotificationMap.size());

reserveInitialCapacity() would be more efficient here since this is a brand new Vector.

> Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.cpp:261
> +    WebProcessPool::sendToAllRemoteWorkerProcesses(Messages::WebNotificationManager::DidUpdateNotificationDecision(origin->securityOrigin().toString(), allowed));

Don't you want to only send to ServiceWorker processes? Why send this to Shared Worker processes too?

> Source/WebKit/UIProcess/WebProcessPool.h:838
> +void WebProcessPool::sendToAllRemoteWorkerProcesses(const T& message)

As mentioned above, maybe this should be named sendToAllServiceWorkerProcesses().

> Source/WebKit/UIProcess/WebProcessPool.h:841
> +        if (process.canSendMessage())

And then you could add a `&& process.isRunningServiceWorkers()` check here.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2071
> +    notifications.append(notificationID);

There has got to be a way to initialize the Vector with this notificationID right away, maybe:
Vector<UUID> notification = { notificationID };

Trying to leverage `Vector(std::initializer_list<T> initializerList)`.

> Source/WebKit/WebProcess/Notifications/WebNotificationManager.cpp:128
> +    if (!page && !notification.scriptExecutionContext())

I am not sure it is safe to call notification.scriptExecutionContext() on the main thread here if we're on the main thread and this is a Notification from a worker thread.

> Source/WebKit/WebProcess/Notifications/WebNotificationManager.cpp:161
> +    m_notificationIDMap.set(notification.identifier(), &notification);

Can the key be already present in the map (and we need to overwrite)? If not, add() would be more efficient.

> Source/WebKit/WebProcess/WebCoreSupport/WebNotificationClient.cpp:57
> +    callOnMainRunLoopAndWait([&result, protectedNotification = Ref { notification }]() {

Why do we need to wait here? (I'm sure there is a good reason, I'm just curious).

If you do need to wait, not that you could simplify the code by dropping the `if (isMainRunLoop())` check above and only do this callOnMainRunLoopAndWait() call. callOnMainRunLoopAndWait() already calls the lambda synchronously if already on the main run loop.

> Source/WebKit/WebProcess/WebCoreSupport/WebNotificationClient.cpp:70
> +    callOnMainRunLoopAndWait([protectedNotification = Ref { notification }]() {

Same comments as function above.

> Source/WebKit/WebProcess/WebCoreSupport/WebNotificationClient.cpp:85
> +    callOnMainRunLoopAndWait([&notification]() {

Same comments as function above.

> Source/WebKit/WebProcess/WebCoreSupport/WebNotificationClient.cpp:92
> +    callOnMainRunLoop([this] {

Does it need to be asynchronous always? If not, we could call ensureOnMainRunLoop() to call synchronously when already on the main thread.

> Source/WebKit/WebProcess/WebCoreSupport/WebNotificationClient.cpp:121
> +    callOnMainRunLoopAndWait([&resultPermission, origin = origin->data().toString()] {

Even though it is OK right now due to the implementation of toString(), I think it would be safer / more robust to call isolatedCopy() here. isolatedCopy() is anyway optimized when called on a rvalue reference and the ref count is 1.

> Tools/WebKitTestRunner/WebNotificationProvider.cpp:67
> +    m_permissions = adoptWK(WKMutableDictionaryCreate());

Should be in the initializer list.

> Tools/WebKitTestRunner/WebNotificationProvider.cpp:94
> +        auto context = WKPageGetContext(page);

We probably don't need this local variable and the curly brackets.

[Brady Eidson]

Most feedback taken in stride.

Some explanation for future posterity


(In reply to Chris Dumez from comment #13)
> 
> > Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.cpp:261
> > +    WebProcessPool::sendToAllRemoteWorkerProcesses(Messages::WebNotificationManager::DidUpdateNotificationDecision(origin->securityOrigin().toString(), allowed));
> 
> Don't you want to only send to ServiceWorker processes? Why send this to
> Shared Worker processes too?

There's zero harm sending this to SharedWorker proceses, and I'd actually made the conscious decision to send to all for general utility going forward.

> 
> > Source/WebKit/WebProcess/Notifications/WebNotificationManager.cpp:128
> > +    if (!page && !notification.scriptExecutionContext())
> 
> I am not sure it is safe to call notification.scriptExecutionContext() on
> the main thread here if we're on the main thread and this is a Notification
> from a worker thread.

This function will always be called from the thread where the notification was created and natively lives.

E.g. whether the ScriptExecutionContext is a Document or SWGS, this is the right thread.

> 
> > Source/WebKit/WebProcess/Notifications/WebNotificationManager.cpp:161
> > +    m_notificationIDMap.set(notification.identifier(), &notification);
> 
> Can the key be already present in the map (and we need to overwrite)? If
> not, add() would be more efficient.
> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebNotificationClient.cpp:57
> > +    callOnMainRunLoopAndWait([&result, protectedNotification = Ref { notification }]() {
> If you do need to wait, not that you could simplify the code by dropping the
> `if (isMainRunLoop())` check above and only do this
> callOnMainRunLoopAndWait() call. callOnMainRunLoopAndWait() already calls
> the lambda synchronously if already on the main run loop.

Not a bad idea, can be used for at least a few of these.

[Brady Eidson]

Created attachment 451802 [details]
Patch for landing v1

[Brady Eidson]

Comment on attachment 451802 [details]
Patch for landing v1

These test failures are a general malaise with the bots right now, and not related to this patch.

[EWS]

Found 5 new test failures: imported/w3c/web-platform-tests/css/css-cascade/layer-counter-style-override.html, imported/w3c/web-platform-tests/css/css-flexbox/overflow-auto-006.html, imported/w3c/web-platform-tests/css/css-flexbox/overflow-auto-008.html, imported/w3c/web-platform-tests/css/css-grid/grid-definition/grid-auto-repeat-max-size-001.html, imported/w3c/web-platform-tests/mathml/relations/html5-tree/dynamic-childlist-002.html

[Brady Eidson]

(In reply to EWS from comment #17)
> Found 5 new test failures:
> imported/w3c/web-platform-tests/css/css-cascade/layer-counter-style-override.
> html,
> imported/w3c/web-platform-tests/css/css-flexbox/overflow-auto-006.html,
> imported/w3c/web-platform-tests/css/css-flexbox/overflow-auto-008.html,
> imported/w3c/web-platform-tests/css/css-grid/grid-definition/grid-auto-
> repeat-max-size-001.html,
> imported/w3c/web-platform-tests/mathml/relations/html5-tree/dynamic-
> childlist-002.html

There is a general malaise on the bots right now, but these specific failures aren't part of it.

That said, they also clearly look unrelated.

[Brady Eidson]

(In reply to Brady Eidson from comment #18)
> (In reply to EWS from comment #17)
> > Found 5 new test failures:
> > imported/w3c/web-platform-tests/css/css-cascade/layer-counter-style-override.
> > html,
> > imported/w3c/web-platform-tests/css/css-flexbox/overflow-auto-006.html,
> > imported/w3c/web-platform-tests/css/css-flexbox/overflow-auto-008.html,
> > imported/w3c/web-platform-tests/css/css-grid/grid-definition/grid-auto-
> > repeat-max-size-001.html,
> > imported/w3c/web-platform-tests/mathml/relations/html5-tree/dynamic-
> > childlist-002.html
> 
> There is a general malaise on the bots right now, but these specific
> failures aren't part of it.
> 
> That said, they also clearly look unrelated.

Can definitely reproduce locally - rollout also fixes them.

So, a real issue.

Now on to: How the heck...

[Brady Eidson]

*Just* the WKTR changes alone cause this. At least I have that to dig in to.

[Brady Eidson]

Created attachment 451842 [details]
Patch for landing v2

[Brady Eidson]

For posterity: 


WKBundlePagePostSynchronousMessageForTesting has the bizarre and undocumented side effect of *forcing a layout*

So the timing of that call vs when the test harness scripts were included was, in fact, throwing off some CSS tests which were trying to inspect certain layout properties at certain times.

WAY too many tests rely on this layout behavior, so I'm adding a second form that doesn't do it.

[Chris Dumez]

Comment on attachment 451842 [details]
Patch for landing v2

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

> LayoutTests/http/tests/workers/service/shownotification-allowed-expected.txt:3
> +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".

Please notice that the tests are still wrong. There is no "TEST COMPLETE" at the end of the test. I explained how to fix this during my earlier review.

> LayoutTests/http/tests/workers/service/shownotification-denied-expected.txt:3
> +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".

ditto.

[Tim Nguyen (:ntim)]

Comment on attachment 451842 [details]
Patch for landing v2

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

> Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.h:132
>  WK_EXPORT void WKBundlePagePostSynchronousMessageForTesting(WKBundlePageRef page, WKStringRef messageName, WKTypeRef messageBody, WKTypeRef* returnRetainedData);
> +WK_EXPORT void WKBundlePagePostSynchronousMessageForTestingWithoutLayout(WKBundlePageRef page, WKStringRef messageName, WKTypeRef messageBody, WKTypeRef* returnRetainedData);

Ideally we'd reverse the naming `WKBundlePagePostSynchronousMessageForTestingWithLayout` and `WKBundlePagePostSynchronousMessageForTesting`, so we don't start using the layout version unnecessarily.

There are a relatively low amount of usages for this function:
https://webkit-search.igalia.com/webkit/search?q=WKBundlePagePostSynchronousMessageForTesting&path=&case=false&regexp=false

I think only the InjectedBundle.h one needs a layout, since they are used by EventSender (which is why the layout was introduced). I don't think InjectedBundle.cpp and TestRunner.cpp need it.

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:155
> +template<typename T> static WKRetainPtr<WKTypeRef> postSynchronousPageMessageWithReturnValue(const char* name, const WKRetainPtr<T>& value, WithLayout withLayout = WithLayout::Yes)

I doubt we need the withLayout setting for postSynchronousPageMessageWithReturnValue, it can just be No all the time IMO.

postSynchronousPageMessageWithReturnValue only has 2 usages: postSynchronousPageMessageReturningBoolean and postSynchronousMessageReturningUInt64.

Those 2 functions are not really used for anything layout related:
- https://webkit-search.igalia.com/webkit/search?q=postSynchronousPageMessageReturningUInt64&redirect=false
- https://webkit-search.igalia.com/webkit/search?q=postSynchronousPageMessageReturningBoolean&redirect=false

[EWS]

Committed r289721 (247206@main): <https://commits.webkit.org/247206@main>

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

[Brady Eidson]

(In reply to Tim Nguyen (:ntim) from comment #24)
> Comment on attachment 451842 [details]
> Patch for landing v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451842&action=review
> 
> > Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.h:132
> >  WK_EXPORT void WKBundlePagePostSynchronousMessageForTesting(WKBundlePageRef page, WKStringRef messageName, WKTypeRef messageBody, WKTypeRef* returnRetainedData);
> > +WK_EXPORT void WKBundlePagePostSynchronousMessageForTestingWithoutLayout(WKBundlePageRef page, WKStringRef messageName, WKTypeRef messageBody, WKTypeRef* returnRetainedData);
> 
> Ideally we'd reverse the naming
> `WKBundlePagePostSynchronousMessageForTestingWithLayout` and
> `WKBundlePagePostSynchronousMessageForTesting`, so we don't start using the
> layout version unnecessarily.
> 
> There are a relatively low amount of usages for this function:
> https://webkit-search.igalia.com/webkit/
> search?q=WKBundlePagePostSynchronousMessageForTesting&path=&case=false&regexp
> =false
> 
> I think only the InjectedBundle.h one needs a layout, since they are used by
> EventSender (which is why the layout was introduced). I don't think
> InjectedBundle.cpp and TestRunner.cpp need it.
> 
> > Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:155
> > +template<typename T> static WKRetainPtr<WKTypeRef> postSynchronousPageMessageWithReturnValue(const char* name, const WKRetainPtr<T>& value, WithLayout withLayout = WithLayout::Yes)
> 
> I doubt we need the withLayout setting for
> postSynchronousPageMessageWithReturnValue, it can just be No all the time
> IMO.
> 
> postSynchronousPageMessageWithReturnValue only has 2 usages:
> postSynchronousPageMessageReturningBoolean and
> postSynchronousMessageReturningUInt64.
> 
> Those 2 functions are not really used for anything layout related:
> -
> https://webkit-search.igalia.com/webkit/
> search?q=postSynchronousPageMessageReturningUInt64&redirect=false
> -
> https://webkit-search.igalia.com/webkit/
> search?q=postSynchronousPageMessageReturningBoolean&redirect=false

When I flipped the switch unconditionally, *TONS* of tests started immediately failing.

MAYBE only the EventSender message needs the layout. But at this point I have a feeling the number of tests that incorrectly *rely* on having the layout is non-zero...

Anyways, a worthwhile endeavor to figure out, but not in this patch.

[youenn fablet]

Comment on attachment 451842 [details]
Patch for landing v2

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

> Source/WebCore/Modules/notifications/Notification.cpp:210
> +        ServiceWorkerGlobalScope::ensureOnContextThread(m_contextIdentifier, [this, protectedThis = Ref { *this }](auto& context) {

AIUI, there is no guarantee that the service worker is running at this point.
Spec says to fire a functional event (https://notifications.spec.whatwg.org/#fire-a-service-worker-notification-event), which would guarantee to start the service worker as needed.
We probably need to use the same path as for push events: go from UIProcess to network process and call SWServer::fireFunctionalEvent.
UIProcess would need to provide the notification data and the service worker registration URL so that we can run the corresponding service worker as needed.

I am not clear what we can do if the service worker is broken or takes some time to answer.
Is there a way we could load a page in case service worker is too slow (like we go to network in case of fetch event) from the notification data?