Changeset 64223 in webkit


Ignore:
Timestamp:
Jul 28, 2010 12:54:13 PM (14 years ago)
Author:
Adam Roben
Message:

Teach CoreIPC the right way to send large messages on Windows

r63776 added support for ::WriteFile failing with ERROR_IO_PENDING,
but it had a major flaw: we didn't ensure that the data being sent
(which is owned by the ArgumentEncoder) stayed around until the write
finished. We'd destroy the data immediately, leading to ::WriteFile
accessing that freed memory later. This seemed to always manifest
itself as a crash in ::WaitForMultipleObjects.

The correct solution (as hinted above) is to make sure that the data
being written is not destroyed until the write completes. When
::WriteFile fails with ERROR_IO_PENDING, we store the data being sent
in Connection::m_pendingWriteArguments, and don't send any more
messages until that write completes. We use an event in the OVERLAPPED
structure passed to ::WriteFile to detect when the write has completed
(similar to what we do for reads).

Fixes <​http://webkit.org/b/42785> <rdar://problem/8218522> Crash in
WebKit2WebProcess in WaitForMultipleObjects beneath
WorkQueue::workQueueThreadBody when running tests that produce a lot
of output

Reviewed by Anders Carlsson.

  • Platform/CoreIPC/Connection.cpp:

(CoreIPC::Connection::canSendOutgoingMessages): Added. This calls out
to a platform-specific function to allow each platform to have its own
policy for when messages can and can't be sent.
(CoreIPC::Connection::sendOutgoingMessages): Use the new
canSendOutgoingMessages to determine whether we can send any messages
right now. We now remove one message at a time from m_outgoingMessages
and send it. We stop sending messages when sendOutgoingMessage returns
false.

  • Platform/CoreIPC/Connection.h: Added m_pendingWriteArguments and

m_writeState on Windows.
(CoreIPC::Connection::Message::Message): Added this default
constructor.

  • Platform/CoreIPC/MessageID.h:

(CoreIPC::MessageID::MessageID): Made the default constructor public
for Message's benefit.

  • Platform/CoreIPC/mac/ConnectionMac.cpp:

(CoreIPC::Connection::platformCanSendOutgoingMessages): Added. Always
returns true.
(CoreIPC::Connection::sendOutgoingMessage): Changed to return a
boolean indicating whether more messages can be sent at this time.

  • Platform/CoreIPC/qt/ConnectionQt.cpp:

(CoreIPC::Connection::platformCanSendOutgoingMessages): Added. Returns
true if we have a socket.
(CoreIPC::Connection::sendOutgoingMessage): Changed a null-check of
m_socket to an assertion since it should be checked for null in
platformCanSendOutgoingMessages. Changed to return a boolean
indicating whether more messages can be sent at this time.

  • Platform/CoreIPC/win/ConnectionWin.cpp:

(CoreIPC::Connection::platformInitialize): Added initialization of
m_writeState.
(CoreIPC::Connection::platformInvalidate): Close m_writeState's event
handle.
(CoreIPC::Connection::writeEventHandler): Added. Checks if the pending
write has completed, cleans up our pending write state, and sends any
remaining messages.
(CoreIPC::Connection::open): Register our write event with the
WorkQueue so that writeEventHandler will be called when the event is
signaled.
(CoreIPC::Connection::platformCanSendOutgoingMessages): Added. We can
only send messages if there isn't a write pending.
(CoreIPC::Connection::sendOutgoingMessage): Changed to return a
boolean indicating whether more messages can be sent at this time. We
now pass m_writeState to ::WriteFile instead of an empty OVERLAPPED
struct so that our write event will be signaled when the write
completes. We also no longer pass a pointer to receive how many bytes
were written, as recommended by MSDN. If ::WriteFile fails with
ERROR_IO_PENDING, we save the ArgumentEncoder for this message and
return false to indicate that no more messages can be sent at this
time.

Location:
trunk/WebKit2
Files:
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/WebKit2/ChangeLog

    r64221 r64223  
     12010-07-28  Adam Roben  <aroben@apple.com>
     2
     3        Teach CoreIPC the right way to send large messages on Windows
     4
     5        r63776 added support for ::WriteFile failing with ERROR_IO_PENDING,
     6        but it had a major flaw: we didn't ensure that the data being sent
     7        (which is owned by the ArgumentEncoder) stayed around until the write
     8        finished. We'd destroy the data immediately, leading to ::WriteFile
     9        accessing that freed memory later. This seemed to always manifest
     10        itself as a crash in ::WaitForMultipleObjects.
     11
     12        The correct solution (as hinted above) is to make sure that the data
     13        being written is not destroyed until the write completes. When
     14        ::WriteFile fails with ERROR_IO_PENDING, we store the data being sent
     15        in Connection::m_pendingWriteArguments, and don't send any more
     16        messages until that write completes. We use an event in the OVERLAPPED
     17        structure passed to ::WriteFile to detect when the write has completed
     18        (similar to what we do for reads).
     19
     20        Fixes <http://webkit.org/b/42785> <rdar://problem/8218522> Crash in
     21        WebKit2WebProcess in WaitForMultipleObjects beneath
     22        WorkQueue::workQueueThreadBody when running tests that produce a lot
     23        of output
     24
     25        Reviewed by Anders Carlsson.
     26
     27        * Platform/CoreIPC/Connection.cpp:
     28        (CoreIPC::Connection::canSendOutgoingMessages): Added. This calls out
     29        to a platform-specific function to allow each platform to have its own
     30        policy for when messages can and can't be sent.
     31        (CoreIPC::Connection::sendOutgoingMessages): Use the new
     32        canSendOutgoingMessages to determine whether we can send any messages
     33        right now. We now remove one message at a time from m_outgoingMessages
     34        and send it. We stop sending messages when sendOutgoingMessage returns
     35        false.
     36
     37        * Platform/CoreIPC/Connection.h: Added m_pendingWriteArguments and
     38        m_writeState on Windows.
     39        (CoreIPC::Connection::Message::Message): Added this default
     40        constructor.
     41
     42        * Platform/CoreIPC/MessageID.h:
     43        (CoreIPC::MessageID::MessageID): Made the default constructor public
     44        for Message's benefit.
     45
     46        * Platform/CoreIPC/mac/ConnectionMac.cpp:
     47        (CoreIPC::Connection::platformCanSendOutgoingMessages): Added. Always
     48        returns true.
     49        (CoreIPC::Connection::sendOutgoingMessage): Changed to return a
     50        boolean indicating whether more messages can be sent at this time.
     51
     52        * Platform/CoreIPC/qt/ConnectionQt.cpp:
     53        (CoreIPC::Connection::platformCanSendOutgoingMessages): Added. Returns
     54        true if we have a socket.
     55        (CoreIPC::Connection::sendOutgoingMessage): Changed a null-check of
     56        m_socket to an assertion since it should be checked for null in
     57        platformCanSendOutgoingMessages. Changed to return a boolean
     58        indicating whether more messages can be sent at this time.
     59
     60        * Platform/CoreIPC/win/ConnectionWin.cpp:
     61        (CoreIPC::Connection::platformInitialize): Added initialization of
     62        m_writeState.
     63        (CoreIPC::Connection::platformInvalidate): Close m_writeState's event
     64        handle.
     65        (CoreIPC::Connection::writeEventHandler): Added. Checks if the pending
     66        write has completed, cleans up our pending write state, and sends any
     67        remaining messages.
     68        (CoreIPC::Connection::open): Register our write event with the
     69        WorkQueue so that writeEventHandler will be called when the event is
     70        signaled.
     71        (CoreIPC::Connection::platformCanSendOutgoingMessages): Added. We can
     72        only send messages if there isn't a write pending.
     73        (CoreIPC::Connection::sendOutgoingMessage): Changed to return a
     74        boolean indicating whether more messages can be sent at this time. We
     75        now pass m_writeState to ::WriteFile instead of an empty OVERLAPPED
     76        struct so that our write event will be signaled when the write
     77        completes. We also no longer pass a pointer to receive how many bytes
     78        were written, as recommended by MSDN. If ::WriteFile fails with
     79        ERROR_IO_PENDING, we save the ArgumentEncoder for this message and
     80        return false to indicate that no more messages can be sent at this
     81        time.
     82
    1832010-07-28  Adam Roben  <aroben@apple.com>
    284

  • trunk/WebKit2/Platform/CoreIPC/Connection.cpp

    r63916 r64223  
    208208}
    209209
     210bool Connection::canSendOutgoingMessages() const
     211{
     212    return m_isConnected && platformCanSendOutgoingMessages();
     213}
     214
    210215void Connection::sendOutgoingMessages()
    211216{
    212     if (!m_isConnected)
     217    if (!canSendOutgoingMessages())
    213218        return;
    214219
    215     Vector<OutgoingMessage> outgoingMessages;
    216 
    217     {
    218         MutexLocker locker(m_outgoingMessagesLock);
    219         m_outgoingMessages.swap(outgoingMessages);
    220     }
    221 
    222     // Send messages.
    223     for (size_t i = 0; i < outgoingMessages.size(); ++i)
    224         sendOutgoingMessage(outgoingMessages[i].messageID(), adoptPtr(outgoingMessages[i].arguments()));
     220    while (true) {
     221        OutgoingMessage message;
     222        {
     223            MutexLocker locker(m_outgoingMessagesLock);
     224            if (m_outgoingMessages.isEmpty())
     225                break;
     226            message = m_outgoingMessages.takeFirst();
     227        }
     228
     229        if (!sendOutgoingMessage(message.messageID(), adoptPtr(message.arguments())))
     230            break;
     231    }
    225232}
    226233

  • trunk/WebKit2/Platform/CoreIPC/Connection.h

    r63148 r64223  
    9393    template<typename T> class Message {
    9494    public:
     95        Message()
     96            : m_arguments(0)
     97        {
     98        }
     99
    95100        Message(MessageID messageID, PassOwnPtr<T> arguments)
    96101            : m_messageID(messageID)
    … …  
    127132    // Called on the connection work queue.
    128133    void processIncomingMessage(MessageID, PassOwnPtr<ArgumentDecoder>);
     134    bool canSendOutgoingMessages() const;
     135    bool platformCanSendOutgoingMessages() const;
    129136    void sendOutgoingMessages();
    130     void sendOutgoingMessage(MessageID, PassOwnPtr<ArgumentEncoder>);
     137    bool sendOutgoingMessage(MessageID, PassOwnPtr<ArgumentEncoder>);
    131138    void connectionDidClose();
    132139   
    … …  
    151158    // Outgoing messages.
    152159    Mutex m_outgoingMessagesLock;
    153     Vector<OutgoingMessage> m_outgoingMessages;
     160    Deque<OutgoingMessage> m_outgoingMessages;
    154161   
    155162    ThreadCondition m_waitForMessageCondition;
    … …  
    167174    // Called on the connection queue.
    168175    void readEventHandler();
     176    void writeEventHandler();
    169177
    170178    Vector<uint8_t> m_readBuffer;
    171179    OVERLAPPED m_readState;
     180    OwnPtr<ArgumentEncoder> m_pendingWriteArguments;
     181    OVERLAPPED m_writeState;
    172182    HANDLE m_connectionPipe;
    173183#elif PLATFORM(QT)

  • trunk/WebKit2/Platform/CoreIPC/MessageID.h

    r61718 r64223  
    6868    };
    6969
     70    MessageID()
     71        : m_messageID(0)
     72    {
     73    }
     74
    7075    template <typename EnumType>
    7176    explicit MessageID(EnumType messageKind, unsigned char flags = 0)
    … …  
    114119    unsigned char getClass() const { return (m_messageID & 0x00ff0000) >> 16; }
    115120
    116     MessageID()
    117         : m_messageID(0)
    118     {
    119     }
    120 
    121121    unsigned m_messageID;
    122122};

  • trunk/WebKit2/Platform/CoreIPC/mac/ConnectionMac.cpp

    r63708 r64223  
    116116}
    117117
    118 void Connection::sendOutgoingMessage(MessageID messageID, PassOwnPtr<ArgumentEncoder> arguments)
     118bool Connection::platformCanSendOutgoingMessages() const
     119{
     120    return true;
     121}
     122
     123bool Connection::sendOutgoingMessage(MessageID messageID, PassOwnPtr<ArgumentEncoder> arguments)
    119124{
    120125    Vector<Attachment> attachments = arguments->releaseAttachments();
    … …  
    200205    if (kr != KERN_SUCCESS) {
    201206        // FIXME: What should we do here?
    202         return;
    203     }
     207    }
     208
     209    return true;
    204210}
    205211

  • trunk/WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp

    r63148 r64223  
    105105}
    106106
    107 void Connection::sendOutgoingMessage(MessageID messageID, PassOwnPtr<ArgumentEncoder> arguments)
     107bool Connection::platformCanSendOutgoingMessages() const
    108108{
    109     if (!m_socket)
    110         return;
     109    return m_socket;
     110}
     111
     112bool Connection::sendOutgoingMessage(MessageID messageID, PassOwnPtr<ArgumentEncoder> arguments)
     113{
     114    ASSERT(m_socket);
    111115
    112116    // We put the message ID last.
    … …  
    122126
    123127    ASSERT(bytesWritten == arguments->bufferSize());
     128    return true;
    124129}
    125130

  • trunk/WebKit2/Platform/CoreIPC/win/ConnectionWin.cpp

    r64221 r64223  
    8383{
    8484    memset(&m_readState, 0, sizeof(m_readState));
    85     m_readState.hEvent = ::CreateEventW(0, false, false, 0);
     85    m_readState.hEvent = ::CreateEventW(0, FALSE, FALSE, 0);
     86
     87    memset(&m_writeState, 0, sizeof(m_writeState));
     88    m_writeState.hEvent = ::CreateEventW(0, FALSE, FALSE, 0);
    8689
    8790    m_connectionPipe = identifier;
    … …  
    97100    ::CloseHandle(m_readState.hEvent);
    98101    m_readState.hEvent = 0;
     102
     103    ::CloseHandle(m_writeState.hEvent);
     104    m_writeState.hEvent = 0;
    99105
    100106    ::CloseHandle(m_connectionPipe);
    … …  
    228234}
    229235
     236void Connection::writeEventHandler()
     237{
     238    DWORD numberOfBytesWritten = 0;
     239    if (!::GetOverlappedResult(m_connectionPipe, &m_writeState, &numberOfBytesWritten, FALSE)) {
     240        DWORD error = ::GetLastError();
     241        ASSERT_NOT_REACHED();
     242    }
     243
     244    // The pending write has finished, so we are now done with its arguments. Clearing this member
     245    // will allow us to send messages again.
     246    m_pendingWriteArguments = 0;
     247
     248    // Now that the pending write has finished, we can try to send a new message.
     249    sendOutgoingMessages();
     250}
     251
    230252bool Connection::open()
    231253{
    232     // Start listening for read state events.
     254    // Start listening for read and write state events.
    233255    m_connectionQueue.registerHandle(m_readState.hEvent, WorkItem::create(this, &Connection::readEventHandler));
     256    m_connectionQueue.registerHandle(m_writeState.hEvent, WorkItem::create(this, &Connection::writeEventHandler));
    234257
    235258    if (m_isServer) {
    … …  
    262285}
    263286
    264 void Connection::sendOutgoingMessage(MessageID messageID, PassOwnPtr<ArgumentEncoder> arguments)
    265 {
     287bool Connection::platformCanSendOutgoingMessages() const
     288{
     289    // We only allow sending one asynchronous message at a time. If we wanted to send more than one
     290    // at once, we'd have to use multiple OVERLAPPED structures and hold onto multiple pending
     291    // ArgumentEncoders (one of each for each simultaneous asynchronous message).
     292    return !m_pendingWriteArguments;
     293}
     294
     295bool Connection::sendOutgoingMessage(MessageID messageID, PassOwnPtr<ArgumentEncoder> arguments)
     296{
     297    ASSERT(!m_pendingWriteArguments);
     298
    266299    // Just bail if the handle has been closed.
    267300    if (m_connectionPipe == INVALID_HANDLE_VALUE)
    268         return;
     301        return false;
    269302
    270303    // We put the message ID last.
    … …  
    272305
    273306    // Write the outgoing message.
    274     OVERLAPPED overlapped = { 0 };
    275 
    276     DWORD bytesWritten;
    277     if (::WriteFile(m_connectionPipe, arguments->buffer(), arguments->bufferSize(), &bytesWritten, &overlapped)) {
     307
     308    if (::WriteFile(m_connectionPipe, arguments->buffer(), arguments->bufferSize(), 0, &m_writeState)) {
    278309        // We successfully sent this message.
    279         return;
     310        return true;
    280311    }
    281312
    282313    DWORD error = ::GetLastError();
    283     if (error == ERROR_IO_PENDING) {
    284         // The message will be sent soon.
    285         return;
    286     }
    287 
    288     ASSERT_NOT_REACHED();
     314    if (error != ERROR_IO_PENDING) {
     315        ASSERT_NOT_REACHED();
     316        return false;
     317    }
     318
     319    // The message will be sent soon. Hold onto the arguments so that they won't be destroyed
     320    // before the write completes.
     321    m_pendingWriteArguments = arguments;
     322
     323    // We can only send one asynchronous message at a time (see comment in platformCanSendOutgoingMessages).
     324    return false;
    289325}
    290326
Note: See TracChangeset for help on using the changeset viewer.