- Revision
- 227221
- Author
- [email protected]
- Date
- 2018-01-19 11:13:54 -0800 (Fri, 19 Jan 2018)
Log Message
ASSERT(registration || isTerminating()) hit in SWServerWorker::skipWaiting()
https://bugs.webkit.org/show_bug.cgi?id=181761
<rdar://problem/36594564>
Reviewed by Youenn Fablet.
There is a short period of time, early in the registration process where a
SWServerWorker object exists for a registration but is not in the registration's
installing/waiting/active slots yet. As a result, if a registration is cleared
during this period (for e.g. due to the user clearing all website data), that
SWServerWorker will not be terminated. We then hit assertion later on when this
worker is trying to do things (like call skipWaiting).
To address the issue, we now keep a reference this SWServerWorker on the
registration, via a new SWServerRegistration::m_preInstallationWorker data member.
When the registration is cleared, we now take care of terminating this worker.
No new tests, covered by existing tests that crash flakily in debug builds.
* workers/WorkerThread.cpp:
(WebCore::WorkerThread::stop):
if the mutex is locked, then the worker thread is still starting. We spin the
runloop and try to stop again later. This avoids the deadlock shown in
Bug 181763 as the worker thread may need to interact with the main thread
during startup.
* workers/service/server/SWServer.cpp:
(WebCore::SWServer::installContextData):
* workers/service/server/SWServerJobQueue.cpp:
(WebCore::SWServerJobQueue::scriptContextFailedToStart):
(WebCore::SWServerJobQueue::install):
* workers/service/server/SWServerRegistration.cpp:
(WebCore::SWServerRegistration::~SWServerRegistration):
(WebCore::SWServerRegistration::setPreInstallationWorker):
(WebCore::SWServerRegistration::clear):
* workers/service/server/SWServerRegistration.h:
(WebCore::SWServerRegistration::preInstallationWorker const):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (227220 => 227221)
--- trunk/Source/WebCore/ChangeLog 2018-01-19 19:13:24 UTC (rev 227220)
+++ trunk/Source/WebCore/ChangeLog 2018-01-19 19:13:54 UTC (rev 227221)
@@ -1,5 +1,45 @@
2018-01-19 Chris Dumez <[email protected]>
+ ASSERT(registration || isTerminating()) hit in SWServerWorker::skipWaiting()
+ https://bugs.webkit.org/show_bug.cgi?id=181761
+ <rdar://problem/36594564>
+
+ Reviewed by Youenn Fablet.
+
+ There is a short period of time, early in the registration process where a
+ SWServerWorker object exists for a registration but is not in the registration's
+ installing/waiting/active slots yet. As a result, if a registration is cleared
+ during this period (for e.g. due to the user clearing all website data), that
+ SWServerWorker will not be terminated. We then hit assertion later on when this
+ worker is trying to do things (like call skipWaiting).
+
+ To address the issue, we now keep a reference this SWServerWorker on the
+ registration, via a new SWServerRegistration::m_preInstallationWorker data member.
+ When the registration is cleared, we now take care of terminating this worker.
+
+ No new tests, covered by existing tests that crash flakily in debug builds.
+
+ * workers/WorkerThread.cpp:
+ (WebCore::WorkerThread::stop):
+ if the mutex is locked, then the worker thread is still starting. We spin the
+ runloop and try to stop again later. This avoids the deadlock shown in
+ Bug 181763 as the worker thread may need to interact with the main thread
+ during startup.
+
+ * workers/service/server/SWServer.cpp:
+ (WebCore::SWServer::installContextData):
+ * workers/service/server/SWServerJobQueue.cpp:
+ (WebCore::SWServerJobQueue::scriptContextFailedToStart):
+ (WebCore::SWServerJobQueue::install):
+ * workers/service/server/SWServerRegistration.cpp:
+ (WebCore::SWServerRegistration::~SWServerRegistration):
+ (WebCore::SWServerRegistration::setPreInstallationWorker):
+ (WebCore::SWServerRegistration::clear):
+ * workers/service/server/SWServerRegistration.h:
+ (WebCore::SWServerRegistration::preInstallationWorker const):
+
+2018-01-19 Chris Dumez <[email protected]>
+
Service worker registrations restored from disk may not be reused when the JS calls register() again
https://bugs.webkit.org/show_bug.cgi?id=181810
<rdar://problem/36591711>
Modified: trunk/Source/WebCore/workers/WorkerThread.cpp (227220 => 227221)
--- trunk/Source/WebCore/workers/WorkerThread.cpp 2018-01-19 19:13:24 UTC (rev 227220)
+++ trunk/Source/WebCore/workers/WorkerThread.cpp 2018-01-19 19:13:54 UTC (rev 227221)
@@ -265,7 +265,15 @@
// Mutex protection is necessary to ensure that m_workerGlobalScope isn't changed by
// WorkerThread::workerThread() while we're accessing it. Note also that stop() can
// be called before m_workerGlobalScope is fully created.
- LockHolder lock(m_threadCreationAndWorkerGlobalScopeMutex);
+ auto locker = Locker<Lock>::tryLock(m_threadCreationAndWorkerGlobalScopeMutex);
+ if (!locker) {
+ // The thread is still starting, spin the runloop and try again to avoid deadlocks if the worker thread
+ // needs to interact with the main thread during startup.
+ callOnMainThread([this, stoppedCallback = WTFMove(stoppedCallback)]() mutable {
+ stop(WTFMove(stoppedCallback));
+ });
+ return;
+ }
ASSERT(!m_stoppedCallback);
m_stoppedCallback = WTFMove(stoppedCallback);
Modified: trunk/Source/WebCore/workers/service/server/SWServer.cpp (227220 => 227221)
--- trunk/Source/WebCore/workers/service/server/SWServer.cpp 2018-01-19 19:13:24 UTC (rev 227220)
+++ trunk/Source/WebCore/workers/service/server/SWServer.cpp 2018-01-19 19:13:54 UTC (rev 227221)
@@ -503,6 +503,7 @@
return;
}
+ registration->setPreInstallationWorker(worker.ptr());
worker->setState(SWServerWorker::State::Running);
auto result = m_runningOrTerminatingWorkers.add(data.serviceWorkerIdentifier, WTFMove(worker));
ASSERT_UNUSED(result, result.isNewEntry);
Modified: trunk/Source/WebCore/workers/service/server/SWServerJobQueue.cpp (227220 => 227221)
--- trunk/Source/WebCore/workers/service/server/SWServerJobQueue.cpp 2018-01-19 19:13:24 UTC (rev 227220)
+++ trunk/Source/WebCore/workers/service/server/SWServerJobQueue.cpp 2018-01-19 19:13:54 UTC (rev 227221)
@@ -109,6 +109,10 @@
auto* registration = m_server.getRegistration(m_registrationKey);
ASSERT(registration);
+ ASSERT(registration->preInstallationWorker());
+ registration->preInstallationWorker()->terminate();
+ registration->setPreInstallationWorker(nullptr);
+
// Invoke Reject Job Promise with job and TypeError.
m_server.rejectJob(firstJob(), { TypeError, message });
@@ -138,6 +142,9 @@
auto* worker = m_server.workerByID(installingWorker);
RELEASE_ASSERT(worker);
+ ASSERT(registration.preInstallationWorker() == worker);
+ registration.setPreInstallationWorker(nullptr);
+
registration.updateRegistrationState(ServiceWorkerRegistrationState::Installing, worker);
registration.updateWorkerState(*worker, ServiceWorkerState::Installing);
Modified: trunk/Source/WebCore/workers/service/server/SWServerRegistration.cpp (227220 => 227221)
--- trunk/Source/WebCore/workers/service/server/SWServerRegistration.cpp 2018-01-19 19:13:24 UTC (rev 227220)
+++ trunk/Source/WebCore/workers/service/server/SWServerRegistration.cpp 2018-01-19 19:13:54 UTC (rev 227221)
@@ -54,6 +54,7 @@
SWServerRegistration::~SWServerRegistration()
{
+ ASSERT(!m_preInstallationWorker || !m_preInstallationWorker->isRunning());
ASSERT(!m_installingWorker || !m_installingWorker->isRunning());
ASSERT(!m_waitingWorker || !m_waitingWorker->isRunning());
ASSERT(!m_activeWorker || !m_activeWorker->isRunning());
@@ -69,6 +70,12 @@
return m_activeWorker.get();
}
+void SWServerRegistration::setPreInstallationWorker(SWServerWorker* worker)
+{
+ ASSERT(!m_preInstallationWorker || !worker);
+ m_preInstallationWorker = worker;
+}
+
void SWServerRegistration::updateRegistrationState(ServiceWorkerRegistrationState state, SWServerWorker* worker)
{
LOG(ServiceWorker, "(%p) Updating registration state to %i with worker %p", this, (int)state, worker);
@@ -231,6 +238,12 @@
// https://w3c.github.io/ServiceWorker/#clear-registration
void SWServerRegistration::clear()
{
+ if (m_preInstallationWorker) {
+ ASSERT(m_preInstallationWorker->state() == ServiceWorkerState::Redundant);
+ m_preInstallationWorker->terminate();
+ m_preInstallationWorker = nullptr;
+ }
+
clearRegistrationWorker(*this, installingWorker(), ServiceWorkerRegistrationState::Installing);
clearRegistrationWorker(*this, waitingWorker(), ServiceWorkerRegistrationState::Waiting);
clearRegistrationWorker(*this, activeWorker(), ServiceWorkerRegistrationState::Active);
Modified: trunk/Source/WebCore/workers/service/server/SWServerRegistration.h (227220 => 227221)
--- trunk/Source/WebCore/workers/service/server/SWServerRegistration.h 2018-01-19 19:13:24 UTC (rev 227220)
+++ trunk/Source/WebCore/workers/service/server/SWServerRegistration.h 2018-01-19 19:13:54 UTC (rev 227221)
@@ -73,6 +73,8 @@
void addClientServiceWorkerRegistration(SWServerConnectionIdentifier);
void removeClientServiceWorkerRegistration(SWServerConnectionIdentifier);
+ void setPreInstallationWorker(SWServerWorker*);
+ SWServerWorker* preInstallationWorker() const { return m_preInstallationWorker.get(); }
SWServerWorker* installingWorker() const { return m_installingWorker.get(); }
SWServerWorker* waitingWorker() const { return m_waitingWorker.get(); }
SWServerWorker* activeWorker() const { return m_activeWorker.get(); }
@@ -105,6 +107,7 @@
URL m_scriptURL;
bool m_uninstalling { false };
+ RefPtr<SWServerWorker> m_preInstallationWorker; // Implementation detail, not part of the specification.
RefPtr<SWServerWorker> m_installingWorker;
RefPtr<SWServerWorker> m_waitingWorker;
RefPtr<SWServerWorker> m_activeWorker;
