Layout Test imported/w3c/web-platform-tests/service-workers/service-worker/register...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 5 Feb 2018 18:12:07 +0000 (18:12 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 5 Feb 2018 18:12:07 +0000 (18:12 +0000)
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:

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

LayoutTests/ChangeLog
LayoutTests/platform/mac-wk2/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/workers/service/ServiceWorkerContainer.cpp
Source/WebCore/workers/service/ServiceWorkerJobData.cpp
Source/WebCore/workers/service/ServiceWorkerJobData.h
Source/WebCore/workers/service/server/SWServer.cpp
Source/WebCore/workers/service/server/SWServer.h
Source/WebCore/workers/service/server/SWServerJobQueue.cpp
Source/WebCore/workers/service/server/SWServerJobQueue.h
Source/WebCore/workers/service/server/SWServerRegistration.cpp

index a370503..0f097f4 100644 (file)
@@ -1,3 +1,15 @@
+2018-02-05  Chris Dumez  <cdumez@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  <dabates@apple.com>
 
         Disallow evaluating JavaScript from NPP_Destroy() in WebKit
index 42143f2..f0ebc91 100644 (file)
@@ -853,8 +853,6 @@ webkit.org/b/179194 [ Debug ] imported/w3c/web-platform-tests/service-workers/se
 
 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 ]
index 67bb415..a625ff6 100644 (file)
@@ -1,3 +1,51 @@
+2018-02-05  Chris Dumez  <cdumez@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  <antti@apple.com>
 
         Crash on sfgate.com because mismatching link preload types
index cfc41dd..657536b 100644 (file)
@@ -126,7 +126,7 @@ void ServiceWorkerContainer::addRegistration(const String& relativeScriptURL, co
         return;
     }
 
-    ServiceWorkerJobData jobData(ensureSWClientConnection().serverConnectionIdentifier());
+    ServiceWorkerJobData jobData(ensureSWClientConnection().serverConnectionIdentifier(), contextIdentifier());
 
     jobData.scriptURL = context->completeURL(relativeScriptURL);
     if (!jobData.scriptURL.isValid()) {
@@ -191,7 +191,7 @@ void ServiceWorkerContainer::removeRegistration(const URL& scopeURL, Ref<Deferre
         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 @@ void ServiceWorkerContainer::updateRegistration(const URL& scopeURL, const URL&
         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;
index 6894cb0..29c80e1 100644 (file)
@@ -35,9 +35,14 @@ ServiceWorkerJobData::ServiceWorkerJobData(const Identifier& identifier)
 {
 }
 
-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 @@ ServiceWorkerRegistrationKey ServiceWorkerJobData::registrationKey() const
 ServiceWorkerJobData ServiceWorkerJobData::isolatedCopy() const
 {
     ServiceWorkerJobData result { identifier() };
+    result.sourceContext = sourceContext;
     result.type = type;
 
     result.scriptURL = scriptURL.isolatedCopy();
index be13b8b..0870bb6 100644 (file)
@@ -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 @@ namespace WebCore {
 
 struct ServiceWorkerJobData {
     using Identifier = ServiceWorkerJobDataIdentifier;
-    explicit ServiceWorkerJobData(SWServerConnectionIdentifier);
+    ServiceWorkerJobData(SWServerConnectionIdentifier, const DocumentOrWorkerIdentifier& sourceContext);
     ServiceWorkerJobData(const ServiceWorkerJobData&) = default;
     ServiceWorkerJobData() = default;
 
@@ -49,6 +50,7 @@ struct ServiceWorkerJobData {
     URL clientCreationURL;
     SecurityOriginData topOrigin;
     URL scopeURL;
+    ServiceWorkerOrClientIdentifier sourceContext;
     ServiceWorkerJobType type;
 
     ServiceWorkerRegistrationOptions registrationOptions;
@@ -69,7 +71,7 @@ private:
 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 @@ std::optional<ServiceWorkerJobData> ServiceWorkerJobData::decode(Decoder& decode
 
     if (!decoder.decode(jobData.scopeURL))
         return std::nullopt;
+    if (!decoder.decode(jobData.sourceContext))
+        return std::nullopt;
     if (!decoder.decodeEnum(jobData.type))
         return std::nullopt;
 
index f83856a..4b5b987 100644 (file)
@@ -327,10 +327,9 @@ void SWServer::startScriptFetch(const ServiceWorkerJobData& jobData, FetchOption
 {
     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 @@ void SWServer::scriptContextFailedToStart(const std::optional<ServiceWorkerJobDa
 
     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,8 +366,21 @@ void SWServer::scriptContextStarted(const std::optional<ServiceWorkerJobDataIden
     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)
@@ -508,6 +525,13 @@ void SWServer::installContextData(const ServiceWorkerContextData& data)
 {
     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 @@ void SWServer::workerContextTerminated(SWServerWorker& worker)
     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 @@ void SWServer::unregisterConnection(Connection& connection)
 
     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)
index 9d83aad..9ef2edd 100644 (file)
@@ -189,6 +189,8 @@ private:
     void addClientServiceWorkerRegistration(Connection&, ServiceWorkerRegistrationIdentifier);
     void removeClientServiceWorkerRegistration(Connection&, ServiceWorkerRegistrationIdentifier);
 
+    void terminatePreinstallationWorker(SWServerWorker&);
+
     WEBCORE_EXPORT SWServerRegistration* doRegistrationMatching(const SecurityOriginData& topOrigin, const URL& clientURL);
     bool runServiceWorker(ServiceWorkerIdentifier);
 
index 7a8c82f..dc36e5f 100644 (file)
@@ -365,6 +365,41 @@ void SWServerJobQueue::finishCurrentJob()
         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)
index 810915d..75a0cbd 100644 (file)
@@ -53,6 +53,10 @@ public:
     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();
@@ -66,7 +70,7 @@ private:
 
     void install(SWServerRegistration&, ServiceWorkerIdentifier);
 
-    bool isCurrentlyProcessingJob(const ServiceWorkerJobDataIdentifier&) const;
+    void removeAllJobsMatching(const WTF::Function<bool(ServiceWorkerJobData&)>&);
 
     Deque<ServiceWorkerJobData> m_jobQueue;
 
index 136ae20..c954344 100644 (file)
@@ -72,7 +72,6 @@ SWServerWorker* SWServerRegistration::getNewestWorker()
 
 void SWServerRegistration::setPreInstallationWorker(SWServerWorker* worker)
 {
-    ASSERT(!m_preInstallationWorker || !worker);
     m_preInstallationWorker = worker;
 }