Title: [228614] releases/WebKitGTK/webkit-2.20
Revision
228614
Author
carlo...@webkit.org
Date
2018-02-19 00:51:08 -0800 (Mon, 19 Feb 2018)

Log Message

Merge r228101 - Layout Test imported/w3c/web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https.html is flaky
https://bugs.webkit.org/show_bug.cgi?id=181166
<rdar://problem/37169508>

Reviewed by Youenn Fablet.

Source/WebCore:

I found out that this test was flakily timing out because our jobQueues would sometimes get stuck
when their current job's connection or service worker (when scheduled by a service worker) would
go away before the job is complete.

This patch makes our job queues operation more robust by:
1. Cancelling all jobs from a given connection when a SWServerConnection goes away
2. Cancelling all jobs from a given service worker when a service worker gets terminated

We also make sure service workers created by a job get properly terminated when a job
is canceled to avoid leaving service workers in limbo.

No new tests, unskipped existing flaky test.

* workers/service/ServiceWorkerContainer.cpp:
(WebCore::ServiceWorkerContainer::addRegistration):
(WebCore::ServiceWorkerContainer::removeRegistration):
(WebCore::ServiceWorkerContainer::updateRegistration):
* workers/service/ServiceWorkerJobData.cpp:
(WebCore::ServiceWorkerJobData::ServiceWorkerJobData):
(WebCore::ServiceWorkerJobData::isolatedCopy const):
* workers/service/ServiceWorkerJobData.h:
(WebCore::ServiceWorkerJobData::encode const):
(WebCore::ServiceWorkerJobData::decode):
* workers/service/server/SWServer.cpp:
(WebCore::SWServer::startScriptFetch):
(WebCore::SWServer::scriptContextFailedToStart):
(WebCore::SWServer::scriptContextStarted):
(WebCore::SWServer::terminatePreinstallationWorker):
(WebCore::SWServer::installContextData):
(WebCore::SWServer::workerContextTerminated):
(WebCore::SWServer::unregisterConnection):
* workers/service/server/SWServer.h:
* workers/service/server/SWServerJobQueue.cpp:
(WebCore::SWServerJobQueue::removeAllJobsMatching):
(WebCore::SWServerJobQueue::cancelJobsFromConnection):
(WebCore::SWServerJobQueue::cancelJobsFromServiceWorker):
* workers/service/server/SWServerJobQueue.h:
* workers/service/server/SWServerRegistration.cpp:
(WebCore::SWServerRegistration::setPreInstallationWorker):

LayoutTests:

Unskip test that is no longer flaky.

* platform/mac-wk2/TestExpectations:

Modified Paths

Diff

Modified: releases/WebKitGTK/webkit-2.20/LayoutTests/ChangeLog (228613 => 228614)


--- releases/WebKitGTK/webkit-2.20/LayoutTests/ChangeLog	2018-02-19 08:28:13 UTC (rev 228613)
+++ releases/WebKitGTK/webkit-2.20/LayoutTests/ChangeLog	2018-02-19 08:51:08 UTC (rev 228614)
@@ -1,3 +1,15 @@
+2018-02-05  Chris Dumez  <cdu...@apple.com>
+
+        Layout Test imported/w3c/web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https.html is flaky
+        https://bugs.webkit.org/show_bug.cgi?id=181166
+        <rdar://problem/37169508>
+
+        Reviewed by Youenn Fablet.
+
+        Unskip test that is no longer flaky.
+
+        * platform/mac-wk2/TestExpectations:
+
 2018-02-05  Daniel Bates  <daba...@apple.com>
 
         Disallow evaluating _javascript_ from NPP_Destroy() in WebKit

Modified: releases/WebKitGTK/webkit-2.20/LayoutTests/platform/mac-wk2/TestExpectations (228613 => 228614)


--- releases/WebKitGTK/webkit-2.20/LayoutTests/platform/mac-wk2/TestExpectations	2018-02-19 08:28:13 UTC (rev 228613)
+++ releases/WebKitGTK/webkit-2.20/LayoutTests/platform/mac-wk2/TestExpectations	2018-02-19 08:51:08 UTC (rev 228614)
@@ -853,8 +853,6 @@
 
 webkit.org/b/180982 [ Debug ] imported/w3c/web-platform-tests/service-workers/service-worker/registration-updateviacache.https.html [ Slow ]
 
-webkit.org/b/181166 imported/w3c/web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https.html [ Pass Failure ]
-
 webkit.org/b/181167 imported/w3c/web-platform-tests/service-workers/service-worker/ServiceWorkerGlobalScope/update.https.html [ Pass Failure ]
 
 webkit.org/b/181069 fast/mediastream/MediaStream-MediaElement-setObject-null.html [ Pass Failure ]

Modified: releases/WebKitGTK/webkit-2.20/Source/WebCore/ChangeLog (228613 => 228614)


--- releases/WebKitGTK/webkit-2.20/Source/WebCore/ChangeLog	2018-02-19 08:28:13 UTC (rev 228613)
+++ releases/WebKitGTK/webkit-2.20/Source/WebCore/ChangeLog	2018-02-19 08:51:08 UTC (rev 228614)
@@ -1,3 +1,51 @@
+2018-02-05  Chris Dumez  <cdu...@apple.com>
+
+        Layout Test imported/w3c/web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https.html is flaky
+        https://bugs.webkit.org/show_bug.cgi?id=181166
+        <rdar://problem/37169508>
+
+        Reviewed by Youenn Fablet.
+
+        I found out that this test was flakily timing out because our jobQueues would sometimes get stuck
+        when their current job's connection or service worker (when scheduled by a service worker) would
+        go away before the job is complete.
+
+        This patch makes our job queues operation more robust by:
+        1. Cancelling all jobs from a given connection when a SWServerConnection goes away
+        2. Cancelling all jobs from a given service worker when a service worker gets terminated
+
+        We also make sure service workers created by a job get properly terminated when a job
+        is canceled to avoid leaving service workers in limbo.
+
+        No new tests, unskipped existing flaky test.
+
+        * workers/service/ServiceWorkerContainer.cpp:
+        (WebCore::ServiceWorkerContainer::addRegistration):
+        (WebCore::ServiceWorkerContainer::removeRegistration):
+        (WebCore::ServiceWorkerContainer::updateRegistration):
+        * workers/service/ServiceWorkerJobData.cpp:
+        (WebCore::ServiceWorkerJobData::ServiceWorkerJobData):
+        (WebCore::ServiceWorkerJobData::isolatedCopy const):
+        * workers/service/ServiceWorkerJobData.h:
+        (WebCore::ServiceWorkerJobData::encode const):
+        (WebCore::ServiceWorkerJobData::decode):
+        * workers/service/server/SWServer.cpp:
+        (WebCore::SWServer::startScriptFetch):
+        (WebCore::SWServer::scriptContextFailedToStart):
+        (WebCore::SWServer::scriptContextStarted):
+        (WebCore::SWServer::terminatePreinstallationWorker):
+        (WebCore::SWServer::installContextData):
+        (WebCore::SWServer::workerContextTerminated):
+        (WebCore::SWServer::unregisterConnection):
+        * workers/service/server/SWServer.h:
+        * workers/service/server/SWServerJobQueue.cpp:
+        (WebCore::SWServerJobQueue::removeAllJobsMatching):
+        (WebCore::SWServerJobQueue::cancelJobsFromConnection):
+        (WebCore::SWServerJobQueue::cancelJobsFromServiceWorker):
+        * workers/service/server/SWServerJobQueue.h:
+        * workers/service/server/SWServerRegistration.cpp:
+        (WebCore::SWServerRegistration::setPreInstallationWorker):
+
 2018-02-05  Antti Koivisto  <an...@apple.com>
 
         Crash on sfgate.com because mismatching link preload types

Modified: releases/WebKitGTK/webkit-2.20/Source/WebCore/workers/service/ServiceWorkerContainer.cpp (228613 => 228614)


--- releases/WebKitGTK/webkit-2.20/Source/WebCore/workers/service/ServiceWorkerContainer.cpp	2018-02-19 08:28:13 UTC (rev 228613)
+++ releases/WebKitGTK/webkit-2.20/Source/WebCore/workers/service/ServiceWorkerContainer.cpp	2018-02-19 08:51:08 UTC (rev 228614)
@@ -126,7 +126,7 @@
         return;
     }
 
-    ServiceWorkerJobData jobData(ensureSWClientConnection().serverConnectionIdentifier());
+    ServiceWorkerJobData jobData(ensureSWClientConnection().serverConnectionIdentifier(), contextIdentifier());
 
     jobData.scriptURL = context->completeURL(relativeScriptURL);
     if (!jobData.scriptURL.isValid()) {
@@ -191,7 +191,7 @@
         return;
     }
 
-    ServiceWorkerJobData jobData(m_swConnection->serverConnectionIdentifier());
+    ServiceWorkerJobData jobData(m_swConnection->serverConnectionIdentifier(), contextIdentifier());
     jobData.clientCreationURL = context->url();
     jobData.topOrigin = SecurityOriginData::fromSecurityOrigin(context->topOrigin());
     jobData.type = ServiceWorkerJobType::Unregister;
@@ -216,7 +216,7 @@
         return;
     }
 
-    ServiceWorkerJobData jobData(m_swConnection->serverConnectionIdentifier());
+    ServiceWorkerJobData jobData(m_swConnection->serverConnectionIdentifier(), contextIdentifier());
     jobData.clientCreationURL = context.url();
     jobData.topOrigin = SecurityOriginData::fromSecurityOrigin(context.topOrigin());
     jobData.type = ServiceWorkerJobType::Update;

Modified: releases/WebKitGTK/webkit-2.20/Source/WebCore/workers/service/ServiceWorkerJobData.cpp (228613 => 228614)


--- releases/WebKitGTK/webkit-2.20/Source/WebCore/workers/service/ServiceWorkerJobData.cpp	2018-02-19 08:28:13 UTC (rev 228613)
+++ releases/WebKitGTK/webkit-2.20/Source/WebCore/workers/service/ServiceWorkerJobData.cpp	2018-02-19 08:51:08 UTC (rev 228614)
@@ -35,9 +35,14 @@
 {
 }
 
-ServiceWorkerJobData::ServiceWorkerJobData(SWServerConnectionIdentifier connectionIdentifier)
+ServiceWorkerJobData::ServiceWorkerJobData(SWServerConnectionIdentifier connectionIdentifier, const DocumentOrWorkerIdentifier& localSourceContext)
     : m_identifier { connectionIdentifier, generateThreadSafeObjectIdentifier<ServiceWorkerJobIdentifierType>() }
 {
+    WTF::switchOn(localSourceContext, [&](DocumentIdentifier documentIdentifier) {
+        sourceContext = ServiceWorkerClientIdentifier { connectionIdentifier, documentIdentifier };
+    }, [&](ServiceWorkerIdentifier serviceWorkerIdentifier) {
+        sourceContext = serviceWorkerIdentifier;
+    });
 }
 
 ServiceWorkerRegistrationKey ServiceWorkerJobData::registrationKey() const
@@ -50,6 +55,7 @@
 ServiceWorkerJobData ServiceWorkerJobData::isolatedCopy() const
 {
     ServiceWorkerJobData result { identifier() };
+    result.sourceContext = sourceContext;
     result.type = type;
 
     result.scriptURL = scriptURL.isolatedCopy();

Modified: releases/WebKitGTK/webkit-2.20/Source/WebCore/workers/service/ServiceWorkerJobData.h (228613 => 228614)


--- releases/WebKitGTK/webkit-2.20/Source/WebCore/workers/service/ServiceWorkerJobData.h	2018-02-19 08:28:13 UTC (rev 228613)
+++ releases/WebKitGTK/webkit-2.20/Source/WebCore/workers/service/ServiceWorkerJobData.h	2018-02-19 08:51:08 UTC (rev 228614)
@@ -28,6 +28,7 @@
 #if ENABLE(SERVICE_WORKER)
 
 #include "SecurityOriginData.h"
+#include "ServiceWorkerClientIdentifier.h"
 #include "ServiceWorkerJobDataIdentifier.h"
 #include "ServiceWorkerJobType.h"
 #include "ServiceWorkerRegistrationKey.h"
@@ -39,7 +40,7 @@
 
 struct ServiceWorkerJobData {
     using Identifier = ServiceWorkerJobDataIdentifier;
-    explicit ServiceWorkerJobData(SWServerConnectionIdentifier);
+    ServiceWorkerJobData(SWServerConnectionIdentifier, const DocumentOrWorkerIdentifier& sourceContext);
     ServiceWorkerJobData(const ServiceWorkerJobData&) = default;
     ServiceWorkerJobData() = default;
 
@@ -49,6 +50,7 @@
     URL clientCreationURL;
     SecurityOriginData topOrigin;
     URL scopeURL;
+    ServiceWorkerOrClientIdentifier sourceContext;
     ServiceWorkerJobType type;
 
     ServiceWorkerRegistrationOptions registrationOptions;
@@ -69,7 +71,7 @@
 template<class Encoder>
 void ServiceWorkerJobData::encode(Encoder& encoder) const
 {
-    encoder << identifier() << scriptURL << clientCreationURL << topOrigin << scopeURL;
+    encoder << identifier() << scriptURL << clientCreationURL << topOrigin << scopeURL << sourceContext;
     encoder.encodeEnum(type);
     switch (type) {
     case ServiceWorkerJobType::Register:
@@ -104,6 +106,8 @@
 
     if (!decoder.decode(jobData.scopeURL))
         return std::nullopt;
+    if (!decoder.decode(jobData.sourceContext))
+        return std::nullopt;
     if (!decoder.decodeEnum(jobData.type))
         return std::nullopt;
 

Modified: releases/WebKitGTK/webkit-2.20/Source/WebCore/workers/service/server/SWServer.cpp (228613 => 228614)


--- releases/WebKitGTK/webkit-2.20/Source/WebCore/workers/service/server/SWServer.cpp	2018-02-19 08:28:13 UTC (rev 228613)
+++ releases/WebKitGTK/webkit-2.20/Source/WebCore/workers/service/server/SWServer.cpp	2018-02-19 08:51:08 UTC (rev 228614)
@@ -327,10 +327,9 @@
 {
     LOG(ServiceWorker, "Server issuing startScriptFetch for current job %s in client", jobData.identifier().loggingString().utf8().data());
     auto* connection = m_connections.get(jobData.connectionIdentifier());
-    if (!connection)
-        return;
-
-    connection->startScriptFetchInClient(jobData.identifier().jobIdentifier, jobData.registrationKey(), cachePolicy);
+    ASSERT_WITH_MESSAGE(connection, "If the connection was lost, this job should have been cancelled");
+    if (connection)
+        connection->startScriptFetchInClient(jobData.identifier().jobIdentifier, jobData.registrationKey(), cachePolicy);
 }
 
 void SWServer::scriptFetchFinished(Connection& connection, const ServiceWorkerFetchResult& result)
@@ -353,8 +352,13 @@
 
     RELEASE_LOG_ERROR(ServiceWorker, "%p - SWServer::scriptContextFailedToStart: Failed to start SW for job %s, error: %s", this, jobDataIdentifier->loggingString().utf8().data(), message.utf8().data());
 
-    if (auto* jobQueue = m_jobQueues.get(worker.registrationKey()))
-        jobQueue->scriptContextFailedToStart(*jobDataIdentifier, worker.identifier(), message);
+    auto* jobQueue = m_jobQueues.get(worker.registrationKey());
+    if (!jobQueue || !jobQueue->isCurrentlyProcessingJob(*jobDataIdentifier)) {
+        // The job which started this worker has been canceled, terminate this worker.
+        terminatePreinstallationWorker(worker);
+        return;
+    }
+    jobQueue->scriptContextFailedToStart(*jobDataIdentifier, worker.identifier(), message);
 }
 
 void SWServer::scriptContextStarted(const std::optional<ServiceWorkerJobDataIdentifier>& jobDataIdentifier, SWServerWorker& worker)
@@ -362,10 +366,23 @@
     if (!jobDataIdentifier)
         return;
 
-    if (auto* jobQueue = m_jobQueues.get(worker.registrationKey()))
-        jobQueue->scriptContextStarted(*jobDataIdentifier, worker.identifier());
+    auto* jobQueue = m_jobQueues.get(worker.registrationKey());
+    if (!jobQueue || !jobQueue->isCurrentlyProcessingJob(*jobDataIdentifier)) {
+        // The job which started this worker has been canceled, terminate this worker.
+        terminatePreinstallationWorker(worker);
+        return;
+    }
+    jobQueue->scriptContextStarted(*jobDataIdentifier, worker.identifier());
 }
 
+void SWServer::terminatePreinstallationWorker(SWServerWorker& worker)
+{
+    worker.terminate();
+    auto* registration = getRegistration(worker.registrationKey());
+    if (registration && registration->preInstallationWorker() == &worker)
+        registration->setPreInstallationWorker(nullptr);
+}
+
 void SWServer::didFinishInstall(const std::optional<ServiceWorkerJobDataIdentifier>& jobDataIdentifier, SWServerWorker& worker, bool wasSuccessful)
 {
     if (!jobDataIdentifier)
@@ -508,6 +525,13 @@
 {
     ASSERT_WITH_MESSAGE(!data.loadedFromDisk, "Workers we just read from disk should only be launched as needed");
 
+    if (data.jobDataIdentifier) {
+        // Abort if the job that scheduled this has been cancelled.
+        auto* jobQueue = m_jobQueues.get(data.registration.key);
+        if (!jobQueue || !jobQueue->isCurrentlyProcessingJob(*data.jobDataIdentifier))
+            return;
+    }
+
     m_registrationStore.updateRegistration(data);
 
     auto* connection = SWServerToContextConnection::globalServerToContextConnection();
@@ -631,6 +655,9 @@
     worker.setState(SWServerWorker::State::NotRunning);
     worker.setContextConnectionIdentifier(std::nullopt);
 
+    if (auto* jobQueue = m_jobQueues.get(worker.registrationKey()))
+        jobQueue->cancelJobsFromServiceWorker(worker.identifier());
+
     // At this point if no registrations are referencing the worker then it will be destroyed,
     // removing itself from the m_workersByID map.
     auto result = m_runningOrTerminatingWorkers.take(worker.identifier());
@@ -684,6 +711,9 @@
 
     for (auto& registration : m_registrations.values())
         registration->unregisterServerConnection(connection.identifier());
+
+    for (auto& jobQueue : m_jobQueues.values())
+        jobQueue->cancelJobsFromConnection(connection.identifier());
 }
 
 SWServerRegistration* SWServer::doRegistrationMatching(const SecurityOriginData& topOrigin, const URL& clientURL)

Modified: releases/WebKitGTK/webkit-2.20/Source/WebCore/workers/service/server/SWServer.h (228613 => 228614)


--- releases/WebKitGTK/webkit-2.20/Source/WebCore/workers/service/server/SWServer.h	2018-02-19 08:28:13 UTC (rev 228613)
+++ releases/WebKitGTK/webkit-2.20/Source/WebCore/workers/service/server/SWServer.h	2018-02-19 08:51:08 UTC (rev 228614)
@@ -189,6 +189,8 @@
     void addClientServiceWorkerRegistration(Connection&, ServiceWorkerRegistrationIdentifier);
     void removeClientServiceWorkerRegistration(Connection&, ServiceWorkerRegistrationIdentifier);
 
+    void terminatePreinstallationWorker(SWServerWorker&);
+
     WEBCORE_EXPORT SWServerRegistration* doRegistrationMatching(const SecurityOriginData& topOrigin, const URL& clientURL);
     bool runServiceWorker(ServiceWorkerIdentifier);
 

Modified: releases/WebKitGTK/webkit-2.20/Source/WebCore/workers/service/server/SWServerJobQueue.cpp (228613 => 228614)


--- releases/WebKitGTK/webkit-2.20/Source/WebCore/workers/service/server/SWServerJobQueue.cpp	2018-02-19 08:28:13 UTC (rev 228613)
+++ releases/WebKitGTK/webkit-2.20/Source/WebCore/workers/service/server/SWServerJobQueue.cpp	2018-02-19 08:51:08 UTC (rev 228614)
@@ -365,6 +365,41 @@
         runNextJob();
 }
 
+void SWServerJobQueue::removeAllJobsMatching(const WTF::Function<bool(ServiceWorkerJobData&)>& matches)
+{
+    bool isFirst = true;
+    bool didRemoveFirstJob = false;
+    m_jobQueue.removeAllMatching([&](auto& job) {
+        bool shouldRemove = matches(job);
+        if (isFirst) {
+            isFirst = false;
+            if (shouldRemove)
+                didRemoveFirstJob = true;
+        }
+        return shouldRemove;
+    });
+
+    if (m_jobTimer.isActive()) {
+        if (m_jobQueue.isEmpty())
+            m_jobTimer.stop();
+    } else if (didRemoveFirstJob && !m_jobQueue.isEmpty())
+        runNextJob();
+}
+
+void SWServerJobQueue::cancelJobsFromConnection(SWServerConnectionIdentifier connectionIdentifier)
+{
+    removeAllJobsMatching([connectionIdentifier](auto& job) {
+        return job.identifier().connectionIdentifier == connectionIdentifier;
+    });
+}
+
+void SWServerJobQueue::cancelJobsFromServiceWorker(ServiceWorkerIdentifier serviceWorkerIdentifier)
+{
+    removeAllJobsMatching([serviceWorkerIdentifier](auto& job) {
+        return WTF::holds_alternative<ServiceWorkerIdentifier>(job.sourceContext) && WTF::get<ServiceWorkerIdentifier>(job.sourceContext) == serviceWorkerIdentifier;
+    });
+}
+
 } // namespace WebCore
 
 #endif // ENABLE(SERVICE_WORKER)

Modified: releases/WebKitGTK/webkit-2.20/Source/WebCore/workers/service/server/SWServerJobQueue.h (228613 => 228614)


--- releases/WebKitGTK/webkit-2.20/Source/WebCore/workers/service/server/SWServerJobQueue.h	2018-02-19 08:28:13 UTC (rev 228613)
+++ releases/WebKitGTK/webkit-2.20/Source/WebCore/workers/service/server/SWServerJobQueue.h	2018-02-19 08:51:08 UTC (rev 228614)
@@ -53,7 +53,11 @@
     void scriptContextStarted(const ServiceWorkerJobDataIdentifier&, ServiceWorkerIdentifier);
     void didFinishInstall(const ServiceWorkerJobDataIdentifier&, ServiceWorkerIdentifier, bool wasSuccessful);
     void didResolveRegistrationPromise();
+    void cancelJobsFromConnection(SWServerConnectionIdentifier);
+    void cancelJobsFromServiceWorker(ServiceWorkerIdentifier);
 
+    bool isCurrentlyProcessingJob(const ServiceWorkerJobDataIdentifier&) const;
+
 private:
     void jobTimerFired();
     void runNextJobSynchronously();
@@ -66,7 +70,7 @@
 
     void install(SWServerRegistration&, ServiceWorkerIdentifier);
 
-    bool isCurrentlyProcessingJob(const ServiceWorkerJobDataIdentifier&) const;
+    void removeAllJobsMatching(const WTF::Function<bool(ServiceWorkerJobData&)>&);
 
     Deque<ServiceWorkerJobData> m_jobQueue;
 

Modified: releases/WebKitGTK/webkit-2.20/Source/WebCore/workers/service/server/SWServerRegistration.cpp (228613 => 228614)


--- releases/WebKitGTK/webkit-2.20/Source/WebCore/workers/service/server/SWServerRegistration.cpp	2018-02-19 08:28:13 UTC (rev 228613)
+++ releases/WebKitGTK/webkit-2.20/Source/WebCore/workers/service/server/SWServerRegistration.cpp	2018-02-19 08:51:08 UTC (rev 228614)
@@ -72,7 +72,6 @@
 
 void SWServerRegistration::setPreInstallationWorker(SWServerWorker* worker)
 {
-    ASSERT(!m_preInstallationWorker || !worker);
     m_preInstallationWorker = worker;
 }
 
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to