Make sure calling showNotification will extend the service worker lifetime
<rdar://92978482>
Created attachment 459099 [details] Patch
Created attachment 459145 [details] Patch
Comment on attachment 459145 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=459145&action=review > Source/WebCore/workers/service/ServiceWorkerRegistration.cpp:312 > + if (auto* serviceWorkerGlobalScope = dynamicDowncast<ServiceWorkerGlobalScope>(context)) { > + if (auto* pushEvent = serviceWorkerGlobalScope->pushEvent()) { > + auto& globalObject = *JSC::jsCast<JSDOMGlobalObject*>(promise->globalObject()); > + auto& jsPromise = *JSC::jsCast<JSC::JSPromise*>(promise->promise()); > + pushEvent->waitUntil(DOMPromise::create(globalObject, jsPromise)); > + } > + } How does this work if the call to showNotification occurs asynchronously? It seems like dispatchPushEvent would have cleared the saved push event from ServiceWorkerGlobalScope by then. For instance: ``` async function showPush(reg, title, delay) { await new Promise(resolve => self.setTimeout(resolve, delay)); // Won't serviceWorkerGlobalScope->pushEvent() be null at this point? reg.showNotification(title); // Note that I'm simulating the mistake of not actually adding the showNotification promise to waitUntil. I think this code // would work as expected with your patch if it was `return reg.showNotification` instead. } self.addEventListener('push', (event) => { event.waitUntil(showPushAfterDelay(self.registration, "foobar", 100)); }); ``` I guess this patch is only here to guard against the specific case where someone calls showNotification directly in the push event handler. This will definitely fix some of the issues we have seen in the wild, but I'm not sure if it will fix all the issues. We might end up needing to add some grace period for the service worker to run after all promises related to the event have settled to support more existing websites.
(In reply to Ben Nham from comment #4) > Comment on attachment 459145 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=459145&action=review > > > Source/WebCore/workers/service/ServiceWorkerRegistration.cpp:312 > > + if (auto* serviceWorkerGlobalScope = dynamicDowncast<ServiceWorkerGlobalScope>(context)) { > > + if (auto* pushEvent = serviceWorkerGlobalScope->pushEvent()) { > > + auto& globalObject = *JSC::jsCast<JSDOMGlobalObject*>(promise->globalObject()); > > + auto& jsPromise = *JSC::jsCast<JSC::JSPromise*>(promise->promise()); > > + pushEvent->waitUntil(DOMPromise::create(globalObject, jsPromise)); > > + } > > + } > > How does this work if the call to showNotification occurs asynchronously? It > seems like dispatchPushEvent would have cleared the saved push event from > ServiceWorkerGlobalScope by then. Right, if you call showNotification, you enter an async world and should probably call waitUntil/do await showNotification. > I guess this patch is only here to guard against the specific case where > someone calls showNotification directly in the push event handler. This will > definitely fix some of the issues we have seen in the wild, but I'm not sure > if it will fix all the issues. We might end up needing to add some grace > period for the service worker to run after all promises related to the event > have settled to support more existing websites. This patch aligns with the spec (resolve the promise once show notifications are done). This patch also ensures there is no race conditions in case showNotification is synchronously called. We should probably consider extending the lifetime of the service worker in case of push events, at least if showNotification was called. In that case, there are good chances that the user will click on the notification, which will require the service worker again. This is best dealt with as a follow-up though.
Comment on attachment 459145 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=459145&action=review r=me with comments > Source/WebCore/Modules/notifications/Notification.cpp:149 > + auto scope = makeScopeExit([&callback] { callback(); }); I feel like using a CompletionHandlerCallingScope would be much nicer, since you could WTFMove() here. > Source/WebCore/Modules/notifications/Notification.cpp:176 > + scope.release(); And the release() here would give you your completion handler again so you can capture it in your lambda. It is less error prone. > Source/WebCore/dom/ScriptExecutionContext.cpp:769 > +CompletionHandler<void()> ScriptExecutionContext::notificationCallback(NotificationCallbackIdentifier identifier) Should have "take" in the name since it is actually modifying the underlying map. > Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp:84 > + m_pushEvent = &pushEvent; Are we sure m_pushEvent was not already initialized to a pushEvent here? > Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp:86 > + m_pushEvent = nullptr; If so, we'd lose that previous pushEvent here. Sometimes we want a "stack" behavior for this sort of things and restore the previous value of m_pushEvent rather than clearing it. I don't know if it applies here but if it does, we should consider using SetForScope instead. > Source/WebKit/NetworkProcess/Notifications/NetworkNotificationManager.cpp:106 > + callback(); Is it really OK here to be calling the callback without we've received the response to the IPC below? It looks weird. > Source/WebKit/UIProcess/Notifications/ServiceWorkerNotificationHandler.cpp:59 > + callback(); Given how callback is used, I'd recommend a ScopeExit or a CompletionHandlerCallingScope. > Source/WebKitLegacy/mac/WebCoreSupport/WebNotificationClient.mm:71 > + callback(); You are failing to call callback() in the early return case above. I'd suggest using a ScopeExit or a CompletionHanderCallingScope to avoid such bugs. > Tools/TestWebKitAPI/TestNotificationProvider.cpp:142 > +bool TestNotificationProvider::hasReceivedNotification() const I'd suggest inlining those 2 in the header given that those are trivial getter / setters.
Thanks for the review. (In reply to Chris Dumez from comment #6) > Comment on attachment 459145 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=459145&action=review > > r=me with comments > > > Source/WebCore/Modules/notifications/Notification.cpp:149 > > + auto scope = makeScopeExit([&callback] { callback(); }); > > I feel like using a CompletionHandlerCallingScope would be much nicer, since > you could WTFMove() here. OK > > Source/WebCore/dom/ScriptExecutionContext.cpp:769 > > +CompletionHandler<void()> ScriptExecutionContext::notificationCallback(NotificationCallbackIdentifier identifier) > > Should have "take" in the name since it is actually modifying the underlying > map. OK > > Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp:84 > > + m_pushEvent = &pushEvent; > > Are we sure m_pushEvent was not already initialized to a pushEvent here? Yes, we enqueue a task for each push event. I'll add an ASSERT. > > Source/WebKit/NetworkProcess/Notifications/NetworkNotificationManager.cpp:106 > > + callback(); > > Is it really OK here to be calling the callback without we've received the > response to the IPC below? It looks weird. AFAIK, this code is non functional (see comments in the same file). > > Source/WebKit/UIProcess/Notifications/ServiceWorkerNotificationHandler.cpp:59 > > + callback(); > > Given how callback is used, I'd recommend a ScopeExit or a > CompletionHandlerCallingScope. OK > > Source/WebKitLegacy/mac/WebCoreSupport/WebNotificationClient.mm:71 > > + callback(); > > You are failing to call callback() in the early return case above. I'd > suggest using a ScopeExit or a CompletionHanderCallingScope to avoid such > bugs. OK
Created attachment 459401 [details] Patch for landing
Created attachment 459403 [details] Patch for landing
Committed r294225 (250583@main): <https://commits.webkit.org/250583@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 459403 [details].