Adopt more smart pointers in service worker code#22738
Merged
webkit-commit-queue merged 1 commit intoWebKit:mainfrom Jan 14, 2024
Merged
Adopt more smart pointers in service worker code#22738webkit-commit-queue merged 1 commit intoWebKit:mainfrom
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
Conversation
Collaborator
|
EWS run on previous version of this PR (hash 9909a85) Details |
webkit-ews-buildbot
added
the
merging-blocked
cdumez
force-pushed
the
267475_service_workers_smart_pointers
branch
from
9909a85 to
f8c57bb
Compare
Collaborator
Collaborator
|
EWS run on current version of this PR (hash f8c57bb) Details |
cdumez
requested review from
brentfulgham,
darinadler,
geoffreygaren,
rniwa and
youennf
geoffreygaren
approved these changes
Jan 14, 2024
Contributor
geoffreygaren
left a comment
geoffreygaren
left a comment
There was a problem hiding this comment.
r=me
I think this can land as-is. I'm curious about the choice to use CheckedPtr.
Contributor
There was a problem hiding this comment.
Given that this class can make weak pointers, and given our experience having a better time debugging WeakPtr / WeakRef usage compared to CheckedPtr usage, Did you consider using WeakRef instead of CheckedPtr?
Contributor
Author
There was a problem hiding this comment.
The issue is for stack variables. SWServer is not RefCounted so I have no way to protect a SWServer variable that is on the stack. Let's consider the following case:
CheckedPtr server = connection->server();
server->doStuff();
CheckedPtr is on the stack here. It prevents any use-after-free of the this pointer during the execution of doStuff(). Since it is on the stack, crashes are very debuggable too. The only issue with CheckedPtr is with non-stack pointers.
Now let's consider the use of WeakPtr instead:
WeakPtr server = connection->server();
server->doStuff();
server->doSomethingElse();
WeakPtr would protect me against doing a use-after-free of server when calling doSomethingElse() if doStuff() caused the SWServer object to get destroyed. However, WeakPtr wouldn't protect me against a use-after-free of the this pointer during the execution of either doStuff() or doSomethingElse().
CheckedPtr has its use cases. WeakPtr is not a straight replacement for all cases.
Contributor
There was a problem hiding this comment.
This is really interesting!
I guess it's really all about the 'this' pointer. The 'this' pointer is often annoying in C++ memory safety because it's implicit. The same problem pertains when you try to adopt handles for a moving garbage collector.
But it would be easy enough to expand our checker to know "Just like a RefCounted object requires a RefPtr in scope during use, so too a CanMakeCheckedPtr object requires a CheckedPtr in scope during use". Or perhaps this class can become reference counted. But I seem to remember that there's a Good Reason (TM) to make it single owner.
https://bugs.webkit.org/show_bug.cgi?id=267475 Reviewed by Geoffrey Garen. * Source/WebCore/workers/service/server/SWServer.cpp: (WebCore::SWServer::~SWServer): (WebCore::SWServer::workerByID const): (WebCore::SWServer::activeWorkerFromRegistrationID): (WebCore::SWServer::addRegistrationFromStore): (WebCore::SWServer::didSaveWorkerScriptsToDisk): (WebCore::SWServer::removeRegistration): (WebCore::SWServer::getRegistrations): (WebCore::SWServer::clearAll): (WebCore::SWServer::clearInternal): (WebCore::SWServer::Connection::finishFetchingScriptInServer): (WebCore::SWServer::Connection::didResolveRegistrationPromise): (WebCore::SWServer::Connection::addServiceWorkerRegistrationInServer): (WebCore::SWServer::Connection::removeServiceWorkerRegistrationInServer): (WebCore::SWServer::SWServer): (WebCore::SWServer::scheduleJob): (WebCore::SWServer::rejectJob): (WebCore::SWServer::resolveRegistrationJob): (WebCore::SWServer::resolveUnregistrationJob): (WebCore::SWServer::startScriptFetch): (WebCore::SWServer::scriptFetchFinished): (WebCore::SWServer::refreshImportedScripts): (WebCore::SWServer::refreshImportedScriptsFinished): (WebCore::SWServer::scriptContextFailedToStart): (WebCore::SWServer::scriptContextStarted): (WebCore::SWServer::terminatePreinstallationWorker): (WebCore::SWServer::didFinishInstall): (WebCore::SWServer::didFinishActivation): (WebCore::SWServer::claim): (WebCore::SWServer::didResolveRegistrationPromise): (WebCore::SWServer::addClientServiceWorkerRegistration): (WebCore::SWServer::removeClientServiceWorkerRegistration): (WebCore::SWServer::tryInstallContextData): (WebCore::SWServer::contextConnectionCreated): (WebCore::SWServer::forEachServiceWorker const): (WebCore::SWServer::terminateContextConnectionWhenPossible): (WebCore::SWServer::installContextData): (WebCore::SWServer::runServiceWorkerIfNecessary): (WebCore::SWServer::runServiceWorker): (WebCore::SWServer::markAllWorkersForRegistrableDomainAsTerminated): (WebCore::SWServer::workerContextTerminated): (WebCore::SWServer::fireInstallEvent): (WebCore::SWServer::removeConnection): (WebCore::SWServer::doRegistrationMatching): (WebCore::SWServer::updateAppInitiatedValueForWorkers): (WebCore::SWServer::registerServiceWorkerClient): (WebCore::SWServer::unregisterServiceWorkerClient): (WebCore::SWServer::removeContextConnectionIfPossible): (WebCore::SWServer::resolveRegistrationReadyRequests): (WebCore::SWServer::Connection::whenRegistrationReady): (WebCore::SWServer::Connection::storeRegistrationsOnDisk): (WebCore::SWServer::getAllOrigins): (WebCore::SWServer::createContextConnection): (WebCore::SWServer::softUpdate): (WebCore::SWServer::processPushMessage): (WebCore::SWServer::processNotificationEvent): (WebCore::SWServer::fireFunctionalEvent): (WebCore::SWServer::postMessageToServiceWorkerClient): (WebCore::SWServer::setInspectable): (WebCore::SWServer::Connection::startBackgroundFetch): (WebCore::SWServer::Connection::backgroundFetchInformation): (WebCore::SWServer::Connection::backgroundFetchIdentifiers): (WebCore::SWServer::Connection::abortBackgroundFetch): (WebCore::SWServer::Connection::matchBackgroundFetch): (WebCore::SWServer::Connection::retrieveRecordResponse): (WebCore::SWServer::Connection::retrieveRecordResponseBody): * Source/WebCore/workers/service/server/SWServer.h: (WebCore::SWServer::Connection::doRegistrationMatching): (WebCore::SWServer::Connection::server): (WebCore::SWServer::Connection::server const): (WebCore::SWServer::Connection::checkedServer const): * Source/WebCore/workers/service/server/SWServerJobQueue.cpp: (WebCore::SWServerJobQueue::scriptFetchFinished): (WebCore::SWServerJobQueue::importedScriptsFetchFinished): (WebCore::SWServerJobQueue::scriptAndImportedScriptsFetchFinished): (WebCore::SWServerJobQueue::scriptContextFailedToStart): (WebCore::SWServerJobQueue::scriptContextStarted): (WebCore::SWServerJobQueue::install): (WebCore::SWServerJobQueue::didResolveRegistrationPromise): (WebCore::SWServerJobQueue::didFinishInstall): (WebCore::SWServerJobQueue::runRegisterJob): (WebCore::SWServerJobQueue::runUnregisterJob): (WebCore::SWServerJobQueue::runUpdateJob): (WebCore::SWServerJobQueue::rejectCurrentJob): * Source/WebCore/workers/service/server/SWServerJobQueue.h: (WebCore::SWServerJobQueue::checkedServer const): * Source/WebCore/workers/service/server/SWServerRegistration.cpp: (WebCore::SWServerRegistration::forEachConnection): (WebCore::SWServerRegistration::notifyClientsOfControllerChange): (WebCore::SWServerRegistration::clear): (WebCore::SWServerRegistration::activate): (WebCore::SWServerRegistration::didFinishActivation): (WebCore::SWServerRegistration::isUnregistered const): (WebCore::SWServerRegistration::controlClient): (WebCore::SWServerRegistration::softUpdate): (WebCore::SWServerRegistration::enableNavigationPreload): (WebCore::SWServerRegistration::disableNavigationPreload): (WebCore::SWServerRegistration::setNavigationPreloadHeaderValue): * Source/WebCore/workers/service/server/SWServerRegistration.h: (WebCore::SWServerRegistration::checkedServer const): * Source/WebCore/workers/service/server/SWServerToContextConnection.cpp: (WebCore::SWServerToContextConnection::scriptContextFailedToStart): (WebCore::SWServerToContextConnection::scriptContextStarted): (WebCore::SWServerToContextConnection::didFinishInstall): (WebCore::SWServerToContextConnection::didFinishActivation): (WebCore::SWServerToContextConnection::setServiceWorkerHasPendingEvents): (WebCore::SWServerToContextConnection::workerTerminated): (WebCore::SWServerToContextConnection::matchAll): (WebCore::SWServerToContextConnection::findClientByVisibleIdentifier): (WebCore::SWServerToContextConnection::claim): (WebCore::SWServerToContextConnection::setScriptResource): (WebCore::SWServerToContextConnection::didFailHeartBeatCheck): (WebCore::SWServerToContextConnection::setAsInspected): (WebCore::SWServerToContextConnection::terminateWhenPossible): * Source/WebCore/workers/service/server/SWServerToContextConnection.h: * Source/WebCore/workers/service/server/SWServerWorker.cpp: (WebCore::m_lastNavigationWasAppInitiated): (WebCore::SWServerWorker::checkedServer const): (WebCore::SWServerWorker::updateAppInitiatedValue): (WebCore::SWServerWorker::startTermination): (WebCore::SWServerWorker::contextConnection): (WebCore::SWServerWorker::scriptContextFailedToStart): (WebCore::SWServerWorker::scriptContextStarted): (WebCore::SWServerWorker::didFinishInstall): (WebCore::SWServerWorker::didFinishActivation): (WebCore::SWServerWorker::contextTerminated): (WebCore::SWServerWorker::findClientByIdentifier const): (WebCore::SWServerWorker::findClientByVisibleIdentifier): (WebCore::SWServerWorker::matchAll): (WebCore::SWServerWorker::userAgent const): (WebCore::SWServerWorker::didSaveScriptsToDisk): (WebCore::SWServerWorker::setHasPendingEvents): (WebCore::SWServerWorker::setState): (WebCore::SWServerWorker::workerThreadMode const): (WebCore::SWServerWorker::shouldBeTerminated const): (WebCore::SWServerWorker::terminationIfPossibleTimerFired): (WebCore::SWServerWorker::isClientActiveServiceWorker const): * Source/WebCore/workers/service/server/SWServerWorker.h: * Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h: (WebKit::NetworkConnectionToWebProcess::swContextConnection): * Source/WebKit/NetworkProcess/NetworkSession.h: * Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerDownloadTask.cpp: (WebKit::ServiceWorkerDownloadTask::startListeningForIPC): (WebKit::ServiceWorkerDownloadTask::close): (WebKit::ServiceWorkerDownloadTask::sendToServiceWorker): (WebKit::ServiceWorkerDownloadTask::cancel): (WebKit::ServiceWorkerDownloadTask::setPendingDownloadLocation): (WebKit::ServiceWorkerDownloadTask::didFinish): (WebKit::ServiceWorkerDownloadTask::didFailDownload): * Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp: (WebKit::ServiceWorkerFetchTask::ServiceWorkerFetchTask): (WebKit::ServiceWorkerFetchTask::~ServiceWorkerFetchTask): (WebKit::ServiceWorkerFetchTask::sendToServiceWorker): (WebKit::ServiceWorkerFetchTask::sendToClient): (WebKit::ServiceWorkerFetchTask::startFetch): (WebKit::ServiceWorkerFetchTask::processRedirectResponse): (WebKit::ServiceWorkerFetchTask::processResponse): (WebKit::ServiceWorkerFetchTask::didReceiveData): (WebKit::ServiceWorkerFetchTask::didReceiveDataFromPreloader): (WebKit::ServiceWorkerFetchTask::didFinish): (WebKit::ServiceWorkerFetchTask::didFail): (WebKit::ServiceWorkerFetchTask::didNotHandle): (WebKit::ServiceWorkerFetchTask::usePreload): (WebKit::ServiceWorkerFetchTask::cannotHandle): (WebKit::ServiceWorkerFetchTask::continueFetchTaskWith): (WebKit::ServiceWorkerFetchTask::timeoutTimerFired): (WebKit::ServiceWorkerFetchTask::softUpdateIfNeeded): (WebKit::ServiceWorkerFetchTask::loadBodyFromPreloader): (WebKit::ServiceWorkerFetchTask::convertToDownload): * Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.h: * Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.cpp: (WebKit::ServiceWorkerNavigationPreloader::start): * Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.h: * Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerSoftUpdateLoader.cpp: (WebKit::ServiceWorkerSoftUpdateLoader::didComplete): * Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerSoftUpdateLoader.h: * Source/WebKit/NetworkProcess/ServiceWorker/WebSWOriginStore.cpp: (WebKit::WebSWOriginStore::didInvalidateSharedMemory): * Source/WebKit/NetworkProcess/ServiceWorker/WebSWRegistrationStore.cpp: (WebKit::WebSWRegistrationStore::checkedManager const): (WebKit::WebSWRegistrationStore::checkedServer const): (WebKit::WebSWRegistrationStore::clearAll): (WebKit::WebSWRegistrationStore::closeFiles): (WebKit::WebSWRegistrationStore::importRegistrations): (WebKit::WebSWRegistrationStore::updateToStorage): * Source/WebKit/NetworkProcess/ServiceWorker/WebSWRegistrationStore.h: * Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp: (WebKit::WebSWServerConnection::WebSWServerConnection): (WebKit::WebSWServerConnection::~WebSWServerConnection): (WebKit::WebSWServerConnection::updateRegistrationStateInClient): (WebKit::WebSWServerConnection::fireUpdateFoundEvent): (WebKit::WebSWServerConnection::setRegistrationLastUpdateTime): (WebKit::WebSWServerConnection::setRegistrationUpdateViaCache): (WebKit::WebSWServerConnection::updateWorkerStateInClient): (WebKit::WebSWServerConnection::createFetchTask): (WebKit::WebSWServerConnection::startFetch): (WebKit::WebSWServerConnection::postMessageToServiceWorker): (WebKit::WebSWServerConnection::scheduleJobInServer): (WebKit::WebSWServerConnection::clientURLFromIdentifier): (WebKit::WebSWServerConnection::scheduleUnregisterJobInServer): (WebKit::WebSWServerConnection::postMessageToServiceWorkerClient): (WebKit::WebSWServerConnection::matchRegistration): (WebKit::WebSWServerConnection::getRegistrations): (WebKit::WebSWServerConnection::registerServiceWorkerClientInternal): (WebKit::WebSWServerConnection::unregisterServiceWorkerClient): (WebKit::WebSWServerConnection::computeThrottleState const): (WebKit::WebSWServerConnection::updateThrottleState): (WebKit::WebSWServerConnection::subscribeToPushService): (WebKit::WebSWServerConnection::unsubscribeFromPushService): (WebKit::WebSWServerConnection::getPushSubscription): (WebKit::WebSWServerConnection::getPushPermissionState): (WebKit::WebSWServerConnection::fetchTaskTimedOut): (WebKit::WebSWServerConnection::enableNavigationPreload): (WebKit::WebSWServerConnection::disableNavigationPreload): (WebKit::WebSWServerConnection::setNavigationPreloadHeaderValue): (WebKit::WebSWServerConnection::getNavigationPreloadState): (WebKit::WebSWServerConnection::gatherClientData): (WebKit::WebSWServerConnection::retrieveRecordResponseBody): * Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp: (WebKit::WebSWServerToContextConnection::~WebSWServerToContextConnection): (WebKit::WebSWServerToContextConnection::networkProcess): (WebKit::WebSWServerToContextConnection::protectedIPCConnection const): (WebKit::WebSWServerToContextConnection::ipcConnection const): (WebKit::WebSWServerToContextConnection::postMessageToServiceWorkerClient): (WebKit::WebSWServerToContextConnection::skipWaiting): (WebKit::WebSWServerToContextConnection::firePushEvent): (WebKit::WebSWServerToContextConnection::fireNotificationEvent): (WebKit::WebSWServerToContextConnection::fireBackgroundFetchEvent): (WebKit::WebSWServerToContextConnection::fireBackgroundFetchClickEvent): (WebKit::WebSWServerToContextConnection::terminateDueToUnresponsiveness): (WebKit::WebSWServerToContextConnection::openWindow): (WebKit::WebSWServerToContextConnection::reportConsoleMessage): (WebKit::WebSWServerToContextConnection::connectionIsNoLongerNeeded): (WebKit::WebSWServerToContextConnection::webProcessIdentifier const): (WebKit::WebSWServerToContextConnection::focus): (WebKit::WebSWServerToContextConnection::navigate): * Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.h: * Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h: Canonical link: https://commits.webkit.org/273025@main
webkit-commit-queue
force-pushed
the
267475_service_workers_smart_pointers
branch
from
f8c57bb to
1c9013f
Compare
Collaborator
|
Committed 273025@main (1c9013f): https://commits.webkit.org/273025@main Reviewed commits have been landed. Closing PR #22738 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
1c9013f
f8c57bb