Attachment 342703 Details for Bug 186584 – Alternative (SWServer owns the connections)

[patch] Alternative (SWServer owns the connections)

SWServer_owns_connections.patch (text/plain), 16.41 KB, created by Chris Dumez on 2018-06-13 16:08:40 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-06-13 16:08:40 PDT

Size: 16.41 KB

patch

obsolete

>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 852f833009f..218d5d34880 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,37 @@
>+2018-06-13  Chris Dumez  <cdumez@apple.com>
>+
>+        Crash under SWServer::unregisterConnection(Connection&)
>+        https://bugs.webkit.org/show_bug.cgi?id=186584
>+        <rdar://problem/40931680>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The crash was due to SWServer::Connection objects outliving their SWServer, even
>+        though SWServer::Connection::m_server is a C++ reference. This was possible because
>+        SWServer does not own the connections, StorageToWebProcessConnection does. This
>+        started crashing recently, after r232423, because SWServer can get destroyed now.
>+        The SWServer might get destroyed before the StorageToWebProcessConnection, in which
>+        case the SWServer::Connection objects will get destroyed later. We were crashing
>+        because the SWServer::Connection destructor tries to unregister the connection from
>+        the SWServer (which is dead).
>+
>+        To address the issue, the SWServer now owns the connections. StorageToWebProcessConnection
>+        merely has weak pointers to the connections.
>+
>+        * workers/service/server/SWServer.cpp:
>+        (WebCore::SWServer::Connection::Connection):
>+        (WebCore::SWServer::addConnection):
>+        (WebCore::SWServer::removeConnection):
>+        (WebCore::SWServer::resolveRegistrationReadyRequests):
>+        * workers/service/server/SWServer.h:
>+        (WebCore::SWServer::Connection::~Connection):
>+        (WebCore::SWServer::Connection::server):
>+        (WebCore::SWServer::connection):
>+        * workers/service/server/SWServerRegistration.cpp:
>+        (WebCore::SWServerRegistration::forEachConnection):
>+        (WebCore::SWServerRegistration::notifyClientsOfControllerChange):
>+        (WebCore::SWServerRegistration::controlClient):
>+
> 2018-06-12  Ryosuke Niwa  <rniwa@webkit.org>
> 
>         iOS WK1: Occasional crash in FrameView::setScrollPosition
>diff --git a/Source/WebCore/workers/service/server/SWServer.cpp b/Source/WebCore/workers/service/server/SWServer.cpp
>index 08c31a79029..0310001af76 100644
>--- a/Source/WebCore/workers/service/server/SWServer.cpp
>+++ b/Source/WebCore/workers/service/server/SWServer.cpp
>@@ -54,12 +54,6 @@ SWServer::Connection::Connection(SWServer& server)
>     : m_server(server)
>     , m_identifier(generateObjectIdentifier<SWServerConnectionIdentifierType>())
> {
>-    m_server.registerConnection(*this);
>-}
>-
>-SWServer::Connection::~Connection()
>-{
>-    m_server.unregisterConnection(*this);
> }
> 
> HashSet<SWServer*>& SWServer::allServers()
>@@ -692,23 +686,23 @@ void SWServer::fireActivateEvent(SWServerWorker& worker)
>     contextConnection->fireActivateEvent(worker.identifier());
> }
> 
>-void SWServer::registerConnection(Connection& connection)
>+void SWServer::addConnection(std::unique_ptr<Connection>&& connection)
> {
>-    auto result = m_connections.add(connection.identifier(), nullptr);
>-    ASSERT(result.isNewEntry);
>-    result.iterator->value = &connection;
>+    auto identifier = connection->identifier();
>+    ASSERT(!m_connections.contains(identifier));
>+    m_connections.add(identifier, WTFMove(connection));
> }
> 
>-void SWServer::unregisterConnection(Connection& connection)
>+void SWServer::removeConnection(SWServerConnectionIdentifier connectionIdentifier)
> {
>-    ASSERT(m_connections.get(connection.identifier()) == &connection);
>-    m_connections.remove(connection.identifier());
>+    ASSERT(m_connections.contains(connectionIdentifier));
>+    m_connections.remove(connectionIdentifier);
> 
>     for (auto& registration : m_registrations.values())
>-        registration->unregisterServerConnection(connection.identifier());
>+        registration->unregisterServerConnection(connectionIdentifier);
> 
>     for (auto& jobQueue : m_jobQueues.values())
>-        jobQueue->cancelJobsFromConnection(connection.identifier());
>+        jobQueue->cancelJobsFromConnection(connectionIdentifier);
> }
> 
> SWServerRegistration* SWServer::doRegistrationMatching(const SecurityOriginData& topOrigin, const URL& clientURL)
>@@ -817,7 +811,7 @@ bool SWServer::needsServerToContextConnectionForOrigin(const SecurityOriginData&
> 
> void SWServer::resolveRegistrationReadyRequests(SWServerRegistration& registration)
> {
>-    for (auto* connection : m_connections.values())
>+    for (auto& connection : m_connections.values())
>         connection->resolveRegistrationReadyRequests(registration);
> }
> 
>diff --git a/Source/WebCore/workers/service/server/SWServer.h b/Source/WebCore/workers/service/server/SWServer.h
>index 49aec184ffa..477b2220b4d 100644
>--- a/Source/WebCore/workers/service/server/SWServer.h
>+++ b/Source/WebCore/workers/service/server/SWServer.h
>@@ -69,7 +69,7 @@ public:
>         WTF_MAKE_FAST_ALLOCATED;
>         friend class SWServer;
>     public:
>-        WEBCORE_EXPORT virtual ~Connection();
>+        WEBCORE_EXPORT virtual ~Connection() { }
> 
>         using Identifier = SWServerConnectionIdentifier;
>         Identifier identifier() const { return m_identifier; }
>@@ -87,9 +87,10 @@ public:
>         virtual void notifyClientsOfControllerChange(const HashSet<DocumentIdentifier>& contextIdentifiers, const ServiceWorkerData& newController) = 0;
>         virtual void registrationReady(uint64_t registrationReadyRequestIdentifier, ServiceWorkerRegistrationData&&) = 0;
> 
>+        SWServer& server() { return m_server; }
>+
>     protected:
>         WEBCORE_EXPORT explicit Connection(SWServer&);
>-        SWServer& server() { return m_server; }
> 
>         WEBCORE_EXPORT void finishFetchingScriptInServer(const ServiceWorkerFetchResult&);
>         WEBCORE_EXPORT void addServiceWorkerRegistrationInServer(ServiceWorkerRegistrationIdentifier);
>@@ -144,7 +145,10 @@ public:
> 
>     WEBCORE_EXPORT void markAllWorkersForOriginAsTerminated(const SecurityOriginData&);
>     
>-    Connection* getConnection(SWServerConnectionIdentifier identifier) { return m_connections.get(identifier); }
>+    WEBCORE_EXPORT void addConnection(std::unique_ptr<Connection>&&);
>+    WEBCORE_EXPORT void removeConnection(SWServerConnectionIdentifier);
>+    Connection* connection(SWServerConnectionIdentifier identifier) { return m_connections.get(identifier); }
>+
>     SWOriginStore& originStore() { return m_originStore; }
> 
>     void scriptContextFailedToStart(const std::optional<ServiceWorkerJobDataIdentifier>&, SWServerWorker&, const String& message);
>@@ -179,9 +183,6 @@ public:
>     void disableServiceWorkerProcessTerminationDelay() { m_shouldDisableServiceWorkerProcessTerminationDelay = true; }
> 
> private:
>-    void registerConnection(Connection&);
>-    void unregisterConnection(Connection&);
>-
>     void scriptFetchFinished(Connection&, const ServiceWorkerFetchResult&);
> 
>     void didResolveRegistrationPromise(Connection&, const ServiceWorkerRegistrationKey&);
>@@ -208,7 +209,7 @@ private:
>     };
>     void terminateWorkerInternal(SWServerWorker&, TerminationMode);
> 
>-    HashMap<SWServerConnectionIdentifier, Connection*> m_connections;
>+    HashMap<SWServerConnectionIdentifier, std::unique_ptr<Connection>> m_connections;
>     HashMap<ServiceWorkerRegistrationKey, std::unique_ptr<SWServerRegistration>> m_registrations;
>     HashMap<ServiceWorkerRegistrationIdentifier, SWServerRegistration*> m_registrationsByID;
>     HashMap<ServiceWorkerRegistrationKey, std::unique_ptr<SWServerJobQueue>> m_jobQueues;
>diff --git a/Source/WebCore/workers/service/server/SWServerRegistration.cpp b/Source/WebCore/workers/service/server/SWServerRegistration.cpp
>index 500f16583e0..3a1b6d1cdbb 100644
>--- a/Source/WebCore/workers/service/server/SWServerRegistration.cpp
>+++ b/Source/WebCore/workers/service/server/SWServerRegistration.cpp
>@@ -136,7 +136,7 @@ void SWServerRegistration::fireUpdateFoundEvent()
> void SWServerRegistration::forEachConnection(const WTF::Function<void(SWServer::Connection&)>& apply)
> {
>     for (auto connectionIdentifierWithClients : m_connectionsWithClientRegistrations.values()) {
>-        if (auto* connection = m_server.getConnection(connectionIdentifierWithClients))
>+        if (auto* connection = m_server.connection(connectionIdentifierWithClients))
>             apply(*connection);
>     }
> }
>@@ -198,7 +198,7 @@ void SWServerRegistration::notifyClientsOfControllerChange()
>     ASSERT(activeWorker());
> 
>     for (auto& item : m_clientsUsingRegistration) {
>-        if (auto* connection = m_server.getConnection(item.key))
>+        if (auto* connection = m_server.connection(item.key))
>             connection->notifyClientsOfControllerChange(item.value, activeWorker()->data());
>     }
> }
>@@ -346,7 +346,7 @@ void SWServerRegistration::controlClient(ServiceWorkerClientIdentifier identifie
> 
>     HashSet<DocumentIdentifier> identifiers;
>     identifiers.add(identifier.contextIdentifier);
>-    m_server.getConnection(identifier.serverConnectionIdentifier)->notifyClientsOfControllerChange(identifiers, activeWorker()->data());
>+    m_server.connection(identifier.serverConnectionIdentifier)->notifyClientsOfControllerChange(identifiers, activeWorker()->data());
> }
> 
> void SWServerRegistration::setIsUninstalling(bool value)
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index a777d75e06d..8477a58f14e 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,22 @@
>+2018-06-13  Chris Dumez  <cdumez@apple.com>
>+
>+        Crash under SWServer::unregisterConnection(Connection&)
>+        https://bugs.webkit.org/show_bug.cgi?id=186584
>+        <rdar://problem/40931680>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * StorageProcess/ServiceWorker/WebSWServerConnection.cpp:
>+        * StorageProcess/ServiceWorker/WebSWServerConnection.h:
>+        * StorageProcess/StorageToWebProcessConnection.cpp:
>+        (WebKit::StorageToWebProcessConnection::~StorageToWebProcessConnection):
>+        (WebKit::StorageToWebProcessConnection::didReceiveMessage):
>+        (WebKit::StorageToWebProcessConnection::didReceiveSyncMessage):
>+        (WebKit::StorageToWebProcessConnection::didClose):
>+        (WebKit::StorageToWebProcessConnection::unregisterSWConnections):
>+        (WebKit::StorageToWebProcessConnection::establishSWServerConnection):
>+        * StorageProcess/StorageToWebProcessConnection.h:
>+
> 2018-06-12  Brent Fulgham  <bfulgham@apple.com>
> 
>         Turn CSS Spring Animations and Link Preload off by default for production builds.
>diff --git a/Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp b/Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp
>index a941843d4f8..ff7ec594c1f 100644
>--- a/Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp
>+++ b/Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp
>@@ -76,11 +76,6 @@ WebSWServerConnection::~WebSWServerConnection()
>         server().unregisterServiceWorkerClient(keyValue.value, keyValue.key);
> }
> 
>-void WebSWServerConnection::disconnectedFromWebProcess()
>-{
>-    notImplemented();
>-}
>-
> void WebSWServerConnection::rejectJobInClient(ServiceWorkerJobIdentifier jobIdentifier, const ExceptionData& exceptionData)
> {
>     send(Messages::WebSWClientConnection::JobRejectedInServer(jobIdentifier, exceptionData));
>diff --git a/Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.h b/Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.h
>index 9cf90464f15..2ca6625d1c0 100644
>--- a/Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.h
>+++ b/Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.h
>@@ -56,7 +56,6 @@ public:
> 
>     IPC::Connection& ipcConnection() const { return m_contentConnection.get(); }
> 
>-    void disconnectedFromWebProcess();
>     void didReceiveMessage(IPC::Connection&, IPC::Decoder&) final;
>     void didReceiveSyncMessage(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder>&);
> 
>diff --git a/Source/WebKit/StorageProcess/StorageToWebProcessConnection.cpp b/Source/WebKit/StorageProcess/StorageToWebProcessConnection.cpp
>index ae75c85cc7e..36b3a2053a5 100644
>--- a/Source/WebKit/StorageProcess/StorageToWebProcessConnection.cpp
>+++ b/Source/WebKit/StorageProcess/StorageToWebProcessConnection.cpp
>@@ -61,6 +61,10 @@ StorageToWebProcessConnection::StorageToWebProcessConnection(IPC::Connection::Id
> StorageToWebProcessConnection::~StorageToWebProcessConnection()
> {
>     m_connection->invalidate();
>+
>+#if ENABLE(SERVICE_WORKER)
>+    unregisterSWConnections();
>+#endif
> }
> 
> void StorageToWebProcessConnection::didReceiveMessage(IPC::Connection& connection, IPC::Decoder& decoder)
>@@ -86,9 +90,8 @@ void StorageToWebProcessConnection::didReceiveMessage(IPC::Connection& connectio
> 
> #if ENABLE(SERVICE_WORKER)
>     if (decoder.messageReceiverName() == Messages::WebSWServerConnection::messageReceiverName()) {
>-        auto iterator = m_swConnections.find(makeObjectIdentifier<SWServerConnectionIdentifierType>(decoder.destinationID()));
>-        if (iterator != m_swConnections.end())
>-            iterator->value->didReceiveMessage(connection, decoder);
>+        if (auto swConnection = m_swConnections.get(makeObjectIdentifier<SWServerConnectionIdentifierType>(decoder.destinationID())))
>+            swConnection->didReceiveMessage(connection, decoder);
>         return;
>     }
> 
>@@ -112,9 +115,8 @@ void StorageToWebProcessConnection::didReceiveSyncMessage(IPC::Connection& conne
> 
> #if ENABLE(SERVICE_WORKER)
>     if (decoder.messageReceiverName() == Messages::WebSWServerConnection::messageReceiverName()) {
>-        auto iterator = m_swConnections.find(makeObjectIdentifier<SWServerConnectionIdentifierType>(decoder.destinationID()));
>-        if (iterator != m_swConnections.end())
>-            iterator->value->didReceiveSyncMessage(connection, decoder, replyEncoder);
>+        if (auto swConnection = m_swConnections.get(makeObjectIdentifier<SWServerConnectionIdentifierType>(decoder.destinationID())))
>+            swConnection->didReceiveSyncMessage(connection, decoder, replyEncoder);
>         return;
>     }
> #endif
>@@ -143,15 +145,7 @@ void StorageToWebProcessConnection::didClose(IPC::Connection& connection)
> #endif
> 
> #if ENABLE(SERVICE_WORKER)
>-    Vector<std::unique_ptr<WebSWServerConnection>> connectionVector;
>-    connectionVector.reserveInitialCapacity(m_swConnections.size());
>-
>-    for (auto& connection : m_swConnections.values())
>-        connectionVector.uncheckedAppend(WTFMove(connection));
>-    for (auto& connection : connectionVector)
>-        connection->disconnectedFromWebProcess();
>-
>-    m_swConnections.clear();
>+    unregisterSWConnections();
> #endif
> }
> 
>@@ -161,6 +155,15 @@ void StorageToWebProcessConnection::didReceiveInvalidMessage(IPC::Connection&, I
> }
> 
> #if ENABLE(SERVICE_WORKER)
>+
>+void StorageToWebProcessConnection::unregisterSWConnections()
>+{
>+    for (auto& swConnection : m_swConnections.values()) {
>+        if (swConnection)
>+            swConnection->server().removeConnection(swConnection->identifier());
>+    }
>+}
>+
> void StorageToWebProcessConnection::establishSWServerConnection(SessionID sessionID, SWServerConnectionIdentifier& serverConnectionIdentifier)
> {
>     auto& server = StorageProcess::singleton().swServerForSession(sessionID);
>@@ -168,11 +171,12 @@ void StorageToWebProcessConnection::establishSWServerConnection(SessionID sessio
> 
>     serverConnectionIdentifier = connection->identifier();
>     LOG(ServiceWorker, "StorageToWebProcessConnection::establishSWServerConnection - %s", serverConnectionIdentifier.loggingString().utf8().data());
>-    ASSERT(!m_swConnections.contains(serverConnectionIdentifier));
> 
>-    auto addResult = m_swConnections.add(serverConnectionIdentifier, WTFMove(connection));
>-    ASSERT_UNUSED(addResult, addResult.isNewEntry);
>+    ASSERT(!m_swConnections.contains(serverConnectionIdentifier));
>+    m_swConnections.add(serverConnectionIdentifier, makeWeakPtr(*connection));
>+    server.addConnection(WTFMove(connection));
> }
>+
> #endif
> 
> #if ENABLE(INDEXED_DATABASE)
>diff --git a/Source/WebKit/StorageProcess/StorageToWebProcessConnection.h b/Source/WebKit/StorageProcess/StorageToWebProcessConnection.h
>index 71e27efc2df..b90767b769c 100644
>--- a/Source/WebKit/StorageProcess/StorageToWebProcessConnection.h
>+++ b/Source/WebKit/StorageProcess/StorageToWebProcessConnection.h
>@@ -70,7 +70,9 @@ private:
> 
> #if ENABLE(SERVICE_WORKER)
>     void establishSWServerConnection(PAL::SessionID, WebCore::SWServerConnectionIdentifier&);
>-    HashMap<WebCore::SWServerConnectionIdentifier, std::unique_ptr<WebSWServerConnection>> m_swConnections;
>+    void unregisterSWConnections();
>+
>+    HashMap<WebCore::SWServerConnectionIdentifier, WeakPtr<WebSWServerConnection>> m_swConnections;
> #endif
> 
>     Ref<IPC::Connection> m_connection;

Actions: View | Formatted Diff | Diff

Attachments on bug 186584: 342625 | 342626 | 342633 | 342636 | 342658 | 342685 | 342703 | 342704 | 342711 | 342716