Attachment 373405 Details for Bug 199450 – Patch

[patch] Patch

bug-199450-20190703124415.patch (text/plain), 15.03 KB, created by youenn fablet on 2019-07-03 12:44:15 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: youenn fablet

Created: 2019-07-03 12:44:15 PDT

Size: 15.03 KB

patch

obsolete

>Subversion Revision: 247073
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 0c9c2c0af53dccacb0b76a64f57ee861f743363e..b19d2d393eca66f05eb8b36a4a14af06cea30446 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,52 @@
>+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 NOBODY (OOPS!).
>+
>+        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-02  Andres Gonzalez  <andresg_22@apple.com>
> 
>         Enhance support of aria-haspopup per ARIA 1.1 specification.
>diff --git a/Source/WebCore/workers/service/ServiceWorkerRegistrationKey.h b/Source/WebCore/workers/service/ServiceWorkerRegistrationKey.h
>index 5f2137654db4c3c74d5bf7520756c443a812ad58..72d988c8faeb45b750d8f340fbb2374f17d8f491 100644
>--- a/Source/WebCore/workers/service/ServiceWorkerRegistrationKey.h
>+++ b/Source/WebCore/workers/service/ServiceWorkerRegistrationKey.h
>@@ -41,6 +41,8 @@ public:
>     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(); }
>diff --git a/Source/WebCore/workers/service/server/RegistrationStore.cpp b/Source/WebCore/workers/service/server/RegistrationStore.cpp
>index bb2e14dd8252e01f984fe2127311149f9eca722b..28055c64bf2e34ae9ef70d44fad44c0359e71d66 100644
>--- a/Source/WebCore/workers/service/server/RegistrationStore.cpp
>+++ b/Source/WebCore/workers/service/server/RegistrationStore.cpp
>@@ -104,21 +104,35 @@ void RegistrationStore::endSuspension()
> 
> 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()
>diff --git a/Source/WebCore/workers/service/server/RegistrationStore.h b/Source/WebCore/workers/service/server/RegistrationStore.h
>index 1dd5a8cd6fc1bb69772b612b7edfd2b51e03664d..5dd7b2eda24dea3bfe72c323d2b48a18d9866f76 100644
>--- a/Source/WebCore/workers/service/server/RegistrationStore.h
>+++ b/Source/WebCore/workers/service/server/RegistrationStore.h
>@@ -57,7 +57,7 @@ public:
> 
>     // Callbacks from the SWServer
>     void updateRegistration(const ServiceWorkerContextData&);
>-    void removeRegistration(SWServerRegistration&);
>+    void removeRegistration(const ServiceWorkerRegistrationKey&);
> 
>     // Callbacks from the database
>     void addRegistrationFromDatabase(ServiceWorkerContextData&&);
>diff --git a/Source/WebCore/workers/service/server/SWServer.cpp b/Source/WebCore/workers/service/server/SWServer.cpp
>index c762903cc93f8244dc9952e71ecc8797ad68d323..3394beb212b4231d9289106b7fd98667204dcf93 100644
>--- a/Source/WebCore/workers/service/server/SWServer.cpp
>+++ b/Source/WebCore/workers/service/server/SWServer.cpp
>@@ -83,7 +83,7 @@ SWServer::~SWServer()
> 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 @@ void SWServer::removeRegistration(const ServiceWorkerRegistrationKey& key)
> 
>     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)
>diff --git a/Source/WebCore/workers/service/server/SWServer.h b/Source/WebCore/workers/service/server/SWServer.h
>index 3c97e8795899f5986def53dfe069e3108de5c46d..5599c6b422e97cf2edb17a54bc9472006e62e27c 100644
>--- a/Source/WebCore/workers/service/server/SWServer.h
>+++ b/Source/WebCore/workers/service/server/SWServer.h
>@@ -62,7 +62,7 @@ struct ServiceWorkerFetchResult;
> struct ServiceWorkerRegistrationData;
> class Timer;
> 
>-class SWServer {
>+class SWServer : public CanMakeWeakPtr<SWServer> {
>     WTF_MAKE_FAST_ALLOCATED;
> public:
>     class Connection : public CanMakeWeakPtr<Connection> {
>diff --git a/Source/WebCore/workers/service/server/SWServerWorker.cpp b/Source/WebCore/workers/service/server/SWServerWorker.cpp
>index 873b5029b520ae818eeb2d5829c74baa059376bb..9d91d0b5cd7486289577af95ac1a1d5f94c51d80 100644
>--- a/Source/WebCore/workers/service/server/SWServerWorker.cpp
>+++ b/Source/WebCore/workers/service/server/SWServerWorker.cpp
>@@ -48,7 +48,7 @@ SWServerWorker* SWServerWorker::existingWorkerForIdentifier(ServiceWorkerIdentif
> 
> // 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 @@ SWServerWorker::SWServerWorker(SWServer& server, SWServerRegistration& registrat
>     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 @@ SWServerWorker::~SWServerWorker()
> 
> 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 @@ SWServerToContextConnection* SWServerWorker::contextConnection()
> 
> 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 @@ void SWServerWorker::skipWaiting()
> {
>     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 @@ void SWServerWorker::setHasPendingEvents(bool hasPendingEvents)
>         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 @@ void SWServerWorker::setState(ServiceWorkerState state)
> 
>     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::callWhenActivatedHandler(bool success)
> 
> 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;
> }
> 
>diff --git a/Source/WebCore/workers/service/server/SWServerWorker.h b/Source/WebCore/workers/service/server/SWServerWorker.h
>index 85b7e71dfbbe8f0fc3efda69cb2082e0bcf75c4e..f1c9abb83a798af50fdb0b48800d0c3861a63b0e 100644
>--- a/Source/WebCore/workers/service/server/SWServerWorker.h
>+++ b/Source/WebCore/workers/service/server/SWServerWorker.h
>@@ -73,7 +73,7 @@ public:
>     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 @@ private:
> 
>     void callWhenActivatedHandler(bool success);
> 
>-    SWServer& m_server;
>+    WeakPtr<SWServer> m_server;
>     ServiceWorkerRegistrationKey m_registrationKey;
>     ServiceWorkerData m_data;
>     String m_script;

Actions: View | Formatted Diff | Diff

Attachments on bug 199450: 373394 | 373401 | 373405