Skip to content
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.

Sign up for GitHub

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

Jump to bottom

SWServerJobQueue::scriptContextStarted might have a null registration #16161

Merged
merged 1 commit into from
Jul 30, 2023
Merged

SWServerJobQueue::scriptContextStarted might have a null registration #16161

merged 1 commit into from
Jul 30, 2023

Conversation

youennf
Copy link
Contributor

@youennf youennf commented Jul 28, 2023 •
edited by webkit-early-warning-system
Loading

3cb928b

SWServerJobQueue::scriptContextStarted might have a null registration
https://bugs.webkit.org/show_bug.cgi?id=259591
rdar://112997411

Reviewed by Alex Christensen.

From logs, it appears SWServerJobQueue::scriptContextStarted might have a nullptr registration.
One possibility is the following:
- A main thread service worker page is created.
- The service worker is being installed (in main thread) and succeeds. This triggers a callOnMainThread to execute the callback that will notify network process to continue its processing
- Before the callback is executed, the service worker page is closed and the network process is notified about this.
- The network process removes the registration from its map in SWServer::unregisterServiceWorkerClient.
- The network process processes the message to continue installing the service worker and continue with the current job.

To prevent this, we are now making sure to cancel the job of a preinstalling service worker whose registration is removed in SWServer::unregisterServiceWorkerClient.
Since this is a speculative fix, we transform the ASSERT(registration) in an if+ASSERT.
We add logging to make sure to keep track of this, in case this might trigger job queue hangs.

* Source/WebCore/workers/service/server/SWServer.cpp:
(WebCore::SWServer::unregisterServiceWorkerClient):
* Source/WebCore/workers/service/server/SWServerJobQueue.cpp:
(WebCore::SWServerJobQueue::scriptContextFailedToStart):
(WebCore::SWServerJobQueue::scriptContextStarted):

Canonical link: https://commits.webkit.org/266419@main

84644fd

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

@youennf youennf requested a review from cdumez as a code owner July 28, 2023 07:22
@youennf youennf self-assigned this Jul 28, 2023
@youennf youennf added the Service Workers Component for Service Workers bugs. label Jul 28, 2023
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jul 28, 2023 •
edited
Loading

EWS run on previous version of this PR (hash 22dbae9)

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

achristensen07
achristensen07 approved these changes Jul 28, 2023
View reviewed changes
Source/WebCore/workers/service/server/SWServerJobQueue.cpp Outdated
@@ -157,7 +157,10 @@ void SWServerJobQueue::scriptContextFailedToStart(const ServiceWorkerJobDataIden

// If an uncaught runtime script error occurs during the above step, then:
auto* registration = m_server.getRegistration(m_registrationKey);
RELEASE_LOG_ERROR(ServiceWorker, "SWServerJobQueue::scriptContextFailedToStart registration is null");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this log go into the if (!registration)?

Source/WebCore/workers/service/server/SWServerJobQueue.cpp Outdated
@@ -180,7 +183,10 @@ void SWServerJobQueue::scriptContextStarted(const ServiceWorkerJobDataIdentifier
return;

auto* registration = m_server.getRegistration(m_registrationKey);
RELEASE_LOG_ERROR(ServiceWorker, "SWServerJobQueue::scriptContextStarted registration is null");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Source/WebCore/workers/service/server/SWServerJobQueue.cpp
ASSERT(registration);
if (!registration)
return;

ASSERT(registration->preInstallationWorker());
registration->preInstallationWorker()->terminate();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems worth null checking this too, while we're at it.

@youennf youennf force-pushed the eng/SWServerJobQueuescriptContextStarted-might-have-a-null-registration branch from 22dbae9 to 84644fd Compare July 30, 2023 16:42
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jul 30, 2023 •
edited
Loading

EWS run on current version of this PR (hash 84644fd)

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

@youennf youennf added the merge-queue Applied to send a pull request to merge-queue label Jul 30, 2023
@youennf
SWServerJobQueue::scriptContextStarted might have a null registration
3cb928b 
https://bugs.webkit.org/show_bug.cgi?id=259591
rdar://112997411

Reviewed by Alex Christensen.

From logs, it appears SWServerJobQueue::scriptContextStarted might have a nullptr registration.
One possibility is the following:
- A main thread service worker page is created.
- The service worker is being installed (in main thread) and succeeds. This triggers a callOnMainThread to execute the callback that will notify network process to continue its processing
- Before the callback is executed, the service worker page is closed and the network process is notified about this.
- The network process removes the registration from its map in SWServer::unregisterServiceWorkerClient.
- The network process processes the message to continue installing the service worker and continue with the current job.

To prevent this, we are now making sure to cancel the job of a preinstalling service worker whose registration is removed in SWServer::unregisterServiceWorkerClient.
Since this is a speculative fix, we transform the ASSERT(registration) in an if+ASSERT.
We add logging to make sure to keep track of this, in case this might trigger job queue hangs.

* Source/WebCore/workers/service/server/SWServer.cpp:
(WebCore::SWServer::unregisterServiceWorkerClient):
* Source/WebCore/workers/service/server/SWServerJobQueue.cpp:
(WebCore::SWServerJobQueue::scriptContextFailedToStart):
(WebCore::SWServerJobQueue::scriptContextStarted):

Canonical link: https://commits.webkit.org/266419@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/SWServerJobQueuescriptContextStarted-might-have-a-null-registration branch from 84644fd to 3cb928b Compare July 30, 2023 23:19
@webkit-commit-queue
Copy link
Collaborator

Committed 266419@main (3cb928b): https://commits.webkit.org/266419@main

Reviewed commits have been landed. Closing PR #16161 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 3cb928b into WebKit:main Jul 30, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jul 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@achristensen07 achristensen07 achristensen07 approved these changes

@cdumez cdumez Awaiting requested review from cdumez cdumez is a code owner

Assignees

@youennf youennf

Labels
Service Workers Component for Service Workers bugs.
Projects
None yet
Milestone
No milestone
4 participants
@youennf @webkit-early-warning-system @webkit-commit-queue @achristensen07