ASSERT(registration || isTerminating()) hit in SWServerWorker::skipWaiting()
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 19 Jan 2018 19:13:54 +0000 (19:13 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 19 Jan 2018 19:13:54 +0000 (19:13 +0000)
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):

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

Source/WebCore/ChangeLog
Source/WebCore/workers/WorkerThread.cpp
Source/WebCore/workers/service/server/SWServer.cpp
Source/WebCore/workers/service/server/SWServerJobQueue.cpp
Source/WebCore/workers/service/server/SWServerRegistration.cpp
Source/WebCore/workers/service/server/SWServerRegistration.h

index 4bec808..3f221eb 100644 (file)
@@ -1,5 +1,45 @@
 2018-01-19  Chris Dumez  <cdumez@apple.com>
 
+        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  <cdumez@apple.com>
+
         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>
index 2741f14..eeb602f 100644 (file)
@@ -265,7 +265,15 @@ void WorkerThread::stop(WTF::Function<void()>&& stoppedCallback)
     // 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);
index 884d02e..161d15b 100644 (file)
@@ -503,6 +503,7 @@ void SWServer::installContextData(const ServiceWorkerContextData& data)
         return;
     }
 
+    registration->setPreInstallationWorker(worker.ptr());
     worker->setState(SWServerWorker::State::Running);
     auto result = m_runningOrTerminatingWorkers.add(data.serviceWorkerIdentifier, WTFMove(worker));
     ASSERT_UNUSED(result, result.isNewEntry);
index bd8e200..db3b81f 100644 (file)
@@ -109,6 +109,10 @@ void SWServerJobQueue::scriptContextFailedToStart(const ServiceWorkerJobDataIden
     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 @@ void SWServerJobQueue::install(SWServerRegistration& registration, ServiceWorker
     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);
 
index 6b031c9..a565dfe 100644 (file)
@@ -54,6 +54,7 @@ SWServerRegistration::SWServerRegistration(SWServer& server, const ServiceWorker
 
 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 @@ SWServerWorker* SWServerRegistration::getNewestWorker()
     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 @@ static void clearRegistrationWorker(SWServerRegistration& registration, SWServer
 // 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);
index 962d962..b971fe0 100644 (file)
@@ -73,6 +73,8 @@ public:
     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 @@ private:
     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;