Changeset 242372 in webkit

Timestamp:

Mar 4, 2019, 12:30:58 PM (7 years ago)

Author:

youenn@apple.com

Message:

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):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• workers/service/ServiceWorkerContainer.cpp (modified) (4 diffs)
• workers/service/ServiceWorkerContainer.h (modified) (2 diffs)
• workers/service/ServiceWorkerJob.cpp (modified) (1 diff)
• workers/service/ServiceWorkerJob.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/workers/service/ServiceWorkerContainer.cpp

@@ -422 +422 @@
     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());
@@ -433 +429 @@
     }
 
-    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);
-            });
-        };
-    }
-
-    if (isStopped()) {
-        if (notifyWhenResolvedIfNeeded)
-            notifyWhenResolvedIfNeeded();
-        return;
-    }
+    auto guard = WTF::makeScopeExit([this, &job] {
+        destroyJob(job);
+    });
+
+    auto notifyIfExitEarly = WTF::makeScopeExit([this, &data, &shouldNotifyWhenResolved] {
+        if (shouldNotifyWhenResolved == ShouldNotifyWhenResolved::Yes)
+            notifyRegistrationIsSettled(data.key);
+    });
+
+    if (isStopped())
+        return;
 
     auto promise = job.takePromise();
-    if (!promise) {
-        if (notifyWhenResolvedIfNeeded)
-            notifyWhenResolvedIfNeeded();
-        return;
-    }
-
-    scriptExecutionContext()->postTask([this, protectedThis = makeRef(*this), promise = WTFMove(promise), jobIdentifier = job.identifier(), data = WTFMove(data), notifyWhenResolvedIfNeeded = WTFMove(notifyWhenResolvedIfNeeded)](ScriptExecutionContext& context) mutable {
+    if (!promise)
+        return;
+
+    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;
         }
@@ -466 +458 @@
         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));
             });
         }
 
         promise->resolve<IDLInterface<ServiceWorkerRegistration>>(WTFMove(registration));
+    });
+}
+
+void ServiceWorkerContainer::notifyRegistrationIsSettled(const ServiceWorkerRegistrationKey& registrationKey)
+{
+    callOnMainThread([connection = m_swConnection, registrationKey = registrationKey.isolatedCopy()] {
+        connection->didResolveRegistrationPromise(registrationKey);
     });
 }
@@ -640 +640 @@
     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);
 }
 

trunk/Source/WebCore/workers/service/ServiceWorkerContainer.h

@@ -115 +115 @@
     void stop() final;
 
+    void notifyRegistrationIsSettled(const ServiceWorkerRegistrationKey&);
+
     std::unique_ptr<ReadyPromise> m_readyPromise;
 
@@ -146 +148 @@
     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;
+
 };
 

trunk/Source/WebCore/workers/service/ServiceWorkerJob.cpp

@@ -167 +167 @@
 }
 
-void ServiceWorkerJob::cancelPendingLoad()
+bool ServiceWorkerJob::cancelPendingLoad()
 {
     if (!m_scriptLoader)
-        return;
+        return false;
+
     m_scriptLoader->cancel();
     m_scriptLoader = nullptr;
+    return true;
 }
 

trunk/Source/WebCore/workers/service/ServiceWorkerJob.h

@@ -70 +70 @@
     const DocumentOrWorkerIdentifier& contextIdentifier() { return m_contextIdentifier; }
 
-    void cancelPendingLoad();
+    bool cancelPendingLoad();
 
 private: