Diff
Modified: trunk/LayoutTests/ChangeLog (228100 => 228101)
--- trunk/LayoutTests/ChangeLog 2018-02-05 18:03:58 UTC (rev 228100)
+++ trunk/LayoutTests/ChangeLog 2018-02-05 18:12:07 UTC (rev 228101)
@@ -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: trunk/LayoutTests/platform/mac-wk2/TestExpectations (228100 => 228101)
--- trunk/LayoutTests/platform/mac-wk2/TestExpectations 2018-02-05 18:03:58 UTC (rev 228100)
+++ trunk/LayoutTests/platform/mac-wk2/TestExpectations 2018-02-05 18:12:07 UTC (rev 228101)
@@ -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: trunk/Source/WebCore/ChangeLog (228100 => 228101)
--- trunk/Source/WebCore/ChangeLog 2018-02-05 18:03:58 UTC (rev 228100)
+++ trunk/Source/WebCore/ChangeLog 2018-02-05 18:12:07 UTC (rev 228101)
@@ -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: trunk/Source/WebCore/workers/service/ServiceWorkerContainer.cpp (228100 => 228101)
--- trunk/Source/WebCore/workers/service/ServiceWorkerContainer.cpp 2018-02-05 18:03:58 UTC (rev 228100)
+++ trunk/Source/WebCore/workers/service/ServiceWorkerContainer.cpp 2018-02-05 18:12:07 UTC (rev 228101)
@@ -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: trunk/Source/WebCore/workers/service/ServiceWorkerJobData.cpp (228100 => 228101)
--- trunk/Source/WebCore/workers/service/ServiceWorkerJobData.cpp 2018-02-05 18:03:58 UTC (rev 228100)
+++ trunk/Source/WebCore/workers/service/ServiceWorkerJobData.cpp 2018-02-05 18:12:07 UTC (rev 228101)
@@ -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: trunk/Source/WebCore/workers/service/ServiceWorkerJobData.h (228100 => 228101)
--- trunk/Source/WebCore/workers/service/ServiceWorkerJobData.h 2018-02-05 18:03:58 UTC (rev 228100)
+++ trunk/Source/WebCore/workers/service/ServiceWorkerJobData.h 2018-02-05 18:12:07 UTC (rev 228101)
@@ -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: trunk/Source/WebCore/workers/service/server/SWServer.cpp (228100 => 228101)
--- trunk/Source/WebCore/workers/service/server/SWServer.cpp 2018-02-05 18:03:58 UTC (rev 228100)
+++ trunk/Source/WebCore/workers/service/server/SWServer.cpp 2018-02-05 18:12:07 UTC (rev 228101)
@@ -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: trunk/Source/WebCore/workers/service/server/SWServer.h (228100 => 228101)
--- trunk/Source/WebCore/workers/service/server/SWServer.h 2018-02-05 18:03:58 UTC (rev 228100)
+++ trunk/Source/WebCore/workers/service/server/SWServer.h 2018-02-05 18:12:07 UTC (rev 228101)
@@ -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: trunk/Source/WebCore/workers/service/server/SWServerJobQueue.cpp (228100 => 228101)
--- trunk/Source/WebCore/workers/service/server/SWServerJobQueue.cpp 2018-02-05 18:03:58 UTC (rev 228100)
+++ trunk/Source/WebCore/workers/service/server/SWServerJobQueue.cpp 2018-02-05 18:12:07 UTC (rev 228101)
@@ -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: trunk/Source/WebCore/workers/service/server/SWServerJobQueue.h (228100 => 228101)
--- trunk/Source/WebCore/workers/service/server/SWServerJobQueue.h 2018-02-05 18:03:58 UTC (rev 228100)
+++ trunk/Source/WebCore/workers/service/server/SWServerJobQueue.h 2018-02-05 18:12:07 UTC (rev 228101)
@@ -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: trunk/Source/WebCore/workers/service/server/SWServerRegistration.cpp (228100 => 228101)
--- trunk/Source/WebCore/workers/service/server/SWServerRegistration.cpp 2018-02-05 18:03:58 UTC (rev 228100)
+++ trunk/Source/WebCore/workers/service/server/SWServerRegistration.cpp 2018-02-05 18:12:07 UTC (rev 228101)
@@ -72,7 +72,6 @@
void SWServerRegistration::setPreInstallationWorker(SWServerWorker* worker)
{
- ASSERT(!m_preInstallationWorker || !worker);
m_preInstallationWorker = worker;
}
