Changeset 222684 in webkit


Ignore:
Timestamp:
Sep 30, 2017 3:50:16 PM (6 years ago)
Author:
Darin Adler
Message:

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.

Location:
trunk/Source/WebKit
Files:
12 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebKit/ChangeLog

    r222674 r222684  
     12017-09-30  Darin Adler  <darin@apple.com>
     2
     3        Have IPC::Connection::Client objects consistently invalidate the connection when destroyed
     4        https://bugs.webkit.org/show_bug.cgi?id=177708
     5
     6        Reviewed by Anders Carlsson.
     7
     8        I ran into an intermittent crash when running regression tests. It looked like a connection
     9        client was being called after it was destroyed. I did an audit of the all the connection
     10        clients to make sure they all invalidate their connection before they are destroyed.
     11
     12        * NetworkProcess/NetworkConnectionToWebProcess.cpp:
     13        (WebKit::NetworkConnectionToWebProcess::~NetworkConnectionToWebProcess): Invalidate the
     14        connection since this object opened the connection. There is no obvious
     15        guarantee that the connection will already be invalid when this is destroyed.
     16        * StorageProcess/StorageToWebProcessConnection.cpp:
     17        (WebKit::StorageToWebProcessConnection::~StorageToWebProcessConnection): Ditto.
     18        * UIProcess/Plugins/PluginProcessProxy.cpp:
     19        (WebKit::PluginProcessProxy::~PluginProcessProxy): Ditto.
     20        * WebProcess/Network/NetworkProcessConnection.cpp:
     21        (WebKit::NetworkProcessConnection::~NetworkProcessConnection): Ditto.
     22        * WebProcess/Storage/WebToStorageProcessConnection.cpp:
     23        (WebKit::WebToStorageProcessConnection::~WebToStorageProcessConnection): Ditto.
     24
     25        * StorageProcess/StorageToWebProcessConnection.h: Derive privately rather than publicly
     26        from IPC::Connection::Client because we can, and this means we don't have to study quite
     27        as much code to understand how this is used as a connection client.
     28        * WebProcess/Databases/IndexedDB/WebIDBConnectionToServer.h: Ditto.
     29        * WebProcess/Storage/WebToStorageProcessConnection.h: Ditto.
     30        * WebProcess/WebPage/WebInspector.h: Ditto.
     31        * WebProcess/WebPage/WebInspectorUI.h: Ditto.
     32
     33        * WebProcess/Databases/IndexedDB/WebIDBConnectionToServer.cpp:
     34        (WebKit::WebIDBConnectionToServer::WebIDBConnectionToServer): Added a comment about a
     35        reference cycle cycle leading to a leak that I believe exists here.
     36
    1372017-09-29  Alex Christensen  <achristensen@webkit.org>
    238
  • trunk/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp

    r222673 r222684  
    7878NetworkConnectionToWebProcess::~NetworkConnectionToWebProcess()
    7979{
     80    m_connection->invalidate();
    8081#if USE(LIBWEBRTC)
    8182    if (m_rtcProvider)
  • trunk/Source/WebKit/StorageProcess/StorageToWebProcessConnection.cpp

    r220977 r222684  
    5454StorageToWebProcessConnection::~StorageToWebProcessConnection()
    5555{
    56 
     56    m_connection->invalidate();
    5757}
    5858
  • trunk/Source/WebKit/StorageProcess/StorageToWebProcessConnection.h

    r220977 r222684  
    3838class WebSWServerConnection;
    3939
    40 class StorageToWebProcessConnection : public RefCounted<StorageToWebProcessConnection>, public IPC::Connection::Client, public IPC::MessageSender {
     40class StorageToWebProcessConnection : public RefCounted<StorageToWebProcessConnection>, private IPC::Connection::Client, private IPC::MessageSender {
    4141public:
    4242    static Ref<StorageToWebProcessConnection> create(IPC::Connection::Identifier);
  • trunk/Source/WebKit/UIProcess/Plugins/PluginProcessProxy.cpp

    r219050 r222684  
    8181PluginProcessProxy::~PluginProcessProxy()
    8282{
     83    if (m_connection)
     84        m_connection->invalidate();
     85
    8386    ASSERT(m_pendingFetchWebsiteDataRequests.isEmpty());
    8487    ASSERT(m_pendingFetchWebsiteDataCallbacks.isEmpty());
  • trunk/Source/WebKit/WebProcess/Databases/IndexedDB/WebIDBConnectionToServer.cpp

    r220857 r222684  
    6767
    6868    m_isOpenInServer = sendSync(Messages::StorageToWebProcessConnection::EstablishIDBConnectionToServer(sessionID), Messages::StorageToWebProcessConnection::EstablishIDBConnectionToServer::Reply(m_identifier));
     69
     70    // FIXME: This creates a reference cycle, so neither this object nor the IDBConnectionToServer will ever be deallocated.
    6971    m_connectionToServer = IDBClient::IDBConnectionToServer::create(*this);
    7072}
  • trunk/Source/WebKit/WebProcess/Databases/IndexedDB/WebIDBConnectionToServer.h

    r220887 r222684  
    3737class WebIDBResult;
    3838
    39 class WebIDBConnectionToServer final : public WebCore::IDBClient::IDBConnectionToServerDelegate, public IPC::MessageSender, public RefCounted<WebIDBConnectionToServer> {
     39class WebIDBConnectionToServer final : private WebCore::IDBClient::IDBConnectionToServerDelegate, private IPC::MessageSender, public RefCounted<WebIDBConnectionToServer> {
    4040public:
    4141    static Ref<WebIDBConnectionToServer> create(PAL::SessionID);
  • trunk/Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp

    r222613 r222684  
    6060NetworkProcessConnection::~NetworkProcessConnection()
    6161{
     62    m_connection->invalidate();
    6263}
    6364
  • trunk/Source/WebKit/WebProcess/Storage/WebToStorageProcessConnection.cpp

    r220977 r222684  
    4747WebToStorageProcessConnection::~WebToStorageProcessConnection()
    4848{
     49    m_connection->invalidate();
    4950}
    5051
  • trunk/Source/WebKit/WebProcess/Storage/WebToStorageProcessConnection.h

    r220977 r222684  
    4141namespace WebKit {
    4242
    43 class WebToStorageProcessConnection : public RefCounted<WebToStorageProcessConnection>, public IPC::Connection::Client, public IPC::MessageSender {
     43class WebToStorageProcessConnection : public RefCounted<WebToStorageProcessConnection>, private IPC::Connection::Client, private IPC::MessageSender {
    4444public:
    4545    static Ref<WebToStorageProcessConnection> create(IPC::Connection::Identifier connectionIdentifier)
  • trunk/Source/WebKit/WebProcess/WebPage/WebInspector.h

    r217924 r222684  
    3737class WebPage;
    3838
    39 class WebInspector : public API::ObjectImpl<API::Object::Type::BundleInspector>, public IPC::Connection::Client, public Inspector::FrontendChannel {
     39class WebInspector : public API::ObjectImpl<API::Object::Type::BundleInspector>, private IPC::Connection::Client, public Inspector::FrontendChannel {
    4040public:
    4141    static Ref<WebInspector> create(WebPage*);
  • trunk/Source/WebKit/WebProcess/WebPage/WebInspectorUI.h

    r212597 r222684  
    3939class WebPage;
    4040
    41 class WebInspectorUI : public RefCounted<WebInspectorUI>, public IPC::Connection::Client, public WebCore::InspectorFrontendClient {
     41class WebInspectorUI : public RefCounted<WebInspectorUI>, private IPC::Connection::Client, public WebCore::InspectorFrontendClient {
    4242public:
    4343    static Ref<WebInspectorUI> create(WebPage&);
Note: See TracChangeset for help on using the changeset viewer.