-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add console logging message to missing or late showNotification calls in service worker #12236
Conversation
EWS run on previous version of this PR (hash 4706b9c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I do wonder if we should try to be more specific in the error message and provide guidance about calling waitUntil on the push event,since that seems to be a common issue devs are running into.
@@ -319,6 +316,12 @@ void ServiceWorkerRegistration::showNotification(ScriptExecutionContext& context | |||
auto& jsPromise = *JSC::jsCast<JSC::JSPromise*>(promise->promise()); | |||
pushEvent->waitUntil(DOMPromise::create(globalObject, jsPromise)); | |||
} | |||
|
|||
if (!serviceWorkerGlobalScope->hasPendingSilentPushEvent() && serviceWorkerGlobalScope->didFirePushEventRecently()) | |||
serviceWorkerGlobalScope->addConsoleMessage(MessageSource::Storage, MessageLevel::Warning, "showNotification was called outside of push event lifetime"_s, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this message also mention that you may want to pass the promised returned by showNotification
to the push event's waitUntil
to appropriately extend the lifetime of the push event?
This is wordy, but I think we need something like: "This service worker showed a notification outside of the lifetime of a recent push event. This may cause the push event to be spuriously marked as a silent push. To fix this, consider passing the promise returned by showNotification to the push event's waitUntil. This will extend the lifetime of the push event to encompass the lifetime of the notification."
An alternative is to put the wordy explanation on the web somewhere, and have this error message link to the wordy explanation. I've seen Chrome do that in console messages but I'm not sure if we do that anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll add a mention to PushEvent.waitUntil, trying to stick with a short version.
if (context.isServiceWorkerGlobalScope()) | ||
downcast<ServiceWorkerGlobalScope>(context).setHasPendingSilentPushEvent(false); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we originally put this check here defensively so that the web developer gets credit for trying to show a notification even if creating the notification creation fails for some reason. But looking through Notification::createForServiceWorker
, this basically shouldn't happen except in edge cases (e.g. if the web dev tries to pass in a too-large data
property into the Notification constructor). So I guess it's okay to move this down a few lines to where you've moved it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I think we should move it, see for instance rdar://107461729
@@ -246,6 +246,9 @@ void ServiceWorkerThread::queueTaskToFirePushEvent(std::optional<Vector<uint8_t> | |||
} | |||
|
|||
bool showedNotification = !serviceWorkerGlobalScope->hasPendingSilentPushEvent(); | |||
serviceWorkerGlobalScope->setHasPendingSilentPushEvent(false); | |||
if (!showedNotification) | |||
serviceWorkerGlobalScope->addConsoleMessage(MessageSource::Storage, MessageLevel::Error, "Push event ended without showing any notification"_s, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should be explicit about the consequences here. Like "Push event ended without showing any notification. After three such silent push events, the browser will remove your push subscription."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll mention the risk of the push subscription removal, but not too precise (I do not want to change the message if we change the rule).
4706b9c
to
8239371
Compare
EWS run on previous version of this PR (hash 8239371) |
8239371
to
e669b60
Compare
EWS run on previous version of this PR (hash e669b60) |
e669b60
to
2165d6b
Compare
EWS run on previous version of this PR (hash 2165d6b) |
2165d6b
to
56fbfa6
Compare
EWS run on previous version of this PR (hash 56fbfa6) |
56fbfa6
to
cfb2153
Compare
EWS run on previous version of this PR (hash cfb2153) |
cfb2153
to
1bd089c
Compare
EWS run on current version of this PR (hash 1bd089c) |
β¦ in service worker https://bugs.webkit.org/show_bug.cgi?id=254811 rdar://problem/107471641 Reviewed by Ben Nham. Add console log messages in case no showNotification is called during the lifetime of the push event. Add console log messages in case a showNotification is called slightly too late after the end of the push event. Add test API to be able to recover these console messages for WTR and related internals API. Covered by added test. * LayoutTests/http/wpt/service-workers/push-console-messages-no-showNotification.https-expected.txt: Added. * LayoutTests/http/wpt/service-workers/push-console-messages-no-showNotification.https.html: Added. * LayoutTests/http/wpt/service-workers/push-console-messages-showNotification-async.https-expected.txt: Added. * LayoutTests/http/wpt/service-workers/push-console-messages-showNotification-async.https.html: Added. * LayoutTests/http/wpt/service-workers/push-console-messages-showNotification-late.https-expected.txt: Added. * LayoutTests/http/wpt/service-workers/push-console-messages-showNotification-late.https.html: Added. * LayoutTests/http/wpt/service-workers/push-console-messages-showNotification-sync.https-expected.txt: Added. * LayoutTests/http/wpt/service-workers/push-console-messages-showNotification-sync.https.html: Added. * LayoutTests/http/wpt/service-workers/push-console-messages-worker.js: Added. (async doPush): (async doTest): * LayoutTests/http/wpt/service-workers/push-console-messages.js: Added. (promise_test.async test): (async doTest): * LayoutTests/platform/glib/http/wpt/service-workers/push-console-messages-no-showNotification.https-expected.txt: Added. * LayoutTests/platform/glib/http/wpt/service-workers/push-console-messages-showNotification-async.https-expected.txt: Added. * LayoutTests/platform/glib/http/wpt/service-workers/push-console-messages-showNotification-late.https-expected.txt: Added. * LayoutTests/platform/glib/http/wpt/service-workers/push-console-messages-showNotification-sync.https-expected.txt: Added. * Source/WebCore/testing/ServiceWorkerInternals.cpp: (WebCore::ServiceWorkerInternals::enableConsoleMessageReporting): (WebCore::logReportedConsoleMessage): * Source/WebCore/testing/ServiceWorkerInternals.h: * Source/WebCore/testing/ServiceWorkerInternals.idl: * Source/WebCore/workers/WorkerGlobalScope.h: * Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp: (WebCore::ServiceWorkerGlobalScope::dispatchPushEvent): (WebCore::ServiceWorkerGlobalScope::didFirePushEventRecently const): (WebCore::ServiceWorkerGlobalScope::addConsoleMessage): * Source/WebCore/workers/service/ServiceWorkerGlobalScope.h: * Source/WebCore/workers/service/ServiceWorkerRegistration.cpp: (WebCore::ServiceWorkerRegistration::showNotification): * Source/WebCore/workers/service/context/SWContextManager.h: * Source/WebCore/workers/service/context/ServiceWorkerThread.cpp: (WebCore::ServiceWorkerThread::queueTaskToFirePushEvent): * Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp: (WebKit::WebSWServerToContextConnection::reportConsoleMessage): * Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.h: * Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.messages.in: * Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm: * Source/WebKit/UIProcess/API/Cocoa/_WKWebsiteDataStoreDelegate.h: * Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp: (WebKit::NetworkProcessProxy::reportConsoleMessage): * Source/WebKit/UIProcess/Network/NetworkProcessProxy.h: * Source/WebKit/UIProcess/Network/NetworkProcessProxy.messages.in: * Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp: (WebKit::WebsiteDataStore::reportServiceWorkerConsoleMessage): * Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h: * Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreClient.h: (WebKit::WebsiteDataStoreClient::reportServiceWorkerConsoleMessage): * Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp: (WebKit::WebSWContextManagerConnection::reportConsoleMessage): * Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.h: * Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm: (-[ServiceWorkerOpenWindowWebsiteDataStoreDelegate websiteDataStore:openWindow:fromServiceWorkerOrigin:completionHandler:]): * Tools/WebKitTestRunner/TestController.cpp: (WTR::TestController::receivedServiceWorkerConsoleMessage): * Tools/WebKitTestRunner/TestController.h: * Tools/WebKitTestRunner/cocoa/TestWebsiteDataStoreDelegate.mm: (-[TestWebsiteDataStoreDelegate websiteDataStore:reportServiceWorkerConsoleMessage:]): Canonical link: https://commits.webkit.org/262584@main
1bd089c
to
9eb7504
Compare
Committed 262584@main (9eb7504): https://commits.webkit.org/262584@main Reviewed commits have been landed. Closing PR #12236 and removing active labels. |
9eb7504
1bd089c
π§ͺ mac-AS-debug-wk2