Changeset 247104 in webkit

Timestamp:

Jul 3, 2019 2:16:43 PM (5 years ago)

Author:

youenn@apple.com

Message:

Strengthen updating/removing of registrations from the database
​https://bugs.webkit.org/show_bug.cgi?id=199450
rdar://problem/51891395

Reviewed by Chris Dumez.

SWServerWorker is ref counted and has a ref to its SWServer.
There is thus a possibility for SWServerWorker to live longer than its SWServer.
To mitigate this, have SWServerWorker use a WeakPtr<SWServer> and
check whether SWServer is null when receiving messages from WebProcess.
Make also sure that RegistrationStore updated registration map does not get corrupted by checking
the registration keys explicitly.

Covered by existing tests.

• workers/service/ServiceWorkerRegistrationKey.h:

(WebCore::ServiceWorkerRegistrationKey::operator!= const):
(WebCore::ServiceWorkerRegistrationKey::isEmpty const):

• workers/service/server/RegistrationStore.cpp:

(WebCore::RegistrationStore::updateRegistration):
(WebCore::RegistrationStore::removeRegistration):
(WebCore::RegistrationStore::addRegistrationFromDatabase):

• workers/service/server/RegistrationStore.h:
• workers/service/server/SWServer.cpp:

(WebCore::SWServer::workerByID const):
(WebCore::SWServer::removeRegistration):

• workers/service/server/SWServer.h:
• workers/service/server/SWServerWorker.cpp:

(WebCore::SWServerWorker::SWServerWorker):
(WebCore::m_scriptResourceMap):
(WebCore::SWServerWorker::contextData const):
(WebCore::SWServerWorker::terminate):
(WebCore::SWServerWorker::scriptContextFailedToStart):
(WebCore::SWServerWorker::scriptContextStarted):
(WebCore::SWServerWorker::didFinishInstall):
(WebCore::SWServerWorker::didFinishActivation):
(WebCore::SWServerWorker::contextTerminated):
(WebCore::SWServerWorker::findClientByIdentifier const):
(WebCore::SWServerWorker::matchAll):
(WebCore::SWServerWorker::userAgent const):
(WebCore::SWServerWorker::claim):
(WebCore::SWServerWorker::skipWaiting):
(WebCore::SWServerWorker::setHasPendingEvents):
(WebCore::SWServerWorker::setState):

• workers/service/server/SWServerWorker.h:

(WebCore::SWServerWorker::server):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• workers/service/ServiceWorkerRegistrationKey.h (modified) (1 diff)
• workers/service/server/RegistrationStore.cpp (modified) (1 diff)
• workers/service/server/RegistrationStore.h (modified) (1 diff)
• workers/service/server/SWServer.cpp (modified) (2 diffs)
• workers/service/server/SWServer.h (modified) (1 diff)
• workers/service/server/SWServerWorker.cpp (modified) (9 diffs)
• workers/service/server/SWServerWorker.h (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-07-03  Youenn Fablet  <youenn@apple.com>
+
+        Strengthen updating/removing of registrations from the database
+        https://bugs.webkit.org/show_bug.cgi?id=199450
+        rdar://problem/51891395
+
+        Reviewed by Chris Dumez.
+
+        SWServerWorker is ref counted and has a ref to its SWServer.
+        There is thus a possibility for SWServerWorker to live longer than its SWServer.
+        To mitigate this, have SWServerWorker use a WeakPtr<SWServer> and
+        check whether SWServer is null when receiving messages from WebProcess.
+        Make also sure that RegistrationStore updated registration map does not get corrupted by checking
+        the registration keys explicitly.
+
+        Covered by existing tests.
+
+        * workers/service/ServiceWorkerRegistrationKey.h:
+        (WebCore::ServiceWorkerRegistrationKey::operator!= const):
+        (WebCore::ServiceWorkerRegistrationKey::isEmpty const):
+        * workers/service/server/RegistrationStore.cpp:
+        (WebCore::RegistrationStore::updateRegistration):
+        (WebCore::RegistrationStore::removeRegistration):
+        (WebCore::RegistrationStore::addRegistrationFromDatabase):
+        * workers/service/server/RegistrationStore.h:
+        * workers/service/server/SWServer.cpp:
+        (WebCore::SWServer::workerByID const):
+        (WebCore::SWServer::removeRegistration):
+        * workers/service/server/SWServer.h:
+        * workers/service/server/SWServerWorker.cpp:
+        (WebCore::SWServerWorker::SWServerWorker):
+        (WebCore::m_scriptResourceMap):
+        (WebCore::SWServerWorker::contextData const):
+        (WebCore::SWServerWorker::terminate):
+        (WebCore::SWServerWorker::scriptContextFailedToStart):
+        (WebCore::SWServerWorker::scriptContextStarted):
+        (WebCore::SWServerWorker::didFinishInstall):
+        (WebCore::SWServerWorker::didFinishActivation):
+        (WebCore::SWServerWorker::contextTerminated):
+        (WebCore::SWServerWorker::findClientByIdentifier const):
+        (WebCore::SWServerWorker::matchAll):
+        (WebCore::SWServerWorker::userAgent const):
+        (WebCore::SWServerWorker::claim):
+        (WebCore::SWServerWorker::skipWaiting):
+        (WebCore::SWServerWorker::setHasPendingEvents):
+        (WebCore::SWServerWorker::setState):
+        * workers/service/server/SWServerWorker.h:
+        (WebCore::SWServerWorker::server):
+
 2019-07-03  Sam Weinig  <weinig@apple.com>
 

trunk/Source/WebCore/workers/service/ServiceWorkerRegistrationKey.h

@@ -42 +42 @@
 
     bool operator==(const ServiceWorkerRegistrationKey&) const;
+    bool operator!=(const ServiceWorkerRegistrationKey& key) const { return !(*this == key); }
+    bool isEmpty() const { return *this == emptyKey(); }
     WEBCORE_EXPORT bool isMatching(const SecurityOriginData& topOrigin, const URL& clientURL) const;
     bool originIsMatching(const SecurityOriginData& topOrigin, const URL& clientURL) const;

trunk/Source/WebCore/workers/service/server/RegistrationStore.cpp

@@ -105 +105 @@
 void RegistrationStore::updateRegistration(const ServiceWorkerContextData& data)
 {
+    ASSERT(isMainThread());
+    ASSERT(!data.registration.key.isEmpty());
+    if (data.registration.key.isEmpty())
+        return;
+
     m_updatedRegistrations.set(data.registration.key, data);
     scheduleDatabasePushIfNecessary();
 }
 
-void RegistrationStore::removeRegistration(SWServerRegistration& registration)
+void RegistrationStore::removeRegistration(const ServiceWorkerRegistrationKey& key)
 {
+    ASSERT(isMainThread());
+    ASSERT(!key.isEmpty());
+    if (key.isEmpty())
+        return;
+
     ServiceWorkerContextData contextData;
-    contextData.registration.key = registration.key();
-    m_updatedRegistrations.set(registration.key(), WTFMove(contextData));
+    contextData.registration.key = key;
+    m_updatedRegistrations.set(key, WTFMove(contextData));
     scheduleDatabasePushIfNecessary();
 }
 
-void RegistrationStore::addRegistrationFromDatabase(ServiceWorkerContextData&& context)
+void RegistrationStore::addRegistrationFromDatabase(ServiceWorkerContextData&& data)
 {
-    m_server.addRegistrationFromStore(WTFMove(context));
+    ASSERT(!data.registration.key.isEmpty());
+    if (data.registration.key.isEmpty())
+        return;
+
+    m_server.addRegistrationFromStore(WTFMove(data));
 }
 

trunk/Source/WebCore/workers/service/server/RegistrationStore.h

@@ -58 +58 @@
     // Callbacks from the SWServer
     void updateRegistration(const ServiceWorkerContextData&);
-    void removeRegistration(SWServerRegistration&);
+    void removeRegistration(const ServiceWorkerRegistrationKey&);
 
     // Callbacks from the database

trunk/Source/WebCore/workers/service/server/SWServer.cpp

@@ -84 +84 @@
 {
     auto* worker = SWServerWorker::existingWorkerForIdentifier(identifier);
-    ASSERT(!worker || &worker->server() == this);
+    ASSERT(!worker || worker->server() == this);
     return worker;
 }
@@ -178 +178 @@
     m_originStore->remove(topOrigin);
     if (m_registrationStore)
-        m_registrationStore->removeRegistration(*registration);
+        m_registrationStore->removeRegistration(key);
 }
 

trunk/Source/WebCore/workers/service/server/SWServer.h

@@ -63 +63 @@
 class Timer;
 
-class SWServer {
+class SWServer : public CanMakeWeakPtr<SWServer> {
     WTF_MAKE_FAST_ALLOCATED;
 public:

trunk/Source/WebCore/workers/service/server/SWServerWorker.cpp

@@ -49 +49 @@
 // FIXME: Use r-value references for script and contentSecurityPolicy
 SWServerWorker::SWServerWorker(SWServer& server, SWServerRegistration& registration, const URL& scriptURL, const String& script, const ContentSecurityPolicyResponseHeaders& contentSecurityPolicy, String&& referrerPolicy, WorkerType type, ServiceWorkerIdentifier identifier, HashMap<URL, ServiceWorkerContextData::ImportedScript>&& scriptResourceMap)
-    : m_server(server)
+    : m_server(makeWeakPtr(server))
     , m_registrationKey(registration.key())
     , m_data { identifier, scriptURL, ServiceWorkerState::Redundant, type, registration.identifier() }
@@ -63 +63 @@
     ASSERT_UNUSED(result, result.isNewEntry);
 
-    ASSERT(m_server.getRegistration(m_registrationKey));
+    ASSERT(m_server->getRegistration(m_registrationKey));
 }
 
@@ -76 +76 @@
 ServiceWorkerContextData SWServerWorker::contextData() const
 {
-    auto* registration = m_server.getRegistration(m_registrationKey);
+    auto* registration = m_server->getRegistration(m_registrationKey);
     ASSERT(registration);
 
-    return { WTF::nullopt, registration->data(), m_data.identifier, m_script, m_contentSecurityPolicy, m_referrerPolicy, m_data.scriptURL, m_data.type, m_server.sessionID(), false, m_scriptResourceMap };
+    return { WTF::nullopt, registration->data(), m_data.identifier, m_script, m_contentSecurityPolicy, m_referrerPolicy, m_data.scriptURL, m_data.type, m_server->sessionID(), false, m_scriptResourceMap };
 }
 
@@ -85 +85 @@
 {
     if (isRunning())
-        m_server.terminateWorker(*this);
+        m_server->terminateWorker(*this);
 }
 
@@ -103 +103 @@
 void SWServerWorker::scriptContextFailedToStart(const Optional<ServiceWorkerJobDataIdentifier>& jobDataIdentifier, const String& message)
 {
-    m_server.scriptContextFailedToStart(jobDataIdentifier, *this, message);
+    ASSERT(m_server);
+    if (m_server)
+        m_server->scriptContextFailedToStart(jobDataIdentifier, *this, message);
 }
 
 void SWServerWorker::scriptContextStarted(const Optional<ServiceWorkerJobDataIdentifier>& jobDataIdentifier)
 {
-    m_server.scriptContextStarted(jobDataIdentifier, *this);
+    ASSERT(m_server);
+    if (m_server)
+        m_server->scriptContextStarted(jobDataIdentifier, *this);
 }
 
 void SWServerWorker::didFinishInstall(const Optional<ServiceWorkerJobDataIdentifier>& jobDataIdentifier, bool wasSuccessful)
 {
-    m_server.didFinishInstall(jobDataIdentifier, *this, wasSuccessful);
+    ASSERT(m_server);
+    if (m_server)
+        m_server->didFinishInstall(jobDataIdentifier, *this, wasSuccessful);
 }
 
 void SWServerWorker::didFinishActivation()
 {
-    m_server.didFinishActivation(*this);
+    ASSERT(m_server);
+    if (m_server)
+        m_server->didFinishActivation(*this);
 }
 
 void SWServerWorker::contextTerminated()
 {
-    m_server.workerContextTerminated(*this);
+    ASSERT(m_server);
+    if (m_server)
+        m_server->workerContextTerminated(*this);
 }
 
 Optional<ServiceWorkerClientData> SWServerWorker::findClientByIdentifier(const ServiceWorkerClientIdentifier& clientId) const
 {
-    return m_server.serviceWorkerClientWithOriginByID(origin(), clientId);
+    ASSERT(m_server);
+    if (!m_server)
+        return { };
+    return m_server->serviceWorkerClientWithOriginByID(origin(), clientId);
 }
 
 void SWServerWorker::matchAll(const ServiceWorkerClientQueryOptions& options, ServiceWorkerClientsMatchAllCallback&& callback)
 {
-    return m_server.matchAll(*this, options, WTFMove(callback));
+    ASSERT(m_server);
+    if (!m_server)
+        return callback({ });
+    return m_server->matchAll(*this, options, WTFMove(callback));
 }
 
 String SWServerWorker::userAgent() const
 {
-    return m_server.serviceWorkerClientUserAgent(origin());
+    ASSERT(m_server);
+    if (!m_server)
+        return { };
+    return m_server->serviceWorkerClientUserAgent(origin());
 }
 
 void SWServerWorker::claim()
 {
-    return m_server.claim(*this);
+    ASSERT(m_server);
+    if (m_server)
+        m_server->claim(*this);
 }
 
@@ -155 +176 @@
     m_isSkipWaitingFlagSet = true;
 
-    auto* registration = m_server.getRegistration(m_registrationKey);
+    auto* registration = m_server->getRegistration(m_registrationKey);
     ASSERT(registration || isTerminating());
     if (registration)
@@ -171 +192 @@
 
     // Do tryClear/tryActivate, as per https://w3c.github.io/ServiceWorker/#wait-until-method.
-    auto* registration = m_server.getRegistration(m_registrationKey);
+    auto* registration = m_server->getRegistration(m_registrationKey);
     if (!registration)
         return;
@@ -196 +217 @@
     m_data.state = state;
 
-    auto* registration = m_server.getRegistration(m_registrationKey);
+    auto* registration = m_server->getRegistration(m_registrationKey);
     ASSERT(registration || state == ServiceWorkerState::Redundant);
     if (registration) {
@@ -217 +238 @@
 void SWServerWorker::setState(State state)
 {
-    ASSERT(state != State::Running || m_server.getRegistration(m_registrationKey));
+    ASSERT(state != State::Running || m_server->getRegistration(m_registrationKey));
     m_state = state;
 }

trunk/Source/WebCore/workers/service/server/SWServerWorker.h

@@ -74 +74 @@
     void setState(State);
 
-    SWServer& server() { return m_server; }
+    SWServer* server() { return m_server.get(); }
     const ServiceWorkerRegistrationKey& registrationKey() const { return m_registrationKey; }
     const URL& scriptURL() const { return m_data.scriptURL; }
@@ -118 +118 @@
     void callWhenActivatedHandler(bool success);
 
-    SWServer& m_server;
+    WeakPtr<SWServer> m_server;
     ServiceWorkerRegistrationKey m_registrationKey;
     ServiceWorkerData m_data;