Have IPC::Connection::Client objects consistently invalidate the conn…

…ection when destroyed

https://bugs.webkit.org/show_bug.cgi?id=177708

Reviewed by Anders Carlsson.

I ran into an intermittent crash when running regression tests. It looked like a connection
client was being called after it was destroyed. I did an audit of the all the connection
clients to make sure they all invalidate their connection before they are destroyed.

* NetworkProcess/NetworkConnectionToWebProcess.cpp:
(WebKit::NetworkConnectionToWebProcess::~NetworkConnectionToWebProcess): Invalidate the
connection since this object opened the connection. There is no obvious
guarantee that the connection will already be invalid when this is destroyed.
* StorageProcess/StorageToWebProcessConnection.cpp:
(WebKit::StorageToWebProcessConnection::~StorageToWebProcessConnection): Ditto.
* UIProcess/Plugins/PluginProcessProxy.cpp:
(WebKit::PluginProcessProxy::~PluginProcessProxy): Ditto.
* WebProcess/Network/NetworkProcessConnection.cpp:
(WebKit::NetworkProcessConnection::~NetworkProcessConnection): Ditto.
* WebProcess/Storage/WebToStorageProcessConnection.cpp:
(WebKit::WebToStorageProcessConnection::~WebToStorageProcessConnection): Ditto.

* StorageProcess/StorageToWebProcessConnection.h: Derive privately rather than publicly
from IPC::Connection::Client because we can, and this means we don't have to study quite
as much code to understand how this is used as a connection client.
* WebProcess/Databases/IndexedDB/WebIDBConnectionToServer.h: Ditto.
* WebProcess/Storage/WebToStorageProcessConnection.h: Ditto.
* WebProcess/WebPage/WebInspector.h: Ditto.
* WebProcess/WebPage/WebInspectorUI.h: Ditto.

* WebProcess/Databases/IndexedDB/WebIDBConnectionToServer.cpp:
(WebKit::WebIDBConnectionToServer::WebIDBConnectionToServer): Added a comment about a
reference cycle cycle leading to a leak that I believe exists here.


Canonical link: https://commits.webkit.org/193968@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@222684 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog

@@ -1,3 +1,39 @@
+2017-09-30  Darin Adler  <darin@apple.com>
+
+        Have IPC::Connection::Client objects consistently invalidate the connection when destroyed
+        https://bugs.webkit.org/show_bug.cgi?id=177708
+
+        Reviewed by Anders Carlsson.
+
+        I ran into an intermittent crash when running regression tests. It looked like a connection
+        client was being called after it was destroyed. I did an audit of the all the connection
+        clients to make sure they all invalidate their connection before they are destroyed.
+
+        * NetworkProcess/NetworkConnectionToWebProcess.cpp:
+        (WebKit::NetworkConnectionToWebProcess::~NetworkConnectionToWebProcess): Invalidate the
+        connection since this object opened the connection. There is no obvious
+        guarantee that the connection will already be invalid when this is destroyed.
+        * StorageProcess/StorageToWebProcessConnection.cpp:
+        (WebKit::StorageToWebProcessConnection::~StorageToWebProcessConnection): Ditto.
+        * UIProcess/Plugins/PluginProcessProxy.cpp:
+        (WebKit::PluginProcessProxy::~PluginProcessProxy): Ditto.
+        * WebProcess/Network/NetworkProcessConnection.cpp:
+        (WebKit::NetworkProcessConnection::~NetworkProcessConnection): Ditto.
+        * WebProcess/Storage/WebToStorageProcessConnection.cpp:
+        (WebKit::WebToStorageProcessConnection::~WebToStorageProcessConnection): Ditto.
+
+        * StorageProcess/StorageToWebProcessConnection.h: Derive privately rather than publicly
+        from IPC::Connection::Client because we can, and this means we don't have to study quite
+        as much code to understand how this is used as a connection client.
+        * WebProcess/Databases/IndexedDB/WebIDBConnectionToServer.h: Ditto.
+        * WebProcess/Storage/WebToStorageProcessConnection.h: Ditto.
+        * WebProcess/WebPage/WebInspector.h: Ditto.
+        * WebProcess/WebPage/WebInspectorUI.h: Ditto.
+
+        * WebProcess/Databases/IndexedDB/WebIDBConnectionToServer.cpp:
+        (WebKit::WebIDBConnectionToServer::WebIDBConnectionToServer): Added a comment about a
+        reference cycle cycle leading to a leak that I believe exists here.
+
 2017-09-29  Alex Christensen  <achristensen@webkit.org>
 
         REGRESSION: ASSERTION FAILED: m_provisionalURL.isEmpty() in WebKit::FrameLoadState::didStartProvisionalLoad

Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp

@@ -77,6 +77,7 @@ NetworkConnectionToWebProcess::NetworkConnectionToWebProcess(IPC::Connection::Id
 
 NetworkConnectionToWebProcess::~NetworkConnectionToWebProcess()
 {
+    m_connection->invalidate();
 #if USE(LIBWEBRTC)
     if (m_rtcProvider)
         m_rtcProvider->close();

Source/WebKit/StorageProcess/StorageToWebProcessConnection.cpp

@@ -53,7 +53,7 @@ StorageToWebProcessConnection::StorageToWebProcessConnection(IPC::Connection::Id
 
 StorageToWebProcessConnection::~StorageToWebProcessConnection()
 {
-
+    m_connection->invalidate();
 }
 
 void StorageToWebProcessConnection::didReceiveMessage(IPC::Connection& connection, IPC::Decoder& decoder)

Source/WebKit/StorageProcess/StorageToWebProcessConnection.h

@@ -37,7 +37,7 @@ namespace WebKit {
 class WebIDBConnectionToClient;
 class WebSWServerConnection;
 
-class StorageToWebProcessConnection : public RefCounted<StorageToWebProcessConnection>, public IPC::Connection::Client, public IPC::MessageSender {
+class StorageToWebProcessConnection : public RefCounted<StorageToWebProcessConnection>, private IPC::Connection::Client, private IPC::MessageSender {
 public:
     static Ref<StorageToWebProcessConnection> create(IPC::Connection::Identifier);
     ~StorageToWebProcessConnection();

Source/WebKit/UIProcess/Plugins/PluginProcessProxy.cpp

@@ -80,6 +80,9 @@ PluginProcessProxy::PluginProcessProxy(PluginProcessManager* PluginProcessManage
 
 PluginProcessProxy::~PluginProcessProxy()
 {
+    if (m_connection)
+        m_connection->invalidate();
+
     ASSERT(m_pendingFetchWebsiteDataRequests.isEmpty());
     ASSERT(m_pendingFetchWebsiteDataCallbacks.isEmpty());
     ASSERT(m_pendingDeleteWebsiteDataRequests.isEmpty());

Source/WebKit/WebProcess/Databases/IndexedDB/WebIDBConnectionToServer.cpp

@@ -66,6 +66,8 @@ WebIDBConnectionToServer::WebIDBConnectionToServer(PAL::SessionID sessionID)
     relaxAdoptionRequirement();
 
     m_isOpenInServer = sendSync(Messages::StorageToWebProcessConnection::EstablishIDBConnectionToServer(sessionID), Messages::StorageToWebProcessConnection::EstablishIDBConnectionToServer::Reply(m_identifier));
+
+    // FIXME: This creates a reference cycle, so neither this object nor the IDBConnectionToServer will ever be deallocated.
     m_connectionToServer = IDBClient::IDBConnectionToServer::create(*this);
 }
 

Source/WebKit/WebProcess/Databases/IndexedDB/WebIDBConnectionToServer.h

@@ -36,7 +36,7 @@ namespace WebKit {
 
 class WebIDBResult;
 
-class WebIDBConnectionToServer final : public WebCore::IDBClient::IDBConnectionToServerDelegate, public IPC::MessageSender, public RefCounted<WebIDBConnectionToServer> {
+class WebIDBConnectionToServer final : private WebCore::IDBClient::IDBConnectionToServerDelegate, private IPC::MessageSender, public RefCounted<WebIDBConnectionToServer> {
 public:
     static Ref<WebIDBConnectionToServer> create(PAL::SessionID);
 

Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp

@@ -59,6 +59,7 @@ NetworkProcessConnection::NetworkProcessConnection(IPC::Connection::Identifier c
 
 NetworkProcessConnection::~NetworkProcessConnection()
 {
+    m_connection->invalidate();
 }
 
 void NetworkProcessConnection::didReceiveMessage(IPC::Connection& connection, IPC::Decoder& decoder)

Source/WebKit/WebProcess/Storage/WebToStorageProcessConnection.cpp

@@ -46,6 +46,7 @@ WebToStorageProcessConnection::WebToStorageProcessConnection(IPC::Connection::Id
 
 WebToStorageProcessConnection::~WebToStorageProcessConnection()
 {
+    m_connection->invalidate();
 }
 
 void WebToStorageProcessConnection::didReceiveMessage(IPC::Connection& connection, IPC::Decoder& decoder)

Source/WebKit/WebProcess/Storage/WebToStorageProcessConnection.h

@@ -40,7 +40,7 @@ class SessionID;
 
 namespace WebKit {
 
-class WebToStorageProcessConnection : public RefCounted<WebToStorageProcessConnection>, public IPC::Connection::Client, public IPC::MessageSender {
+class WebToStorageProcessConnection : public RefCounted<WebToStorageProcessConnection>, private IPC::Connection::Client, private IPC::MessageSender {
 public:
     static Ref<WebToStorageProcessConnection> create(IPC::Connection::Identifier connectionIdentifier)
     {

Source/WebKit/WebProcess/WebPage/WebInspector.h

@@ -36,7 +36,7 @@ namespace WebKit {
 
 class WebPage;
 
-class WebInspector : public API::ObjectImpl<API::Object::Type::BundleInspector>, public IPC::Connection::Client, public Inspector::FrontendChannel {
+class WebInspector : public API::ObjectImpl<API::Object::Type::BundleInspector>, private IPC::Connection::Client, public Inspector::FrontendChannel {
 public:
     static Ref<WebInspector> create(WebPage*);
 

Source/WebKit/WebProcess/WebPage/WebInspectorUI.h

@@ -38,7 +38,7 @@ namespace WebKit {
 
 class WebPage;
 
-class WebInspectorUI : public RefCounted<WebInspectorUI>, public IPC::Connection::Client, public WebCore::InspectorFrontendClient {
+class WebInspectorUI : public RefCounted<WebInspectorUI>, private IPC::Connection::Client, public WebCore::InspectorFrontendClient {
 public:
     static Ref<WebInspectorUI> create(WebPage&);