WebKit Bugzilla
Attachment 342703 Details for
Bug 186584
: Crash under SWServer::unregisterConnection(Connection&)
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[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:
Alternative (SWServer owns the connections)
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;
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 186584
:
342625
|
342626
|
342633
|
342636
|
342658
|
342685
|
342703
|
342704
|
342711
|
342716