Make sure to correctly notify of end of a ServiceWorkerJob when the context is stopped
authoryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 4 Mar 2019 20:30:58 +0000 (20:30 +0000)
committeryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 4 Mar 2019 20:30:58 +0000 (20:30 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195195

Reviewed by Chris Dumez.

Before the patch, we were notifying that some jobs were finished too aggressively at context stop time.
This was confusing the Network Process.
Only notify such jobs that have pending loads.
Improve the tracking of jobs doing registration resolution to ensure the Network Process gets notified
in case of a registration promise being resolved but the settling callback being not yet called while the context is stopped.

Covered by existing tests not crashing anymore, in particular imported/w3c/web-platform-tests/service-workers/service-worker/skip-waiting.https.html.

* workers/service/ServiceWorkerContainer.cpp:
(WebCore::ServiceWorkerContainer::jobResolvedWithRegistration):
(WebCore::ServiceWorkerContainer::notifyRegistrationIsSettled):
(WebCore::ServiceWorkerContainer::stop):
* workers/service/ServiceWorkerContainer.h:
* workers/service/ServiceWorkerJob.cpp:
(WebCore::ServiceWorkerJob::cancelPendingLoad):
* workers/service/ServiceWorkerJob.h:
(WebCore::ServiceWorkerJob::isLoading const):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@242372 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/workers/service/ServiceWorkerContainer.cpp
Source/WebCore/workers/service/ServiceWorkerContainer.h
Source/WebCore/workers/service/ServiceWorkerJob.cpp
Source/WebCore/workers/service/ServiceWorkerJob.h

index 0f6599e..c45e4eb 100644 (file)
@@ -1,3 +1,28 @@
+2019-03-04  Youenn Fablet  <youenn@apple.com>
+
+        Make sure to correctly notify of end of a ServiceWorkerJob when the context is stopped
+        https://bugs.webkit.org/show_bug.cgi?id=195195
+
+        Reviewed by Chris Dumez.
+
+        Before the patch, we were notifying that some jobs were finished too aggressively at context stop time.
+        This was confusing the Network Process.
+        Only notify such jobs that have pending loads.
+        Improve the tracking of jobs doing registration resolution to ensure the Network Process gets notified
+        in case of a registration promise being resolved but the settling callback being not yet called while the context is stopped.
+
+        Covered by existing tests not crashing anymore, in particular imported/w3c/web-platform-tests/service-workers/service-worker/skip-waiting.https.html.
+
+        * workers/service/ServiceWorkerContainer.cpp:
+        (WebCore::ServiceWorkerContainer::jobResolvedWithRegistration):
+        (WebCore::ServiceWorkerContainer::notifyRegistrationIsSettled):
+        (WebCore::ServiceWorkerContainer::stop):
+        * workers/service/ServiceWorkerContainer.h:
+        * workers/service/ServiceWorkerJob.cpp:
+        (WebCore::ServiceWorkerJob::cancelPendingLoad):
+        * workers/service/ServiceWorkerJob.h:
+        (WebCore::ServiceWorkerJob::isLoading const):
+
 2019-03-04  Chris Dumez  <cdumez@apple.com>
 
         [iOS] Improve our file picker
index b37da47..e866fb2 100644 (file)
@@ -421,10 +421,6 @@ void ServiceWorkerContainer::jobResolvedWithRegistration(ServiceWorkerJob& job,
 #endif
     ASSERT_WITH_MESSAGE(job.hasPromise() || job.data().type == ServiceWorkerJobType::Update, "Only soft updates have no promise");
 
-    auto guard = WTF::makeScopeExit([this, &job] {
-        destroyJob(job);
-    });
-
     if (job.data().type == ServiceWorkerJobType::Register)
         CONTAINER_RELEASE_LOG_IF_ALLOWED("jobResolvedWithRegistration: Registration job %" PRIu64 " succeeded", job.identifier().toUInt64());
     else {
@@ -432,32 +428,28 @@ void ServiceWorkerContainer::jobResolvedWithRegistration(ServiceWorkerJob& job,
         CONTAINER_RELEASE_LOG_IF_ALLOWED("jobResolvedWithRegistration: Update job %" PRIu64 " succeeded", job.identifier().toUInt64());
     }
 
-    std::function<void()> notifyWhenResolvedIfNeeded;
-    if (shouldNotifyWhenResolved == ShouldNotifyWhenResolved::Yes) {
-        notifyWhenResolvedIfNeeded = [connection = m_swConnection, registrationKey = data.key]() mutable {
-            callOnMainThread([connection = WTFMove(connection), registrationKey = registrationKey.isolatedCopy()] {
-                connection->didResolveRegistrationPromise(registrationKey);
-            });
-        };
-    }
+    auto guard = WTF::makeScopeExit([this, &job] {
+        destroyJob(job);
+    });
+
+    auto notifyIfExitEarly = WTF::makeScopeExit([this, &data, &shouldNotifyWhenResolved] {
+        if (shouldNotifyWhenResolved == ShouldNotifyWhenResolved::Yes)
+            notifyRegistrationIsSettled(data.key);
+    });
 
-    if (isStopped()) {
-        if (notifyWhenResolvedIfNeeded)
-            notifyWhenResolvedIfNeeded();
+    if (isStopped())
         return;
-    }
 
     auto promise = job.takePromise();
-    if (!promise) {
-        if (notifyWhenResolvedIfNeeded)
-            notifyWhenResolvedIfNeeded();
+    if (!promise)
         return;
-    }
 
-    scriptExecutionContext()->postTask([this, protectedThis = makeRef(*this), promise = WTFMove(promise), jobIdentifier = job.identifier(), data = WTFMove(data), notifyWhenResolvedIfNeeded = WTFMove(notifyWhenResolvedIfNeeded)](ScriptExecutionContext& context) mutable {
+    notifyIfExitEarly.release();
+
+    scriptExecutionContext()->postTask([this, protectedThis = RefPtr<ServiceWorkerContainer>(this), promise = WTFMove(promise), jobIdentifier = job.identifier(), data = WTFMove(data), shouldNotifyWhenResolved](ScriptExecutionContext& context) mutable {
         if (isStopped() || !context.sessionID().isValid()) {
-            if (notifyWhenResolvedIfNeeded)
-                notifyWhenResolvedIfNeeded();
+            if (shouldNotifyWhenResolved == ShouldNotifyWhenResolved::Yes)
+                notifyRegistrationIsSettled(data.key);
             return;
         }
 
@@ -465,9 +457,10 @@ void ServiceWorkerContainer::jobResolvedWithRegistration(ServiceWorkerJob& job,
 
         CONTAINER_RELEASE_LOG_IF_ALLOWED("jobResolvedWithRegistration: Resolving promise for job %" PRIu64 ". Registration ID: %" PRIu64, jobIdentifier.toUInt64(), registration->identifier().toUInt64());
 
-        if (notifyWhenResolvedIfNeeded) {
-            promise->whenSettled([notifyWhenResolvedIfNeeded = WTFMove(notifyWhenResolvedIfNeeded)] {
-                notifyWhenResolvedIfNeeded();
+        if (shouldNotifyWhenResolved == ShouldNotifyWhenResolved::Yes) {
+            m_ongoingSettledRegistrations.add(++m_lastOngoingSettledRegistrationIdentifier, registration->data().key);
+            promise->whenSettled([this, protectedThis = WTFMove(protectedThis), identifier = m_lastOngoingSettledRegistrationIdentifier] {
+                notifyRegistrationIsSettled(m_ongoingSettledRegistrations.take(identifier));
             });
         }
 
@@ -475,6 +468,13 @@ void ServiceWorkerContainer::jobResolvedWithRegistration(ServiceWorkerJob& job,
     });
 }
 
+void ServiceWorkerContainer::notifyRegistrationIsSettled(const ServiceWorkerRegistrationKey& registrationKey)
+{
+    callOnMainThread([connection = m_swConnection, registrationKey = registrationKey.isolatedCopy()] {
+        connection->didResolveRegistrationPromise(registrationKey);
+    });
+}
+
 void ServiceWorkerContainer::jobResolvedWithUnregistrationResult(ServiceWorkerJob& job, bool unregistrationResult)
 {
 #ifndef NDEBUG
@@ -639,9 +639,13 @@ void ServiceWorkerContainer::stop()
     m_readyPromise = nullptr;
     auto jobMap = WTFMove(m_jobMap);
     for (auto& ongoingJob : jobMap.values()) {
-        notifyFailedFetchingScript(*ongoingJob.job.get(), ResourceError { errorDomainWebKitInternal, 0, ongoingJob.job->data().scriptURL, "Job cancelled"_s, ResourceError::Type::Cancellation });
-        ongoingJob.job->cancelPendingLoad();
+        if (ongoingJob.job->cancelPendingLoad())
+            notifyFailedFetchingScript(*ongoingJob.job.get(), ResourceError { errorDomainWebKitInternal, 0, ongoingJob.job->data().scriptURL, "Job cancelled"_s, ResourceError::Type::Cancellation });
     }
+
+    auto registrationMap = WTFMove(m_ongoingSettledRegistrations);
+    for (auto& registration : registrationMap.values())
+        notifyRegistrationIsSettled(registration);
 }
 
 DocumentOrWorkerIdentifier ServiceWorkerContainer::contextIdentifier()
index 60818dd..9f4ce9b 100644 (file)
@@ -114,6 +114,8 @@ private:
     void derefEventTarget() final;
     void stop() final;
 
+    void notifyRegistrationIsSettled(const ServiceWorkerRegistrationKey&);
+
     std::unique_ptr<ReadyPromise> m_readyPromise;
 
     NavigatorBase& m_navigator;
@@ -145,6 +147,10 @@ private:
 
     uint64_t m_lastPendingPromiseIdentifier { 0 };
     HashMap<uint64_t, std::unique_ptr<PendingPromise>> m_pendingPromises;
+
+    uint64_t m_lastOngoingSettledRegistrationIdentifier { 0 };
+    HashMap<uint64_t, ServiceWorkerRegistrationKey> m_ongoingSettledRegistrations;
+
 };
 
 } // namespace WebCore
index 009b987..f0b146a 100644 (file)
@@ -166,12 +166,14 @@ void ServiceWorkerJob::notifyFinished()
     m_client.jobFailedLoadingScript(*this, error, Exception { error.isAccessControl() ? SecurityError : TypeError, "Script load failed"_s });
 }
 
-void ServiceWorkerJob::cancelPendingLoad()
+bool ServiceWorkerJob::cancelPendingLoad()
 {
     if (!m_scriptLoader)
-        return;
+        return false;
+
     m_scriptLoader->cancel();
     m_scriptLoader = nullptr;
+    return true;
 }
 
 } // namespace WebCore
index 3915f85..b55ab95 100644 (file)
@@ -69,7 +69,7 @@ public:
 
     const DocumentOrWorkerIdentifier& contextIdentifier() { return m_contextIdentifier; }
 
-    void cancelPendingLoad();
+    bool cancelPendingLoad();
 
 private:
     // WorkerScriptLoaderClient