Skip to content

Commit 2172873

Browse files
youennfyouennf
committed
A service worker task should be done when its NetworkResourceLoader is don
https://bugs.webkit.org/show_bug.cgi?id=327822 rdar://problem/190211445 Reviewed by NOBODY (OOPS!). We want to align NetworkResourceLoader handling of m_serviceWorkerFetchTask with m_networkLoad. For that reason, we are now cancelling/nullifying m_serviceWorkerFetchTask in NetworkResourceLoader::cleanup. Given there are some async callbacks (content filtering for instance), to ensure that we do not restart either network loads or service worker tasks after the NetworkResourceLoader has been cleaned up, we add NetworkResourceLoader::m_isDone and set it to true in NetworkResourceLoader::cleanup. We use it in NetworkResourceLoader::continueWillSendRequest and NetworkResourceLoader::startWithServiceWorker to check at async return time whether we should continue. As a strengthening, we update ServiceWorkerFetchTask to include more m_isDone checks so that we do not create reentrancy potential issues. Covered by existing tests. * Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp: (WebKit::NetworkResourceLoader::~NetworkResourceLoader): (WebKit::NetworkResourceLoader::cleanup): (WebKit::NetworkResourceLoader::convertToDownload): (WebKit::NetworkResourceLoader::abort): (WebKit::NetworkResourceLoader::continueWillSendRequest): (WebKit::NetworkResourceLoader::startWithServiceWorker): * Source/WebKit/NetworkProcess/NetworkResourceLoader.h: * Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp: (WebKit::ServiceWorkerFetchTask::didFinish): (WebKit::ServiceWorkerFetchTask::didFail):
1 parent e50b892 commit 2172873

3 files changed

Lines changed: 30 additions & 5 deletions

File tree

‎Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp‎

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,7 @@ NetworkResourceLoader::~NetworkResourceLoader()
213213
{
214214
ASSERT(RunLoop::isMain());
215215
ASSERT(!m_networkLoad);
216+
ASSERT(!m_serviceWorkerFetchTask);
216217
ASSERT(!isSynchronous() || !m_synchronousLoadData->delayedReply);
217218
ASSERT(m_fileReferences.isEmpty());
218219
#if HAVE(BROKEN_MULTIPART_RESPONSE_FLOW_CONTROL)
@@ -680,6 +681,8 @@ void NetworkResourceLoader::cleanup(LoadResult result)
680681
ASSERT(RunLoop::isMain());
681682
LOADER_RELEASE_LOG("cleanup: (result=%u)", static_cast<unsigned>(result));
682683

684+
m_isDone = true;
685+
683686
NetworkActivityTracker::CompletionCode code { };
684687
switch (result) {
685688
case LoadResult::Unknown:
@@ -706,6 +709,9 @@ void NetworkResourceLoader::cleanup(LoadResult result)
706709
if (RefPtr networkLoad = std::exchange(m_networkLoad, nullptr))
707710
networkLoad->clearClient();
708711

712+
if (RefPtr task = std::exchange(m_serviceWorkerFetchTask, { }))
713+
task->cancelFromClient();
714+
709715
// This will cause NetworkResourceLoader to be destroyed and therefore we do it last.
710716
connection->didCleanupResourceLoader(*this);
711717
}
@@ -714,9 +720,11 @@ void NetworkResourceLoader::convertToDownload(DownloadID downloadID, const Resou
714720
{
715721
LOADER_RELEASE_LOG("convertToDownload: (downloadID=%" PRIu64 ", hasNetworkLoad=%d, pendingResponseCompletionHandlers=%zu)", downloadID.toUInt64(), !!m_networkLoad, m_responseCompletionHandlers.size());
716722

717-
RefPtr task = m_serviceWorkerFetchTask;
718-
if (task && task->convertToDownload(protect(protect(connectionToWebProcess().networkProcess())->downloadManager()), downloadID, request, response))
719-
return;
723+
if (RefPtr task = std::exchange(m_serviceWorkerFetchTask, { })) {
724+
if (task->convertToDownload(protect(protect(connectionToWebProcess().networkProcess())->downloadManager()), downloadID, request, response))
725+
return;
726+
task->cancelFromClient();
727+
}
720728

721729
// This can happen if the resource came from the disk cache.
722730
if (!m_networkLoad) {
@@ -750,7 +758,7 @@ void NetworkResourceLoader::abort()
750758
return;
751759
}
752760

753-
if (auto task = WTF::move(m_serviceWorkerFetchTask)) {
761+
if (RefPtr task = std::exchange(m_serviceWorkerFetchTask, { })) {
754762
LOADER_RELEASE_LOG("abort: Cancelling pending service worker fetch task (fetchIdentifier=%" PRIu64 ")", task->fetchIdentifier().toUInt64());
755763
task->cancelFromClient();
756764
}
@@ -2102,6 +2110,10 @@ void NetworkResourceLoader::continueWillSendRequest(ResourceRequest&& newRequest
21022110
if (CheckedPtr session = protect(connectionToWebProcess())->networkSession()) {
21032111
if (RefPtr server = session->swServer(); server && !server->isImportCompletedForOrigin(topOrigin)) {
21042112
server->importRegistrationsForOrigin(topOrigin, [this, protectedThis = Ref { *this }, newRequest = WTF::move(newRequest), isAllowedToAskUserForCredentials, completionHandler = WTF::move(completionHandler)]() mutable {
2113+
if (m_isDone) {
2114+
LOADER_RELEASE_LOG("continueWillSendRequest: Load was cleaned up while importing registrations");
2115+
return completionHandler({ });
2116+
}
21052117
continueWillSendRequest(WTF::move(newRequest), isAllowedToAskUserForCredentials, WTF::move(completionHandler));
21062118
});
21072119
return;
@@ -2874,6 +2886,11 @@ void NetworkResourceLoader::startWithServiceWorker()
28742886
if (request.isNull())
28752887
return;
28762888

2889+
if (protectedThis->m_isDone) {
2890+
LOADER_RELEASE_LOG_WITH_THIS(protectedThis, "startWithServiceWorker: Load was cleaned up while waiting, not starting");
2891+
return;
2892+
}
2893+
28772894
ASSERT(!protectedThis->m_serviceWorkerFetchTask);
28782895
protectedThis->m_serviceWorkerFetchTask = protect(protectedThis->connectionToWebProcess())->createFetchTask(protectedThis, request);
28792896
if (protectedThis->m_serviceWorkerFetchTask) {

‎Source/WebKit/NetworkProcess/NetworkResourceLoader.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -417,6 +417,7 @@ class NetworkResourceLoader final
417417

418418
bool m_shouldCaptureExtraNetworkLoadMetrics { false };
419419
bool m_isKeptAlive { false };
420+
bool m_isDone { false };
420421
bool m_hasReceivedEarlyHints { false };
421422
bool m_canUseCompressionDictionary { false };
422423

‎Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -382,7 +382,10 @@ void ServiceWorkerFetchTask::didReceiveFormData(const IPC::FormDataReference& fo
382382
void ServiceWorkerFetchTask::didFinish(const NetworkLoadMetrics& networkLoadMetrics)
383383
{
384384
ASSERT(!m_timeoutTimer || !m_timeoutTimer->isActive());
385-
SWFETCH_RELEASE_LOG("didFinish:");
385+
SWFETCH_RELEASE_LOG("didFinish: (m_isDone=%d)", m_isDone);
386+
387+
if (m_isDone)
388+
return;
386389

387390
m_isDone = true;
388391
if (m_timeoutTimer)
@@ -399,6 +402,9 @@ void ServiceWorkerFetchTask::didFinish(const NetworkLoadMetrics& networkLoadMetr
399402

400403
void ServiceWorkerFetchTask::didFail(const ResourceError& error)
401404
{
405+
if (m_isDone)
406+
return;
407+
402408
m_isDone = true;
403409
if (m_timeoutTimer && m_timeoutTimer->isActive()) {
404410
m_timeoutTimer->stop();
@@ -498,6 +504,7 @@ void ServiceWorkerFetchTask::continueFetchTaskWith(ResourceRequest&& request)
498504
SWFETCH_RELEASE_LOG("continueFetchTaskWith: (hasServiceWorkerConnection=%d)", !!m_serviceWorkerConnection);
499505
Ref loader = *m_loader;
500506
if (!m_serviceWorkerConnection) {
507+
m_isDone = true;
501508
loader->serviceWorkerDidNotHandle(this);
502509
return;
503510
}

0 commit comments

Comments
 (0)