Changeset 213030 in webkit


Ignore:
Timestamp:
Feb 26, 2017 11:50:31 PM (7 years ago)
Author:
Carlos Garcia Campos
Message:

[GTK] Hangs when showing Google search results
​https://bugs.webkit.org/show_bug.cgi?id=168699

Reviewed by Žan Doberšek.

Connection::sendOutgoingMessage() can poll forever if sendmsg fails with EAGAIN or EWOULDBLOCK. For example if
socket read buffers are full, poll will be blocked until we read the pending data, but we can't read because
the thread is blocked in the poll. In case of EAGAIN/EWOULDBLOCK we should poll using the run loop, to allow
reads to happen in thread while we wait for the socket to be writable again. In the GTK+ port we use
GSocketMonitor to poll socket file descriptor without blocking, using the run loop. This patch renames the
socket monitor as readSocketMonitor and adds another one for polling output. When sendmsg fails with
EAGAIN/EWOULDBLOCK, the pending message is saved and the write monitor starts polling. Once the socket is
writable again we send the pending message. Helper class MessageInfo and a new one UnixMessage have been moved
to its own header file to be able to use std::unique_ptr member to save the pending message.

  • Platform/IPC/Connection.cpp: Include UnixMessage.h as required by std::unique_ptr.
  • Platform/IPC/Connection.h: Add write socket monitor and also keep the GSocket as a member to reuse it.
  • Platform/IPC/glib/GSocketMonitor.cpp: Use Function instead of std::function.

(IPC::GSocketMonitor::start):

  • Platform/IPC/glib/GSocketMonitor.h:
  • Platform/IPC/unix/ConnectionUnix.cpp:

(IPC::Connection::platformInitialize): Initialize the GSocket here since we rely on it to take the ownership of
the descriptor. We were leaking it if the connection was invalidated without being opened.
(IPC::Connection::platformInvalidate): Destroy the GSocket even when not connected. Also stop the write monitor.
(IPC::Connection::processMessage):
(IPC::Connection::open):
(IPC::Connection::platformCanSendOutgoingMessages): Return false if we have a pending message to ensure
Connection doesn't try to send more messages until the pending message is dispatched. We don't need to check
m_isConnected because the caller already checks that.
(IPC::Connection::sendOutgoingMessage): Split it in two. This creates and prepares a UnixMessage and then calls
sendOutputMessage() to do the rest.
(IPC::Connection::sendOutputMessage): Send the message, or save it if sendmsg fails with EAGAIN or EWOULDBLOCK
to be sent later when the socket is writable.

  • Platform/IPC/unix/UnixMessage.h: Added.

(IPC::MessageInfo::MessageInfo):
(IPC::MessageInfo::setMessageBodyIsOutOfLine):
(IPC::MessageInfo::isMessageBodyIsOutOfLine):
(IPC::MessageInfo::bodySize):
(IPC::MessageInfo::attachmentCount):
(IPC::UnixMessage::UnixMessage):
(IPC::UnixMessage::~UnixMessage):
(IPC::UnixMessage::attachments):
(IPC::UnixMessage::messageInfo):
(IPC::UnixMessage::body):
(IPC::UnixMessage::bodySize):
(IPC::UnixMessage::appendAttachment):

  • PlatformGTK.cmake:
Location:
trunk/Source/WebKit2
Files:
1 added
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit2/ChangeLog

    r213025 r213030  
     12017-02-26  Carlos Garcia Campos  <cgarcia@igalia.com>
     2
     3        [GTK] Hangs when showing Google search results
     4        https://bugs.webkit.org/show_bug.cgi?id=168699
     5
     6        Reviewed by Žan Doberšek.
     7
     8        Connection::sendOutgoingMessage() can poll forever if sendmsg fails with EAGAIN or EWOULDBLOCK. For example if
     9        socket read buffers are full, poll will be blocked until we read the pending data, but we can't read because
     10        the thread is blocked in the poll. In case of EAGAIN/EWOULDBLOCK we should poll using the run loop, to allow
     11        reads to happen in thread while we wait for the socket to be writable again. In the GTK+ port we use
     12        GSocketMonitor to poll socket file descriptor without blocking, using the run loop. This patch renames the
     13        socket monitor as readSocketMonitor and adds another one for polling output. When sendmsg fails with
     14        EAGAIN/EWOULDBLOCK, the pending message is saved and the write monitor starts polling. Once the socket is
     15        writable again we send the pending message. Helper class MessageInfo and a new one UnixMessage have been moved
     16        to its own header file to be able to use std::unique_ptr member to save the pending message.
     17
     18        * Platform/IPC/Connection.cpp: Include UnixMessage.h as required by std::unique_ptr.
     19        * Platform/IPC/Connection.h: Add write socket monitor and also keep the GSocket as a member to reuse it.
     20        * Platform/IPC/glib/GSocketMonitor.cpp: Use Function instead of std::function.
     21        (IPC::GSocketMonitor::start):
     22        * Platform/IPC/glib/GSocketMonitor.h:
     23        * Platform/IPC/unix/ConnectionUnix.cpp:
     24        (IPC::Connection::platformInitialize): Initialize the GSocket here since we rely on it to take the ownership of
     25        the descriptor. We were leaking it if the connection was invalidated without being opened.
     26        (IPC::Connection::platformInvalidate): Destroy the GSocket even when not connected. Also stop the write monitor.
     27        (IPC::Connection::processMessage):
     28        (IPC::Connection::open):
     29        (IPC::Connection::platformCanSendOutgoingMessages): Return false if we have a pending message to ensure
     30        Connection doesn't try to send more messages until the pending message is dispatched. We don't need to check
     31        m_isConnected because the caller already checks that.
     32        (IPC::Connection::sendOutgoingMessage): Split it in two. This creates and prepares a UnixMessage and then calls
     33        sendOutputMessage() to do the rest.
     34        (IPC::Connection::sendOutputMessage): Send the message, or save it if sendmsg fails with EAGAIN or EWOULDBLOCK
     35        to be sent later when the socket is writable.
     36        * Platform/IPC/unix/UnixMessage.h: Added.
     37        (IPC::MessageInfo::MessageInfo):
     38        (IPC::MessageInfo::setMessageBodyIsOutOfLine):
     39        (IPC::MessageInfo::isMessageBodyIsOutOfLine):
     40        (IPC::MessageInfo::bodySize):
     41        (IPC::MessageInfo::attachmentCount):
     42        (IPC::UnixMessage::UnixMessage):
     43        (IPC::UnixMessage::~UnixMessage):
     44        (IPC::UnixMessage::attachments):
     45        (IPC::UnixMessage::messageInfo):
     46        (IPC::UnixMessage::body):
     47        (IPC::UnixMessage::bodySize):
     48        (IPC::UnixMessage::appendAttachment):
     49        * PlatformGTK.cmake:
     50
    1512017-02-26  Devin Rousso  <dcrousso+webkit@gmail.com>
    252

  • trunk/Source/WebKit2/Platform/IPC/Connection.cpp

    r211482 r213030  
    4040#endif
    4141
     42#if USE(UNIX_DOMAIN_SOCKETS)
     43#include "UnixMessage.h"
     44#endif
     45
    4246namespace IPC {
    4347

  • trunk/Source/WebKit2/Platform/IPC/Connection.h

    r209575 r213030  
    8080
    8181class MachMessage;
     82class UnixMessage;
    8283
    8384class Connection : public ThreadSafeRefCounted<Connection> {
    … …  
    309310    void readyReadHandler();
    310311    bool processMessage();
     312    bool sendOutputMessage(UnixMessage&);
    311313
    312314    Vector<uint8_t> m_readBuffer;
    313315    Vector<int> m_fileDescriptors;
    314316    int m_socketDescriptor;
     317    std::unique_ptr<UnixMessage> m_pendingOutputMessage;
    315318#if PLATFORM(GTK)
    316     GSocketMonitor m_socketMonitor;
     319    GRefPtr<GSocket> m_socket;
     320    GSocketMonitor m_readSocketMonitor;
     321    GSocketMonitor m_writeSocketMonitor;
    317322#endif
    318323#elif OS(DARWIN)

  • trunk/Source/WebKit2/Platform/IPC/glib/GSocketMonitor.cpp

    r194496 r213030  
    4343}
    4444
    45 void GSocketMonitor::start(GSocket* socket, GIOCondition condition, RunLoop& runLoop, std::function<gboolean (GIOCondition)>&& callback)
     45void GSocketMonitor::start(GSocket* socket, GIOCondition condition, RunLoop& runLoop, Function<gboolean (GIOCondition)>&& callback)
    4646{
    4747    stop();

  • trunk/Source/WebKit2/Platform/IPC/glib/GSocketMonitor.h

    r191881 r213030  
    4444    ~GSocketMonitor();
    4545
    46     void start(GSocket*, GIOCondition, RunLoop&, std::function<gboolean (GIOCondition)>&&);
     46    void start(GSocket*, GIOCondition, RunLoop&, Function<gboolean (GIOCondition)>&&);
    4747    void stop();
    4848
    … …  
    5252    GRefPtr<GSource> m_source;
    5353    GRefPtr<GCancellable> m_cancellable;
    54     std::function<gboolean (GIOCondition)> m_callback;
     54    Function<gboolean (GIOCondition)> m_callback;
    5555};
    5656

  • trunk/Source/WebKit2/Platform/IPC/unix/ConnectionUnix.cpp

    r212557 r213030  
    3131#include "DataReference.h"
    3232#include "SharedMemory.h"
     33#include "UnixMessage.h"
    3334#include <sys/socket.h>
    3435#include <unistd.h>
    … …  
    6162static const size_t attachmentMaxAmount = 255;
    6263
    63 enum {
    64     MessageBodyIsOutOfLine = 1U << 31
    65 };
    66 
    67 class MessageInfo {
    68 public:
    69     MessageInfo() { }
    70 
    71     MessageInfo(size_t bodySize, size_t initialAttachmentCount)
    72         : m_bodySize(bodySize)
    73         , m_attachmentCount(initialAttachmentCount)
    74         , m_isMessageBodyOutOfLine(false)
    75     {
    76     }
    77 
    78     void setMessageBodyIsOutOfLine()
    79     {
    80         ASSERT(!isMessageBodyIsOutOfLine());
    81 
    82         m_isMessageBodyOutOfLine = true;
    83         m_attachmentCount++;
    84     }
    85 
    86     bool isMessageBodyIsOutOfLine() const { return m_isMessageBodyOutOfLine; }
    87 
    88     size_t bodySize() const { return m_bodySize; }
    89 
    90     size_t attachmentCount() const { return m_attachmentCount; }
    91 
    92 private:
    93     size_t m_bodySize;
    94     size_t m_attachmentCount;
    95     bool m_isMessageBodyOutOfLine;
    96 };
    97 
    9864class AttachmentInfo {
    9965    WTF_MAKE_FAST_ALLOCATED;
    10066public:
    101     AttachmentInfo()
    102         : m_type(Attachment::Uninitialized)
    103         , m_size(0)
    104         , m_isNull(false)
    105     {
    106     }
     67    AttachmentInfo() = default;
    10768
    10869    void setType(Attachment::Type type) { m_type = type; }
    109     Attachment::Type getType() { return m_type; }
     70    Attachment::Type type() const { return m_type; }
    11071    void setSize(size_t size)
    11172    {
    … …  
    11475    }
    11576
    116     size_t getSize()
     77    size_t size() const
    11778    {
    11879        ASSERT(m_type == Attachment::MappedMemoryType);
    … …  
    12283    // The attachment is not null unless explicitly set.
    12384    void setNull() { m_isNull = true; }
    124     bool isNull() { return m_isNull; }
     85    bool isNull() const { return m_isNull; }
    12586
    12687private:
    127     Attachment::Type m_type;
    128     size_t m_size;
    129     bool m_isNull;
     88    Attachment::Type m_type { Attachment::Uninitialized };
     89    size_t m_size { 0 };
     90    bool m_isNull { false };
    13091};
    13192
    … …  
    13394{
    13495    m_socketDescriptor = identifier;
     96#if PLATFORM(GTK)
     97    m_socket = adoptGRef(g_socket_new_from_fd(m_socketDescriptor, nullptr));
     98#endif
    13599    m_readBuffer.reserveInitialCapacity(messageMaxSize);
    136100    m_fileDescriptors.reserveInitialCapacity(attachmentMaxAmount);
    … …  
    139103void Connection::platformInvalidate()
    140104{
    141     // In GTK+ platform the socket is closed by the work queue.
    142 #if !PLATFORM(GTK)
     105#if PLATFORM(GTK)
     106    // In GTK+ platform the socket descriptor is owned by GSocket.
     107    m_socket = nullptr;
     108#else
    143109    if (m_socketDescriptor != -1)
    144110        closeWithRetry(m_socketDescriptor);
    … …  
    149115
    150116#if PLATFORM(GTK)
    151     m_socketMonitor.stop();
     117    m_readSocketMonitor.stop();
     118    m_writeSocketMonitor.stop();
    152119#endif
    153120
    … …  
    166133    messageData += sizeof(messageInfo);
    167134
    168     size_t messageLength = sizeof(MessageInfo) + messageInfo.attachmentCount() * sizeof(AttachmentInfo) + (messageInfo.isMessageBodyIsOutOfLine() ? 0 : messageInfo.bodySize());
     135    size_t messageLength = sizeof(MessageInfo) + messageInfo.attachmentCount() * sizeof(AttachmentInfo) + (messageInfo.isBodyOutOfLine() ? 0 : messageInfo.bodySize());
    169136    if (m_readBuffer.size() < messageLength)
    170137        return false;
    … …  
    180147
    181148        for (size_t i = 0; i < attachmentCount; ++i) {
    182             switch (attachmentInfo[i].getType()) {
     149            switch (attachmentInfo[i].type()) {
    183150            case Attachment::MappedMemoryType:
    184151            case Attachment::SocketType:
    … …  
    192159        }
    193160
    194         if (messageInfo.isMessageBodyIsOutOfLine())
     161        if (messageInfo.isBodyOutOfLine())
    195162            attachmentCount--;
    196163    }
    … …  
    202169    for (size_t i = 0; i < attachmentCount; ++i) {
    203170        int fd = -1;
    204         switch (attachmentInfo[i].getType()) {
     171        switch (attachmentInfo[i].type()) {
    205172        case Attachment::MappedMemoryType:
    206173            if (!attachmentInfo[i].isNull())
    207174                fd = m_fileDescriptors[fdIndex++];
    208             attachments[attachmentCount - i - 1] = Attachment(fd, attachmentInfo[i].getSize());
     175            attachments[attachmentCount - i - 1] = Attachment(fd, attachmentInfo[i].size());
    209176            break;
    210177        case Attachment::SocketType:
    … …  
    220187    }
    221188
    222     if (messageInfo.isMessageBodyIsOutOfLine()) {
     189    if (messageInfo.isBodyOutOfLine()) {
    223190        ASSERT(messageInfo.bodySize());
    224191
    … …  
    229196
    230197        WebKit::SharedMemory::Handle handle;
    231         handle.adoptAttachment(IPC::Attachment(m_fileDescriptors[attachmentFileDescriptorCount - 1], attachmentInfo[attachmentCount].getSize()));
     198        handle.adoptAttachment(IPC::Attachment(m_fileDescriptors[attachmentFileDescriptorCount - 1], attachmentInfo[attachmentCount].size()));
    232199
    233200        oolMessageBody = WebKit::SharedMemory::map(handle, WebKit::SharedMemory::Protection::ReadOnly);
    … …  
    238205    }
    239206
    240     ASSERT(attachments.size() == (messageInfo.isMessageBodyIsOutOfLine() ? messageInfo.attachmentCount() - 1 : messageInfo.attachmentCount()));
     207    ASSERT(attachments.size() == (messageInfo.isBodyOutOfLine() ? messageInfo.attachmentCount() - 1 : messageInfo.attachmentCount()));
    241208
    242209    uint8_t* messageBody = messageData;
    243     if (messageInfo.isMessageBodyIsOutOfLine())
     210    if (messageInfo.isBodyOutOfLine())
    244211        messageBody = reinterpret_cast<uint8_t*>(oolMessageBody->data());
    245212
    … …  
    366333    m_isConnected = true;
    367334#if PLATFORM(GTK)
    368     GRefPtr<GSocket> socket = adoptGRef(g_socket_new_from_fd(m_socketDescriptor, nullptr));
    369     m_socketMonitor.start(socket.get(), G_IO_IN, m_connectionQueue->runLoop(), [protectedThis] (GIOCondition condition) -> gboolean {
     335    m_readSocketMonitor.start(m_socket.get(), G_IO_IN, m_connectionQueue->runLoop(), [protectedThis] (GIOCondition condition) -> gboolean {
    370336        if (condition & G_IO_HUP || condition & G_IO_ERR || condition & G_IO_NVAL) {
    371337            protectedThis->connectionDidClose();
    … …  
    393359bool Connection::platformCanSendOutgoingMessages() const
    394360{
    395     return m_isConnected;
     361    return !m_pendingOutputMessage;
    396362}
    397363
    … …  
    400366    COMPILE_ASSERT(sizeof(MessageInfo) + attachmentMaxAmount * sizeof(size_t) <= messageMaxSize, AttachmentsFitToMessageInline);
    401367
    402     Vector<Attachment> attachments = encoder->releaseAttachments();
    403     if (attachments.size() > (attachmentMaxAmount - 1)) {
     368    UnixMessage outputMessage(*encoder);
     369    if (outputMessage.attachments().size() > (attachmentMaxAmount - 1)) {
    404370        ASSERT_NOT_REACHED();
    405371        return false;
    406372    }
    407373
    408     MessageInfo messageInfo(encoder->bufferSize(), attachments.size());
    409     size_t messageSizeWithBodyInline = sizeof(messageInfo) + (attachments.size() * sizeof(AttachmentInfo)) + encoder->bufferSize();
    410     if (messageSizeWithBodyInline > messageMaxSize && encoder->bufferSize()) {
     374    size_t messageSizeWithBodyInline = sizeof(MessageInfo) + (outputMessage.attachments().size() * sizeof(AttachmentInfo)) + outputMessage.bodySize();
     375    if (messageSizeWithBodyInline > messageMaxSize && outputMessage.bodySize()) {
    411376        RefPtr<WebKit::SharedMemory> oolMessageBody = WebKit::SharedMemory::allocate(encoder->bufferSize());
    412377        if (!oolMessageBody)
    … …  
    417382            return false;
    418383
    419         messageInfo.setMessageBodyIsOutOfLine();
    420 
    421         memcpy(oolMessageBody->data(), encoder->buffer(), encoder->bufferSize());
    422 
    423         attachments.append(handle.releaseAttachment());
    424     }
    425 
     384        outputMessage.messageInfo().setBodyOutOfLine();
     385
     386        memcpy(oolMessageBody->data(), outputMessage.body(), outputMessage.bodySize());
     387
     388        outputMessage.appendAttachment(handle.releaseAttachment());
     389    }
     390
     391    return sendOutputMessage(outputMessage);
     392}
     393
     394bool Connection::sendOutputMessage(UnixMessage& outputMessage)
     395{
     396    ASSERT(!m_pendingOutputMessage);
     397
     398    auto& messageInfo = outputMessage.messageInfo();
    426399    struct msghdr message;
    427400    memset(&message, 0, sizeof(message));
    … …  
    439412    MallocPtr<char> attachmentFDBuffer;
    440413
     414    auto& attachments = outputMessage.attachments();
    441415    if (!attachments.isEmpty()) {
    442416        int* fdPtr = 0;
    … …  
    489463    }
    490464
    491     if (!messageInfo.isMessageBodyIsOutOfLine() && encoder->bufferSize()) {
    492         iov[iovLength].iov_base = reinterpret_cast<void*>(encoder->buffer());
    493         iov[iovLength].iov_len = encoder->bufferSize();
     465    if (!messageInfo.isBodyOutOfLine() && outputMessage.bodySize()) {
     466        iov[iovLength].iov_base = reinterpret_cast<void*>(outputMessage.body());
     467        iov[iovLength].iov_len = outputMessage.bodySize();
    494468        ++iovLength;
    495469    }
    … …  
    501475            continue;
    502476        if (errno == EAGAIN || errno == EWOULDBLOCK) {
     477#if PLATFORM(GTK)
     478            m_pendingOutputMessage = std::make_unique<UnixMessage>(WTFMove(outputMessage));
     479            m_writeSocketMonitor.start(m_socket.get(), G_IO_OUT, m_connectionQueue->runLoop(), [this, protectedThis = makeRef(*this)] (GIOCondition condition) -> gboolean {
     480                if (condition & G_IO_OUT) {
     481                    ASSERT(m_pendingOutputMessage);
     482                    // We can't stop the monitor from this lambda, because stop destroys the lambda.
     483                    m_connectionQueue->dispatch([this, protectedThis = makeRef(*this)] {
     484                        m_writeSocketMonitor.stop();
     485                        auto message = WTFMove(m_pendingOutputMessage);
     486                        if (m_isConnected) {
     487                            sendOutputMessage(*message);
     488                            sendOutgoingMessages();
     489                        }
     490                    });
     491                }
     492                return G_SOURCE_REMOVE;
     493            });
     494            return false;
     495#else
    503496            struct pollfd pollfd;
    504497
    … …  
    508501            poll(&pollfd, 1, -1);
    509502            continue;
     503#endif
    510504        }
    511505

  • trunk/Source/WebKit2/PlatformGTK.cmake

    r212949 r213030  
    868868    "${WEBKIT2_DIR}/NetworkProcess/unix"
    869869    "${WEBKIT2_DIR}/Platform/IPC/glib"
     870    "${WEBKIT2_DIR}/Platform/IPC/unix"
    870871    "${WEBKIT2_DIR}/Platform/classifier"
    871872    "${WEBKIT2_DIR}/Shared/API/c/gtk"
Note: See TracChangeset for help on using the changeset viewer.