Changeset 232824 in webkit


Ignore:
Timestamp:
Jun 13, 2018 7:31:29 PM (6 years ago)
Author:
Chris Dumez
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:
Location:
trunk/Source
Files:
9 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r232823 r232824  
     12018-06-13  Chris Dumez  <cdumez@apple.com>
     2
     3        Crash under SWServer::unregisterConnection(Connection&)
     4        https://bugs.webkit.org/show_bug.cgi?id=186584
     5        <rdar://problem/40931680>
     6
     7        Reviewed by Youenn Fablet.
     8
     9        The crash was due to SWServer::Connection objects outliving their SWServer, even
     10        though SWServer::Connection::m_server is a C++ reference. This was possible because
     11        SWServer does not own the connections, StorageToWebProcessConnection does. This
     12        started crashing recently, after r232423, because SWServer can get destroyed now.
     13        The SWServer might get destroyed before the StorageToWebProcessConnection, in which
     14        case the SWServer::Connection objects will get destroyed later. We were crashing
     15        because the SWServer::Connection destructor tries to unregister the connection from
     16        the SWServer (which is dead).
     17
     18        To address the issue, the SWServer now owns the connections. StorageToWebProcessConnection
     19        merely has weak pointers to the connections.
     20
     21        * workers/service/server/SWServer.cpp:
     22        (WebCore::SWServer::Connection::Connection):
     23        (WebCore::SWServer::addConnection):
     24        (WebCore::SWServer::removeConnection):
     25        (WebCore::SWServer::resolveRegistrationReadyRequests):
     26        * workers/service/server/SWServer.h:
     27        (WebCore::SWServer::Connection::~Connection):
     28        (WebCore::SWServer::Connection::server):
     29        (WebCore::SWServer::connection):
     30        * workers/service/server/SWServerRegistration.cpp:
     31        (WebCore::SWServerRegistration::forEachConnection):
     32        (WebCore::SWServerRegistration::notifyClientsOfControllerChange):
     33        (WebCore::SWServerRegistration::controlClient):
     34
    1352018-06-13  Zalan Bujtas  <zalan@apple.com>
    236

  • trunk/Source/WebCore/workers/service/server/SWServer.cpp

    r232516 r232824  
    5555    , m_identifier(generateObjectIdentifier<SWServerConnectionIdentifierType>())
    5656{
    57     m_server.registerConnection(*this);
    58 }
    59 
    60 SWServer::Connection::~Connection()
    61 {
    62     m_server.unregisterConnection(*this);
    6357}
    6458
    … …  
    693687}
    694688
    695 void SWServer::registerConnection(Connection& connection)
    696 {
    697     auto result = m_connections.add(connection.identifier(), nullptr);
    698     ASSERT(result.isNewEntry);
    699     result.iterator->value = &connection;
    700 }
    701 
    702 void SWServer::unregisterConnection(Connection& connection)
    703 {
    704     ASSERT(m_connections.get(connection.identifier()) == &connection);
    705     m_connections.remove(connection.identifier());
     689void SWServer::addConnection(std::unique_ptr<Connection>&& connection)
     690{
     691    auto identifier = connection->identifier();
     692    ASSERT(!m_connections.contains(identifier));
     693    m_connections.add(identifier, WTFMove(connection));
     694}
     695
     696void SWServer::removeConnection(SWServerConnectionIdentifier connectionIdentifier)
     697{
     698    ASSERT(m_connections.contains(connectionIdentifier));
     699    m_connections.remove(connectionIdentifier);
    706700
    707701    for (auto& registration : m_registrations.values())
    708         registration->unregisterServerConnection(connection.identifier());
     702        registration->unregisterServerConnection(connectionIdentifier);
    709703
    710704    for (auto& jobQueue : m_jobQueues.values())
    711         jobQueue->cancelJobsFromConnection(connection.identifier());
     705        jobQueue->cancelJobsFromConnection(connectionIdentifier);
    712706}
    713707
    … …  
    818812void SWServer::resolveRegistrationReadyRequests(SWServerRegistration& registration)
    819813{
    820     for (auto* connection : m_connections.values())
     814    for (auto& connection : m_connections.values())
    821815        connection->resolveRegistrationReadyRequests(registration);
    822816}

  • trunk/Source/WebCore/workers/service/server/SWServer.h

    r232613 r232824  
    7070        friend class SWServer;
    7171    public:
    72         WEBCORE_EXPORT virtual ~Connection();
     72        WEBCORE_EXPORT virtual ~Connection() { }
    7373
    7474        using Identifier = SWServerConnectionIdentifier;
    … …  
    8888        virtual void registrationReady(uint64_t registrationReadyRequestIdentifier, ServiceWorkerRegistrationData&&) = 0;
    8989
     90        SWServer& server() { return m_server; }
     91
    9092    protected:
    9193        WEBCORE_EXPORT explicit Connection(SWServer&);
    92         SWServer& server() { return m_server; }
    9394
    9495        WEBCORE_EXPORT void finishFetchingScriptInServer(const ServiceWorkerFetchResult&);
    … …  
    145146    WEBCORE_EXPORT void markAllWorkersForOriginAsTerminated(const SecurityOriginData&);
    146147   
    147     Connection* getConnection(SWServerConnectionIdentifier identifier) { return m_connections.get(identifier); }
     148    WEBCORE_EXPORT void addConnection(std::unique_ptr<Connection>&&);
     149    WEBCORE_EXPORT void removeConnection(SWServerConnectionIdentifier);
     150    Connection* connection(SWServerConnectionIdentifier identifier) const { return m_connections.get(identifier); }
     151
    148152    SWOriginStore& originStore() { return m_originStore; }
    149153
    … …  
    180184
    181185private:
    182     void registerConnection(Connection&);
    183     void unregisterConnection(Connection&);
    184 
    185186    void scriptFetchFinished(Connection&, const ServiceWorkerFetchResult&);
    186187
    … …  
    209210    void terminateWorkerInternal(SWServerWorker&, TerminationMode);
    210211
    211     HashMap<SWServerConnectionIdentifier, Connection*> m_connections;
     212    HashMap<SWServerConnectionIdentifier, std::unique_ptr<Connection>> m_connections;
    212213    HashMap<ServiceWorkerRegistrationKey, std::unique_ptr<SWServerRegistration>> m_registrations;
    213214    HashMap<ServiceWorkerRegistrationIdentifier, SWServerRegistration*> m_registrationsByID;

  • trunk/Source/WebCore/workers/service/server/SWServerRegistration.cpp

    r229183 r232824  
    137137{
    138138    for (auto connectionIdentifierWithClients : m_connectionsWithClientRegistrations.values()) {
    139         if (auto* connection = m_server.getConnection(connectionIdentifierWithClients))
     139        if (auto* connection = m_server.connection(connectionIdentifierWithClients))
    140140            apply(*connection);
    141141    }
    … …  
    199199
    200200    for (auto& item : m_clientsUsingRegistration) {
    201         if (auto* connection = m_server.getConnection(item.key))
     201        if (auto* connection = m_server.connection(item.key))
    202202            connection->notifyClientsOfControllerChange(item.value, activeWorker()->data());
    203203    }
    … …  
    347347    HashSet<DocumentIdentifier> identifiers;
    348348    identifiers.add(identifier.contextIdentifier);
    349     m_server.getConnection(identifier.serverConnectionIdentifier)->notifyClientsOfControllerChange(identifiers, activeWorker()->data());
     349    m_server.connection(identifier.serverConnectionIdentifier)->notifyClientsOfControllerChange(identifiers, activeWorker()->data());
    350350}
    351351

  • trunk/Source/WebKit/ChangeLog

    r232817 r232824  
     12018-06-13  Chris Dumez  <cdumez@apple.com>
     2
     3        Crash under SWServer::unregisterConnection(Connection&)
     4        https://bugs.webkit.org/show_bug.cgi?id=186584
     5        <rdar://problem/40931680>
     6
     7        Reviewed by Youenn Fablet.
     8
     9        * StorageProcess/ServiceWorker/WebSWServerConnection.cpp:
     10        * StorageProcess/ServiceWorker/WebSWServerConnection.h:
     11        * StorageProcess/StorageToWebProcessConnection.cpp:
     12        (WebKit::StorageToWebProcessConnection::~StorageToWebProcessConnection):
     13        (WebKit::StorageToWebProcessConnection::didReceiveMessage):
     14        (WebKit::StorageToWebProcessConnection::didReceiveSyncMessage):
     15        (WebKit::StorageToWebProcessConnection::didClose):
     16        (WebKit::StorageToWebProcessConnection::unregisterSWConnections):
     17        (WebKit::StorageToWebProcessConnection::establishSWServerConnection):
     18        * StorageProcess/StorageToWebProcessConnection.h:
     19
    1202018-06-13  Dean Jackson  <dino@apple.com>
    221

  • trunk/Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp

    r232484 r232824  
    7777}
    7878
    79 void WebSWServerConnection::disconnectedFromWebProcess()
    80 {
    81     notImplemented();
    82 }
    83 
    8479void WebSWServerConnection::rejectJobInClient(ServiceWorkerJobIdentifier jobIdentifier, const ExceptionData& exceptionData)
    8580{

  • trunk/Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.h

    r232484 r232824  
    5757    IPC::Connection& ipcConnection() const { return m_contentConnection.get(); }
    5858
    59     void disconnectedFromWebProcess();
    6059    void didReceiveMessage(IPC::Connection&, IPC::Decoder&) final;
    6160    void didReceiveSyncMessage(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder>&);

  • trunk/Source/WebKit/StorageProcess/StorageToWebProcessConnection.cpp

    r229735 r232824  
    6262{
    6363    m_connection->invalidate();
     64
     65#if ENABLE(SERVICE_WORKER)
     66    unregisterSWConnections();
     67#endif
    6468}
    6569
    … …  
    8791#if ENABLE(SERVICE_WORKER)
    8892    if (decoder.messageReceiverName() == Messages::WebSWServerConnection::messageReceiverName()) {
    89         auto iterator = m_swConnections.find(makeObjectIdentifier<SWServerConnectionIdentifierType>(decoder.destinationID()));
    90         if (iterator != m_swConnections.end())
    91             iterator->value->didReceiveMessage(connection, decoder);
     93        if (auto swConnection = m_swConnections.get(makeObjectIdentifier<SWServerConnectionIdentifierType>(decoder.destinationID())))
     94            swConnection->didReceiveMessage(connection, decoder);
    9295        return;
    9396    }
    … …  
    113116#if ENABLE(SERVICE_WORKER)
    114117    if (decoder.messageReceiverName() == Messages::WebSWServerConnection::messageReceiverName()) {
    115         auto iterator = m_swConnections.find(makeObjectIdentifier<SWServerConnectionIdentifierType>(decoder.destinationID()));
    116         if (iterator != m_swConnections.end())
    117             iterator->value->didReceiveSyncMessage(connection, decoder, replyEncoder);
     118        if (auto swConnection = m_swConnections.get(makeObjectIdentifier<SWServerConnectionIdentifierType>(decoder.destinationID())))
     119            swConnection->didReceiveSyncMessage(connection, decoder, replyEncoder);
    118120        return;
    119121    }
    … …  
    144146
    145147#if ENABLE(SERVICE_WORKER)
    146     Vector<std::unique_ptr<WebSWServerConnection>> connectionVector;
    147     connectionVector.reserveInitialCapacity(m_swConnections.size());
    148 
    149     for (auto& connection : m_swConnections.values())
    150         connectionVector.uncheckedAppend(WTFMove(connection));
    151     for (auto& connection : connectionVector)
    152         connection->disconnectedFromWebProcess();
    153 
    154     m_swConnections.clear();
     148    unregisterSWConnections();
    155149#endif
    156150}
    … …  
    162156
    163157#if ENABLE(SERVICE_WORKER)
     158
     159void StorageToWebProcessConnection::unregisterSWConnections()
     160{
     161    auto swConnections = WTFMove(m_swConnections);
     162    for (auto& swConnection : swConnections.values()) {
     163        if (swConnection)
     164            swConnection->server().removeConnection(swConnection->identifier());
     165    }
     166}
     167
    164168void StorageToWebProcessConnection::establishSWServerConnection(SessionID sessionID, SWServerConnectionIdentifier& serverConnectionIdentifier)
    165169{
    … …  
    169173    serverConnectionIdentifier = connection->identifier();
    170174    LOG(ServiceWorker, "StorageToWebProcessConnection::establishSWServerConnection - %s", serverConnectionIdentifier.loggingString().utf8().data());
     175
    171176    ASSERT(!m_swConnections.contains(serverConnectionIdentifier));
    172 
    173     auto addResult = m_swConnections.add(serverConnectionIdentifier, WTFMove(connection));
    174     ASSERT_UNUSED(addResult, addResult.isNewEntry);
    175 }
     177    m_swConnections.add(serverConnectionIdentifier, makeWeakPtr(*connection));
     178    server.addConnection(WTFMove(connection));
     179}
     180
    176181#endif
    177182

  • trunk/Source/WebKit/StorageProcess/StorageToWebProcessConnection.h

    r229735 r232824  
    7171#if ENABLE(SERVICE_WORKER)
    7272    void establishSWServerConnection(PAL::SessionID, WebCore::SWServerConnectionIdentifier&);
    73     HashMap<WebCore::SWServerConnectionIdentifier, std::unique_ptr<WebSWServerConnection>> m_swConnections;
     73    void unregisterSWConnections();
     74
     75    HashMap<WebCore::SWServerConnectionIdentifier, WeakPtr<WebSWServerConnection>> m_swConnections;
    7476#endif
    7577
Note: See TracChangeset for help on using the changeset viewer.