- Revision
- 232824
- Author
- [email protected]
- Date
- 2018-06-13 19:31:29 -0700 (Wed, 13 Jun 2018)
Log Message
Crash under SWServer::unregisterConnection(Connection&)
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:
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (232823 => 232824)
--- trunk/Source/WebCore/ChangeLog 2018-06-14 01:36:24 UTC (rev 232823)
+++ trunk/Source/WebCore/ChangeLog 2018-06-14 02:31:29 UTC (rev 232824)
@@ -1,3 +1,37 @@
+2018-06-13 Chris Dumez <[email protected]>
+
+ 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 <[email protected]>
[Mail] Use the Mail Viewer width as the base for resolving horizontal viewport units
Modified: trunk/Source/WebCore/workers/service/server/SWServer.cpp (232823 => 232824)
--- trunk/Source/WebCore/workers/service/server/SWServer.cpp 2018-06-14 01:36:24 UTC (rev 232823)
+++ trunk/Source/WebCore/workers/service/server/SWServer.cpp 2018-06-14 02:31:29 UTC (rev 232824)
@@ -54,14 +54,8 @@
: m_server(server)
, m_identifier(generateObjectIdentifier<SWServerConnectionIdentifierType>())
{
- m_server.registerConnection(*this);
}
-SWServer::Connection::~Connection()
-{
- m_server.unregisterConnection(*this);
-}
-
HashSet<SWServer*>& SWServer::allServers()
{
static NeverDestroyed<HashSet<SWServer*>> servers;
@@ -692,23 +686,23 @@
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 @@
void SWServer::resolveRegistrationReadyRequests(SWServerRegistration& registration)
{
- for (auto* connection : m_connections.values())
+ for (auto& connection : m_connections.values())
connection->resolveRegistrationReadyRequests(registration);
}
Modified: trunk/Source/WebCore/workers/service/server/SWServer.h (232823 => 232824)
--- trunk/Source/WebCore/workers/service/server/SWServer.h 2018-06-14 01:36:24 UTC (rev 232823)
+++ trunk/Source/WebCore/workers/service/server/SWServer.h 2018-06-14 02:31:29 UTC (rev 232824)
@@ -69,7 +69,7 @@
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 @@
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 @@
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 @@
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 @@
};
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;
Modified: trunk/Source/WebCore/workers/service/server/SWServerRegistration.cpp (232823 => 232824)
--- trunk/Source/WebCore/workers/service/server/SWServerRegistration.cpp 2018-06-14 01:36:24 UTC (rev 232823)
+++ trunk/Source/WebCore/workers/service/server/SWServerRegistration.cpp 2018-06-14 02:31:29 UTC (rev 232824)
@@ -136,7 +136,7 @@
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 @@
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 @@
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)
Modified: trunk/Source/WebKit/ChangeLog (232823 => 232824)
--- trunk/Source/WebKit/ChangeLog 2018-06-14 01:36:24 UTC (rev 232823)
+++ trunk/Source/WebKit/ChangeLog 2018-06-14 02:31:29 UTC (rev 232824)
@@ -1,3 +1,22 @@
+2018-06-13 Chris Dumez <[email protected]>
+
+ 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 <[email protected]>
Disable AR support in WKWebView clients
Modified: trunk/Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp (232823 => 232824)
--- trunk/Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp 2018-06-14 01:36:24 UTC (rev 232823)
+++ trunk/Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp 2018-06-14 02:31:29 UTC (rev 232824)
@@ -76,11 +76,6 @@
server().unregisterServiceWorkerClient(keyValue.value, keyValue.key);
}
-void WebSWServerConnection::disconnectedFromWebProcess()
-{
- notImplemented();
-}
-
void WebSWServerConnection::rejectJobInClient(ServiceWorkerJobIdentifier jobIdentifier, const ExceptionData& exceptionData)
{
send(Messages::WebSWClientConnection::JobRejectedInServer(jobIdentifier, exceptionData));
Modified: trunk/Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.h (232823 => 232824)
--- trunk/Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.h 2018-06-14 01:36:24 UTC (rev 232823)
+++ trunk/Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.h 2018-06-14 02:31:29 UTC (rev 232824)
@@ -56,7 +56,6 @@
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>&);
Modified: trunk/Source/WebKit/StorageProcess/StorageToWebProcessConnection.cpp (232823 => 232824)
--- trunk/Source/WebKit/StorageProcess/StorageToWebProcessConnection.cpp 2018-06-14 01:36:24 UTC (rev 232823)
+++ trunk/Source/WebKit/StorageProcess/StorageToWebProcessConnection.cpp 2018-06-14 02:31:29 UTC (rev 232824)
@@ -61,6 +61,10 @@
StorageToWebProcessConnection::~StorageToWebProcessConnection()
{
m_connection->invalidate();
+
+#if ENABLE(SERVICE_WORKER)
+ unregisterSWConnections();
+#endif
}
void StorageToWebProcessConnection::didReceiveMessage(IPC::Connection& connection, IPC::Decoder& decoder)
@@ -86,9 +90,8 @@
#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 @@
#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 @@
#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 @@
}
#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 @@
serverConnectionIdentifier = connection->identifier();
LOG(ServiceWorker, "StorageToWebProcessConnection::establishSWServerConnection - %s", serverConnectionIdentifier.loggingString().utf8().data());
+
ASSERT(!m_swConnections.contains(serverConnectionIdentifier));
+ m_swConnections.add(serverConnectionIdentifier, makeWeakPtr(*connection));
+ server.addConnection(WTFMove(connection));
+}
- auto addResult = m_swConnections.add(serverConnectionIdentifier, WTFMove(connection));
- ASSERT_UNUSED(addResult, addResult.isNewEntry);
-}
#endif
#if ENABLE(INDEXED_DATABASE)
Modified: trunk/Source/WebKit/StorageProcess/StorageToWebProcessConnection.h (232823 => 232824)
--- trunk/Source/WebKit/StorageProcess/StorageToWebProcessConnection.h 2018-06-14 01:36:24 UTC (rev 232823)
+++ trunk/Source/WebKit/StorageProcess/StorageToWebProcessConnection.h 2018-06-14 02:31:29 UTC (rev 232824)
@@ -70,7 +70,9 @@
#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;
