Crash under SWServer::unregisterConnection(Connection&)
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 14 Jun 2018 02:31:29 +0000 (02:31 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 14 Jun 2018 02:31:29 +0000 (02:31 +0000)
https://bugs.webkit.org/show_bug.cgi?id=186584
<rdar://problem/40931680>

Reviewed by Youenn Fablet.

Source/WebCore:

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):

Source/WebKit:

* 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:

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

Source/WebCore/ChangeLog
Source/WebCore/workers/service/server/SWServer.cpp
Source/WebCore/workers/service/server/SWServer.h
Source/WebCore/workers/service/server/SWServerRegistration.cpp
Source/WebKit/ChangeLog
Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp
Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.h
Source/WebKit/StorageProcess/StorageToWebProcessConnection.cpp
Source/WebKit/StorageProcess/StorageToWebProcessConnection.h

index 9ce4c97..8b4845d 100644 (file)
@@ -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 Youenn Fablet.
+
+        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-13  Zalan Bujtas  <zalan@apple.com>
 
         [Mail] Use the Mail Viewer width as the base for resolving horizontal viewport units
index 08c31a7..0310001 100644 (file)
@@ -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);
 }
 
index 49aec18..c913094 100644 (file)
@@ -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) const { 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;
index 500f165..3a1b6d1 100644 (file)
@@ -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)
index cad6659..feb1d22 100644 (file)
@@ -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 Youenn Fablet.
+
+        * 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-13  Dean Jackson  <dino@apple.com>
 
         Disable AR support in WKWebView clients
index a941843..ff7ec59 100644 (file)
@@ -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));
index 9cf9046..2ca6625 100644 (file)
@@ -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>&);
 
index ae75c85..240df70 100644 (file)
@@ -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,16 @@ void StorageToWebProcessConnection::didReceiveInvalidMessage(IPC::Connection&, I
 }
 
 #if ENABLE(SERVICE_WORKER)
+
+void StorageToWebProcessConnection::unregisterSWConnections()
+{
+    auto swConnections = WTFMove(m_swConnections);
+    for (auto& swConnection : swConnections.values()) {
+        if (swConnection)
+            swConnection->server().removeConnection(swConnection->identifier());
+    }
+}
+
 void StorageToWebProcessConnection::establishSWServerConnection(SessionID sessionID, SWServerConnectionIdentifier& serverConnectionIdentifier)
 {
     auto& server = StorageProcess::singleton().swServerForSession(sessionID);
@@ -168,11 +172,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)
index 71e27ef..b90767b 100644 (file)
@@ -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;