Log 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):
Modified Paths
- trunk/Source/WebCore/ChangeLog
- trunk/Source/WebCore/workers/service/ServiceWorkerRegistrationKey.h
- trunk/Source/WebCore/workers/service/server/RegistrationStore.cpp
- trunk/Source/WebCore/workers/service/server/RegistrationStore.h
- trunk/Source/WebCore/workers/service/server/SWServer.cpp
- trunk/Source/WebCore/workers/service/server/SWServer.h
- trunk/Source/WebCore/workers/service/server/SWServerWorker.cpp
- trunk/Source/WebCore/workers/service/server/SWServerWorker.h
Diff
Modified: trunk/Source/WebCore/ChangeLog (247103 => 247104)
--- trunk/Source/WebCore/ChangeLog 2019-07-03 21:01:50 UTC (rev 247103)
+++ trunk/Source/WebCore/ChangeLog 2019-07-03 21:16:43 UTC (rev 247104)
@@ -1,3 +1,52 @@
+2019-07-03 Youenn Fablet <[email protected]>
+
+ 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 <[email protected]>
Adopt simple structured bindings in more places
Modified: trunk/Source/WebCore/workers/service/ServiceWorkerRegistrationKey.h (247103 => 247104)
--- trunk/Source/WebCore/workers/service/ServiceWorkerRegistrationKey.h 2019-07-03 21:01:50 UTC (rev 247103)
+++ trunk/Source/WebCore/workers/service/ServiceWorkerRegistrationKey.h 2019-07-03 21:16:43 UTC (rev 247104)
@@ -41,6 +41,8 @@
unsigned hash() const;
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;
size_t scopeLength() const { return m_scope.string().length(); }
Modified: trunk/Source/WebCore/workers/service/server/RegistrationStore.cpp (247103 => 247104)
--- trunk/Source/WebCore/workers/service/server/RegistrationStore.cpp 2019-07-03 21:01:50 UTC (rev 247103)
+++ trunk/Source/WebCore/workers/service/server/RegistrationStore.cpp 2019-07-03 21:16:43 UTC (rev 247104)
@@ -104,21 +104,35 @@
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));
}
void RegistrationStore::databaseFailedToOpen()
Modified: trunk/Source/WebCore/workers/service/server/RegistrationStore.h (247103 => 247104)
--- trunk/Source/WebCore/workers/service/server/RegistrationStore.h 2019-07-03 21:01:50 UTC (rev 247103)
+++ trunk/Source/WebCore/workers/service/server/RegistrationStore.h 2019-07-03 21:16:43 UTC (rev 247104)
@@ -57,7 +57,7 @@
// Callbacks from the SWServer
void updateRegistration(const ServiceWorkerContextData&);
- void removeRegistration(SWServerRegistration&);
+ void removeRegistration(const ServiceWorkerRegistrationKey&);
// Callbacks from the database
void addRegistrationFromDatabase(ServiceWorkerContextData&&);
Modified: trunk/Source/WebCore/workers/service/server/SWServer.cpp (247103 => 247104)
--- trunk/Source/WebCore/workers/service/server/SWServer.cpp 2019-07-03 21:01:50 UTC (rev 247103)
+++ trunk/Source/WebCore/workers/service/server/SWServer.cpp 2019-07-03 21:16:43 UTC (rev 247104)
@@ -83,7 +83,7 @@
SWServerWorker* SWServer::workerByID(ServiceWorkerIdentifier identifier) const
{
auto* worker = SWServerWorker::existingWorkerForIdentifier(identifier);
- ASSERT(!worker || &worker->server() == this);
+ ASSERT(!worker || worker->server() == this);
return worker;
}
@@ -177,7 +177,7 @@
m_originStore->remove(topOrigin);
if (m_registrationStore)
- m_registrationStore->removeRegistration(*registration);
+ m_registrationStore->removeRegistration(key);
}
Vector<ServiceWorkerRegistrationData> SWServer::getRegistrations(const SecurityOriginData& topOrigin, const URL& clientURL)
Modified: trunk/Source/WebCore/workers/service/server/SWServer.h (247103 => 247104)
--- trunk/Source/WebCore/workers/service/server/SWServer.h 2019-07-03 21:01:50 UTC (rev 247103)
+++ trunk/Source/WebCore/workers/service/server/SWServer.h 2019-07-03 21:16:43 UTC (rev 247104)
@@ -62,7 +62,7 @@
struct ServiceWorkerRegistrationData;
class Timer;
-class SWServer {
+class SWServer : public CanMakeWeakPtr<SWServer> {
WTF_MAKE_FAST_ALLOCATED;
public:
class Connection : public CanMakeWeakPtr<Connection> {
Modified: trunk/Source/WebCore/workers/service/server/SWServerWorker.cpp (247103 => 247104)
--- trunk/Source/WebCore/workers/service/server/SWServerWorker.cpp 2019-07-03 21:01:50 UTC (rev 247103)
+++ trunk/Source/WebCore/workers/service/server/SWServerWorker.cpp 2019-07-03 21:16:43 UTC (rev 247104)
@@ -48,7 +48,7 @@
// 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() }
, m_script(script)
@@ -62,7 +62,7 @@
auto result = allWorkers().add(identifier, this);
ASSERT_UNUSED(result, result.isNewEntry);
- ASSERT(m_server.getRegistration(m_registrationKey));
+ ASSERT(m_server->getRegistration(m_registrationKey));
}
SWServerWorker::~SWServerWorker()
@@ -75,16 +75,16 @@
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 };
}
void SWServerWorker::terminate()
{
if (isRunning())
- m_server.terminateWorker(*this);
+ m_server->terminateWorker(*this);
}
const ClientOrigin& SWServerWorker::origin() const
@@ -102,47 +102,68 @@
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);
}
void SWServerWorker::setScriptResource(URL&& url, ServiceWorkerContextData::ImportedScript&& script)
@@ -154,7 +175,7 @@
{
m_isSkipWaitingFlagSet = true;
- auto* registration = m_server.getRegistration(m_registrationKey);
+ auto* registration = m_server->getRegistration(m_registrationKey);
ASSERT(registration || isTerminating());
if (registration)
registration->tryActivate();
@@ -170,7 +191,7 @@
return;
// 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;
@@ -195,7 +216,7 @@
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) {
registration->forEachConnection([&](auto& connection) {
@@ -216,7 +237,7 @@
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;
}
Modified: trunk/Source/WebCore/workers/service/server/SWServerWorker.h (247103 => 247104)
--- trunk/Source/WebCore/workers/service/server/SWServerWorker.h 2019-07-03 21:01:50 UTC (rev 247103)
+++ trunk/Source/WebCore/workers/service/server/SWServerWorker.h 2019-07-03 21:16:43 UTC (rev 247104)
@@ -73,7 +73,7 @@
bool isTerminating() const { return m_state == State::Terminating; }
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; }
const String& script() const { return m_script; }
@@ -117,7 +117,7 @@
void callWhenActivatedHandler(bool success);
- SWServer& m_server;
+ WeakPtr<SWServer> m_server;
ServiceWorkerRegistrationKey m_registrationKey;
ServiceWorkerData m_data;
String m_script;
_______________________________________________ webkit-changes mailing list [email protected] https://lists.webkit.org/mailman/listinfo/webkit-changes
