<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[242372] trunk/Source/WebCore</title>
</head>
<body>

<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt;  }
#msg dl a { font-weight: bold}
#msg dl a:link    { color:#fc3; }
#msg dl a:active  { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff  {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/242372">242372</a></dd>
<dt>Author</dt> <dd>youenn@apple.com</dd>
<dt>Date</dt> <dd>2019-03-04 12:30:58 -0800 (Mon, 04 Mar 2019)</dd>
</dl>

<h3>Log Message</h3>
<pre>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):</pre>

<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceWebCoreChangeLog">trunk/Source/WebCore/ChangeLog</a></li>
<li><a href="#trunkSourceWebCoreworkersserviceServiceWorkerContainercpp">trunk/Source/WebCore/workers/service/ServiceWorkerContainer.cpp</a></li>
<li><a href="#trunkSourceWebCoreworkersserviceServiceWorkerContainerh">trunk/Source/WebCore/workers/service/ServiceWorkerContainer.h</a></li>
<li><a href="#trunkSourceWebCoreworkersserviceServiceWorkerJobcpp">trunk/Source/WebCore/workers/service/ServiceWorkerJob.cpp</a></li>
<li><a href="#trunkSourceWebCoreworkersserviceServiceWorkerJobh">trunk/Source/WebCore/workers/service/ServiceWorkerJob.h</a></li>
</ul>

</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (242371 => 242372)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/ChangeLog   2019-03-04 20:26:06 UTC (rev 242371)
+++ trunk/Source/WebCore/ChangeLog      2019-03-04 20:30:58 UTC (rev 242372)
</span><span class="lines">@@ -1,3 +1,28 @@
</span><ins>+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):
+
</ins><span class="cx"> 2019-03-04  Chris Dumez  <cdumez@apple.com>
</span><span class="cx"> 
</span><span class="cx">         [iOS] Improve our file picker
</span></span></pre></div>
<a id="trunkSourceWebCoreworkersserviceServiceWorkerContainercpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/workers/service/ServiceWorkerContainer.cpp (242371 => 242372)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/workers/service/ServiceWorkerContainer.cpp  2019-03-04 20:26:06 UTC (rev 242371)
+++ trunk/Source/WebCore/workers/service/ServiceWorkerContainer.cpp     2019-03-04 20:30:58 UTC (rev 242372)
</span><span class="lines">@@ -421,10 +421,6 @@
</span><span class="cx"> #endif
</span><span class="cx">     ASSERT_WITH_MESSAGE(job.hasPromise() || job.data().type == ServiceWorkerJobType::Update, "Only soft updates have no promise");
</span><span class="cx"> 
</span><del>-    auto guard = WTF::makeScopeExit([this, &job] {
-        destroyJob(job);
-    });
-
</del><span class="cx">     if (job.data().type == ServiceWorkerJobType::Register)
</span><span class="cx">         CONTAINER_RELEASE_LOG_IF_ALLOWED("jobResolvedWithRegistration: Registration job %" PRIu64 " succeeded", job.identifier().toUInt64());
</span><span class="cx">     else {
</span><span class="lines">@@ -432,32 +428,28 @@
</span><span class="cx">         CONTAINER_RELEASE_LOG_IF_ALLOWED("jobResolvedWithRegistration: Update job %" PRIu64 " succeeded", job.identifier().toUInt64());
</span><span class="cx">     }
</span><span class="cx"> 
</span><del>-    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);
-            });
-        };
-    }
</del><ins>+    auto guard = WTF::makeScopeExit([this, &job] {
+        destroyJob(job);
+    });
</ins><span class="cx"> 
</span><del>-    if (isStopped()) {
-        if (notifyWhenResolvedIfNeeded)
-            notifyWhenResolvedIfNeeded();
</del><ins>+    auto notifyIfExitEarly = WTF::makeScopeExit([this, &data, &shouldNotifyWhenResolved] {
+        if (shouldNotifyWhenResolved == ShouldNotifyWhenResolved::Yes)
+            notifyRegistrationIsSettled(data.key);
+    });
+
+    if (isStopped())
</ins><span class="cx">         return;
</span><del>-    }
</del><span class="cx"> 
</span><span class="cx">     auto promise = job.takePromise();
</span><del>-    if (!promise) {
-        if (notifyWhenResolvedIfNeeded)
-            notifyWhenResolvedIfNeeded();
</del><ins>+    if (!promise)
</ins><span class="cx">         return;
</span><del>-    }
</del><span class="cx"> 
</span><del>-    scriptExecutionContext()->postTask([this, protectedThis = makeRef(*this), promise = WTFMove(promise), jobIdentifier = job.identifier(), data = WTFMove(data), notifyWhenResolvedIfNeeded = WTFMove(notifyWhenResolvedIfNeeded)](ScriptExecutionContext& context) mutable {
</del><ins>+    notifyIfExitEarly.release();
+
+    scriptExecutionContext()->postTask([this, protectedThis = RefPtr<ServiceWorkerContainer>(this), promise = WTFMove(promise), jobIdentifier = job.identifier(), data = WTFMove(data), shouldNotifyWhenResolved](ScriptExecutionContext& context) mutable {
</ins><span class="cx">         if (isStopped() || !context.sessionID().isValid()) {
</span><del>-            if (notifyWhenResolvedIfNeeded)
-                notifyWhenResolvedIfNeeded();
</del><ins>+            if (shouldNotifyWhenResolved == ShouldNotifyWhenResolved::Yes)
+                notifyRegistrationIsSettled(data.key);
</ins><span class="cx">             return;
</span><span class="cx">         }
</span><span class="cx"> 
</span><span class="lines">@@ -465,9 +457,10 @@
</span><span class="cx"> 
</span><span class="cx">         CONTAINER_RELEASE_LOG_IF_ALLOWED("jobResolvedWithRegistration: Resolving promise for job %" PRIu64 ". Registration ID: %" PRIu64, jobIdentifier.toUInt64(), registration->identifier().toUInt64());
</span><span class="cx"> 
</span><del>-        if (notifyWhenResolvedIfNeeded) {
-            promise->whenSettled([notifyWhenResolvedIfNeeded = WTFMove(notifyWhenResolvedIfNeeded)] {
-                notifyWhenResolvedIfNeeded();
</del><ins>+        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));
</ins><span class="cx">             });
</span><span class="cx">         }
</span><span class="cx"> 
</span><span class="lines">@@ -475,6 +468,13 @@
</span><span class="cx">     });
</span><span class="cx"> }
</span><span class="cx"> 
</span><ins>+void ServiceWorkerContainer::notifyRegistrationIsSettled(const ServiceWorkerRegistrationKey& registrationKey)
+{
+    callOnMainThread([connection = m_swConnection, registrationKey = registrationKey.isolatedCopy()] {
+        connection->didResolveRegistrationPromise(registrationKey);
+    });
+}
+
</ins><span class="cx"> void ServiceWorkerContainer::jobResolvedWithUnregistrationResult(ServiceWorkerJob& job, bool unregistrationResult)
</span><span class="cx"> {
</span><span class="cx"> #ifndef NDEBUG
</span><span class="lines">@@ -639,9 +639,13 @@
</span><span class="cx">     m_readyPromise = nullptr;
</span><span class="cx">     auto jobMap = WTFMove(m_jobMap);
</span><span class="cx">     for (auto& ongoingJob : jobMap.values()) {
</span><del>-        notifyFailedFetchingScript(*ongoingJob.job.get(), ResourceError { errorDomainWebKitInternal, 0, ongoingJob.job->data().scriptURL, "Job cancelled"_s, ResourceError::Type::Cancellation });
-        ongoingJob.job->cancelPendingLoad();
</del><ins>+        if (ongoingJob.job->cancelPendingLoad())
+            notifyFailedFetchingScript(*ongoingJob.job.get(), ResourceError { errorDomainWebKitInternal, 0, ongoingJob.job->data().scriptURL, "Job cancelled"_s, ResourceError::Type::Cancellation });
</ins><span class="cx">     }
</span><ins>+
+    auto registrationMap = WTFMove(m_ongoingSettledRegistrations);
+    for (auto& registration : registrationMap.values())
+        notifyRegistrationIsSettled(registration);
</ins><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> DocumentOrWorkerIdentifier ServiceWorkerContainer::contextIdentifier()
</span></span></pre></div>
<a id="trunkSourceWebCoreworkersserviceServiceWorkerContainerh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/workers/service/ServiceWorkerContainer.h (242371 => 242372)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/workers/service/ServiceWorkerContainer.h    2019-03-04 20:26:06 UTC (rev 242371)
+++ trunk/Source/WebCore/workers/service/ServiceWorkerContainer.h       2019-03-04 20:30:58 UTC (rev 242372)
</span><span class="lines">@@ -114,6 +114,8 @@
</span><span class="cx">     void derefEventTarget() final;
</span><span class="cx">     void stop() final;
</span><span class="cx"> 
</span><ins>+    void notifyRegistrationIsSettled(const ServiceWorkerRegistrationKey&);
+
</ins><span class="cx">     std::unique_ptr<ReadyPromise> m_readyPromise;
</span><span class="cx"> 
</span><span class="cx">     NavigatorBase& m_navigator;
</span><span class="lines">@@ -145,6 +147,10 @@
</span><span class="cx"> 
</span><span class="cx">     uint64_t m_lastPendingPromiseIdentifier { 0 };
</span><span class="cx">     HashMap<uint64_t, std::unique_ptr<PendingPromise>> m_pendingPromises;
</span><ins>+
+    uint64_t m_lastOngoingSettledRegistrationIdentifier { 0 };
+    HashMap<uint64_t, ServiceWorkerRegistrationKey> m_ongoingSettledRegistrations;
+
</ins><span class="cx"> };
</span><span class="cx"> 
</span><span class="cx"> } // namespace WebCore
</span></span></pre></div>
<a id="trunkSourceWebCoreworkersserviceServiceWorkerJobcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/workers/service/ServiceWorkerJob.cpp (242371 => 242372)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/workers/service/ServiceWorkerJob.cpp        2019-03-04 20:26:06 UTC (rev 242371)
+++ trunk/Source/WebCore/workers/service/ServiceWorkerJob.cpp   2019-03-04 20:30:58 UTC (rev 242372)
</span><span class="lines">@@ -166,12 +166,14 @@
</span><span class="cx">     m_client.jobFailedLoadingScript(*this, error, Exception { error.isAccessControl() ? SecurityError : TypeError, "Script load failed"_s });
</span><span class="cx"> }
</span><span class="cx"> 
</span><del>-void ServiceWorkerJob::cancelPendingLoad()
</del><ins>+bool ServiceWorkerJob::cancelPendingLoad()
</ins><span class="cx"> {
</span><span class="cx">     if (!m_scriptLoader)
</span><del>-        return;
</del><ins>+        return false;
+
</ins><span class="cx">     m_scriptLoader->cancel();
</span><span class="cx">     m_scriptLoader = nullptr;
</span><ins>+    return true;
</ins><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> } // namespace WebCore
</span></span></pre></div>
<a id="trunkSourceWebCoreworkersserviceServiceWorkerJobh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/workers/service/ServiceWorkerJob.h (242371 => 242372)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/workers/service/ServiceWorkerJob.h  2019-03-04 20:26:06 UTC (rev 242371)
+++ trunk/Source/WebCore/workers/service/ServiceWorkerJob.h     2019-03-04 20:30:58 UTC (rev 242372)
</span><span class="lines">@@ -69,7 +69,7 @@
</span><span class="cx"> 
</span><span class="cx">     const DocumentOrWorkerIdentifier& contextIdentifier() { return m_contextIdentifier; }
</span><span class="cx"> 
</span><del>-    void cancelPendingLoad();
</del><ins>+    bool cancelPendingLoad();
</ins><span class="cx"> 
</span><span class="cx"> private:
</span><span class="cx">     // WorkerScriptLoaderClient
</span></span></pre>
</div>
</div>

</body>
</html>