From 7e013c8c5ff959cd71a939a4795ef7dfc18a51f1 Mon Sep 17 00:00:00 2001 From: Carlos Garcia Campos Date: Fri, 24 Feb 2017 13:03:14 +0100 Subject: [PATCH] [GTK] Hangs when showing Google search results https://bugs.webkit.org/show_bug.cgi?id=168699 Reviewed by NOBODY (OOPS!). 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: --- Source/WebKit2/Platform/IPC/Connection.cpp | 4 + Source/WebKit2/Platform/IPC/Connection.h | 7 +- .../WebKit2/Platform/IPC/glib/GSocketMonitor.cpp | 2 +- Source/WebKit2/Platform/IPC/glib/GSocketMonitor.h | 4 +- .../WebKit2/Platform/IPC/unix/ConnectionUnix.cpp | 140 ++++++++++----------- Source/WebKit2/Platform/IPC/unix/UnixMessage.h | 113 +++++++++++++++++ Source/WebKit2/PlatformGTK.cmake | 1 + 7 files changed, 194 insertions(+), 77 deletions(-) create mode 100644 Source/WebKit2/Platform/IPC/unix/UnixMessage.h diff --git a/Source/WebKit2/Platform/IPC/Connection.cpp b/Source/WebKit2/Platform/IPC/Connection.cpp index daa6510..baf5306 100644 --- a/Source/WebKit2/Platform/IPC/Connection.cpp +++ b/Source/WebKit2/Platform/IPC/Connection.cpp @@ -39,6 +39,10 @@ #include "MachMessage.h" #endif +#if USE(UNIX_DOMAIN_SOCKETS) +#include "UnixMessage.h" +#endif + namespace IPC { struct Connection::ReplyHandler { diff --git a/Source/WebKit2/Platform/IPC/Connection.h b/Source/WebKit2/Platform/IPC/Connection.h index 87a3d91..a5a8ad7 100644 --- a/Source/WebKit2/Platform/IPC/Connection.h +++ b/Source/WebKit2/Platform/IPC/Connection.h @@ -79,6 +79,7 @@ enum class WaitForOption { while (0) class MachMessage; +class UnixMessage; class Connection : public ThreadSafeRefCounted { public: @@ -308,12 +309,16 @@ private: // Called on the connection queue. void readyReadHandler(); bool processMessage(); + bool sendOutputMessage(UnixMessage&); Vector m_readBuffer; Vector m_fileDescriptors; int m_socketDescriptor; + std::unique_ptr m_pendingOutputMessage; #if PLATFORM(GTK) - GSocketMonitor m_socketMonitor; + GRefPtr m_socket; + GSocketMonitor m_readSocketMonitor; + GSocketMonitor m_writeSocketMonitor; #endif #elif OS(DARWIN) // Called on the connection queue. diff --git a/Source/WebKit2/Platform/IPC/glib/GSocketMonitor.cpp b/Source/WebKit2/Platform/IPC/glib/GSocketMonitor.cpp index 0faf266..14329b9 100644 --- a/Source/WebKit2/Platform/IPC/glib/GSocketMonitor.cpp +++ b/Source/WebKit2/Platform/IPC/glib/GSocketMonitor.cpp @@ -42,7 +42,7 @@ gboolean GSocketMonitor::socketSourceCallback(GSocket*, GIOCondition condition, return monitor->m_callback(condition); } -void GSocketMonitor::start(GSocket* socket, GIOCondition condition, RunLoop& runLoop, std::function&& callback) +void GSocketMonitor::start(GSocket* socket, GIOCondition condition, RunLoop& runLoop, Function&& callback) { stop(); diff --git a/Source/WebKit2/Platform/IPC/glib/GSocketMonitor.h b/Source/WebKit2/Platform/IPC/glib/GSocketMonitor.h index 11e528a..ff37bf9 100644 --- a/Source/WebKit2/Platform/IPC/glib/GSocketMonitor.h +++ b/Source/WebKit2/Platform/IPC/glib/GSocketMonitor.h @@ -43,7 +43,7 @@ public: GSocketMonitor() = default; ~GSocketMonitor(); - void start(GSocket*, GIOCondition, RunLoop&, std::function&&); + void start(GSocket*, GIOCondition, RunLoop&, Function&&); void stop(); private: @@ -51,7 +51,7 @@ private: GRefPtr m_source; GRefPtr m_cancellable; - std::function m_callback; + Function m_callback; }; } // namespace IPC diff --git a/Source/WebKit2/Platform/IPC/unix/ConnectionUnix.cpp b/Source/WebKit2/Platform/IPC/unix/ConnectionUnix.cpp index fcd1047..0e04238 100644 --- a/Source/WebKit2/Platform/IPC/unix/ConnectionUnix.cpp +++ b/Source/WebKit2/Platform/IPC/unix/ConnectionUnix.cpp @@ -30,6 +30,7 @@ #include "DataReference.h" #include "SharedMemory.h" +#include "UnixMessage.h" #include #include #include @@ -60,60 +61,20 @@ namespace IPC { static const size_t messageMaxSize = 4096; static const size_t attachmentMaxAmount = 255; -enum { - MessageBodyIsOutOfLine = 1U << 31 -}; - -class MessageInfo { -public: - MessageInfo() { } - - MessageInfo(size_t bodySize, size_t initialAttachmentCount) - : m_bodySize(bodySize) - , m_attachmentCount(initialAttachmentCount) - , m_isMessageBodyOutOfLine(false) - { - } - - void setMessageBodyIsOutOfLine() - { - ASSERT(!isMessageBodyIsOutOfLine()); - - m_isMessageBodyOutOfLine = true; - m_attachmentCount++; - } - - bool isMessageBodyIsOutOfLine() const { return m_isMessageBodyOutOfLine; } - - size_t bodySize() const { return m_bodySize; } - - size_t attachmentCount() const { return m_attachmentCount; } - -private: - size_t m_bodySize; - size_t m_attachmentCount; - bool m_isMessageBodyOutOfLine; -}; - class AttachmentInfo { WTF_MAKE_FAST_ALLOCATED; public: - AttachmentInfo() - : m_type(Attachment::Uninitialized) - , m_size(0) - , m_isNull(false) - { - } + AttachmentInfo() = default; void setType(Attachment::Type type) { m_type = type; } - Attachment::Type getType() { return m_type; } + Attachment::Type type() const { return m_type; } void setSize(size_t size) { ASSERT(m_type == Attachment::MappedMemoryType); m_size = size; } - size_t getSize() + size_t size() const { ASSERT(m_type == Attachment::MappedMemoryType); return m_size; @@ -121,25 +82,30 @@ public: // The attachment is not null unless explicitly set. void setNull() { m_isNull = true; } - bool isNull() { return m_isNull; } + bool isNull() const { return m_isNull; } private: - Attachment::Type m_type; - size_t m_size; - bool m_isNull; + Attachment::Type m_type { Attachment::Uninitialized }; + size_t m_size { 0 }; + bool m_isNull { false }; }; void Connection::platformInitialize(Identifier identifier) { m_socketDescriptor = identifier; +#if PLATFORM(GTK) + m_socket = adoptGRef(g_socket_new_from_fd(m_socketDescriptor, nullptr)); +#endif m_readBuffer.reserveInitialCapacity(messageMaxSize); m_fileDescriptors.reserveInitialCapacity(attachmentMaxAmount); } void Connection::platformInvalidate() { - // In GTK+ platform the socket is closed by the work queue. -#if !PLATFORM(GTK) +#if PLATFORM(GTK) + // In GTK+ platform the socket descriptor is owned by GSocket. + m_socket = nullptr; +#else if (m_socketDescriptor != -1) closeWithRetry(m_socketDescriptor); #endif @@ -148,7 +114,8 @@ void Connection::platformInvalidate() return; #if PLATFORM(GTK) - m_socketMonitor.stop(); + m_readSocketMonitor.stop(); + m_writeSocketMonitor.stop(); #endif m_socketDescriptor = -1; @@ -165,7 +132,7 @@ bool Connection::processMessage() memcpy(&messageInfo, messageData, sizeof(messageInfo)); messageData += sizeof(messageInfo); - size_t messageLength = sizeof(MessageInfo) + messageInfo.attachmentCount() * sizeof(AttachmentInfo) + (messageInfo.isMessageBodyIsOutOfLine() ? 0 : messageInfo.bodySize()); + size_t messageLength = sizeof(MessageInfo) + messageInfo.attachmentCount() * sizeof(AttachmentInfo) + (messageInfo.isBodyOutOfLine() ? 0 : messageInfo.bodySize()); if (m_readBuffer.size() < messageLength) return false; @@ -179,7 +146,7 @@ bool Connection::processMessage() messageData += sizeof(AttachmentInfo) * attachmentCount; for (size_t i = 0; i < attachmentCount; ++i) { - switch (attachmentInfo[i].getType()) { + switch (attachmentInfo[i].type()) { case Attachment::MappedMemoryType: case Attachment::SocketType: if (!attachmentInfo[i].isNull()) @@ -191,7 +158,7 @@ bool Connection::processMessage() } } - if (messageInfo.isMessageBodyIsOutOfLine()) + if (messageInfo.isBodyOutOfLine()) attachmentCount--; } @@ -201,11 +168,11 @@ bool Connection::processMessage() size_t fdIndex = 0; for (size_t i = 0; i < attachmentCount; ++i) { int fd = -1; - switch (attachmentInfo[i].getType()) { + switch (attachmentInfo[i].type()) { case Attachment::MappedMemoryType: if (!attachmentInfo[i].isNull()) fd = m_fileDescriptors[fdIndex++]; - attachments[attachmentCount - i - 1] = Attachment(fd, attachmentInfo[i].getSize()); + attachments[attachmentCount - i - 1] = Attachment(fd, attachmentInfo[i].size()); break; case Attachment::SocketType: if (!attachmentInfo[i].isNull()) @@ -219,7 +186,7 @@ bool Connection::processMessage() } } - if (messageInfo.isMessageBodyIsOutOfLine()) { + if (messageInfo.isBodyOutOfLine()) { ASSERT(messageInfo.bodySize()); if (attachmentInfo[attachmentCount].isNull()) { @@ -228,7 +195,7 @@ bool Connection::processMessage() } WebKit::SharedMemory::Handle handle; - handle.adoptAttachment(IPC::Attachment(m_fileDescriptors[attachmentFileDescriptorCount - 1], attachmentInfo[attachmentCount].getSize())); + handle.adoptAttachment(IPC::Attachment(m_fileDescriptors[attachmentFileDescriptorCount - 1], attachmentInfo[attachmentCount].size())); oolMessageBody = WebKit::SharedMemory::map(handle, WebKit::SharedMemory::Protection::ReadOnly); if (!oolMessageBody) { @@ -237,10 +204,10 @@ bool Connection::processMessage() } } - ASSERT(attachments.size() == (messageInfo.isMessageBodyIsOutOfLine() ? messageInfo.attachmentCount() - 1 : messageInfo.attachmentCount())); + ASSERT(attachments.size() == (messageInfo.isBodyOutOfLine() ? messageInfo.attachmentCount() - 1 : messageInfo.attachmentCount())); uint8_t* messageBody = messageData; - if (messageInfo.isMessageBodyIsOutOfLine()) + if (messageInfo.isBodyOutOfLine()) messageBody = reinterpret_cast(oolMessageBody->data()); auto decoder = std::make_unique(messageBody, messageInfo.bodySize(), nullptr, WTFMove(attachments)); @@ -365,8 +332,7 @@ bool Connection::open() RefPtr protectedThis(this); m_isConnected = true; #if PLATFORM(GTK) - GRefPtr socket = adoptGRef(g_socket_new_from_fd(m_socketDescriptor, nullptr)); - m_socketMonitor.start(socket.get(), G_IO_IN, m_connectionQueue->runLoop(), [protectedThis] (GIOCondition condition) -> gboolean { + m_readSocketMonitor.start(m_socket.get(), G_IO_IN, m_connectionQueue->runLoop(), [protectedThis] (GIOCondition condition) -> gboolean { if (condition & G_IO_HUP || condition & G_IO_ERR || condition & G_IO_NVAL) { protectedThis->connectionDidClose(); return G_SOURCE_REMOVE; @@ -392,22 +358,21 @@ bool Connection::open() bool Connection::platformCanSendOutgoingMessages() const { - return m_isConnected; + return !m_pendingOutputMessage; } bool Connection::sendOutgoingMessage(std::unique_ptr encoder) { COMPILE_ASSERT(sizeof(MessageInfo) + attachmentMaxAmount * sizeof(size_t) <= messageMaxSize, AttachmentsFitToMessageInline); - Vector attachments = encoder->releaseAttachments(); - if (attachments.size() > (attachmentMaxAmount - 1)) { + UnixMessage outputMessage(encoder.get()); + if (outputMessage.attachments().size() > (attachmentMaxAmount - 1)) { ASSERT_NOT_REACHED(); return false; } - MessageInfo messageInfo(encoder->bufferSize(), attachments.size()); - size_t messageSizeWithBodyInline = sizeof(messageInfo) + (attachments.size() * sizeof(AttachmentInfo)) + encoder->bufferSize(); - if (messageSizeWithBodyInline > messageMaxSize && encoder->bufferSize()) { + size_t messageSizeWithBodyInline = sizeof(MessageInfo) + (outputMessage.attachments().size() * sizeof(AttachmentInfo)) + outputMessage.bodySize(); + if (messageSizeWithBodyInline > messageMaxSize && outputMessage.bodySize()) { RefPtr oolMessageBody = WebKit::SharedMemory::allocate(encoder->bufferSize()); if (!oolMessageBody) return false; @@ -416,13 +381,21 @@ bool Connection::sendOutgoingMessage(std::unique_ptr encoder) if (!oolMessageBody->createHandle(handle, WebKit::SharedMemory::Protection::ReadOnly)) return false; - messageInfo.setMessageBodyIsOutOfLine(); + outputMessage.messageInfo().setBodyOutOfLine(); - memcpy(oolMessageBody->data(), encoder->buffer(), encoder->bufferSize()); + memcpy(oolMessageBody->data(), outputMessage.body(), outputMessage.bodySize()); - attachments.append(handle.releaseAttachment()); + outputMessage.appendAttachment(handle.releaseAttachment()); } + return sendOutputMessage(outputMessage); +} + +bool Connection::sendOutputMessage(UnixMessage& outputMessage) +{ + ASSERT(!m_pendingOutputMessage); + + auto& messageInfo = outputMessage.messageInfo(); struct msghdr message; memset(&message, 0, sizeof(message)); @@ -438,6 +411,7 @@ bool Connection::sendOutgoingMessage(std::unique_ptr encoder) std::unique_ptr attachmentInfo; MallocPtr attachmentFDBuffer; + auto& attachments = outputMessage.attachments(); if (!attachments.isEmpty()) { int* fdPtr = 0; @@ -488,9 +462,9 @@ bool Connection::sendOutgoingMessage(std::unique_ptr encoder) ++iovLength; } - if (!messageInfo.isMessageBodyIsOutOfLine() && encoder->bufferSize()) { - iov[iovLength].iov_base = reinterpret_cast(encoder->buffer()); - iov[iovLength].iov_len = encoder->bufferSize(); + if (!messageInfo.isBodyOutOfLine() && outputMessage.bodySize()) { + iov[iovLength].iov_base = reinterpret_cast(outputMessage.body()); + iov[iovLength].iov_len = outputMessage.bodySize(); ++iovLength; } @@ -500,6 +474,25 @@ bool Connection::sendOutgoingMessage(std::unique_ptr encoder) if (errno == EINTR) continue; if (errno == EAGAIN || errno == EWOULDBLOCK) { +#if PLATFORM(GTK) + m_pendingOutputMessage = std::make_unique(WTFMove(outputMessage)); + m_writeSocketMonitor.start(m_socket.get(), G_IO_OUT, m_connectionQueue->runLoop(), [this, protectedThis = makeRef(*this)] (GIOCondition condition) -> gboolean { + if (condition & G_IO_OUT) { + ASSERT(m_pendingOutputMessage); + // We can't stop the monitor from this lambda, because stop destroys the lambda. + m_connectionQueue->dispatch([this, protectedThis = makeRef(*this)] { + m_writeSocketMonitor.stop(); + auto message = WTFMove(m_pendingOutputMessage); + if (m_isConnected) { + sendOutputMessage(*message); + sendOutgoingMessages(); + } + }); + } + return G_SOURCE_REMOVE; + }); + return false; +#else struct pollfd pollfd; pollfd.fd = m_socketDescriptor; @@ -507,6 +500,7 @@ bool Connection::sendOutgoingMessage(std::unique_ptr encoder) pollfd.revents = 0; poll(&pollfd, 1, -1); continue; +#endif } if (m_isConnected) diff --git a/Source/WebKit2/Platform/IPC/unix/UnixMessage.h b/Source/WebKit2/Platform/IPC/unix/UnixMessage.h new file mode 100644 index 0000000..e99a0a4 --- /dev/null +++ b/Source/WebKit2/Platform/IPC/unix/UnixMessage.h @@ -0,0 +1,113 @@ +/* + * Copyright (C) 2010 Apple Inc. All rights reserved. + * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies) + * Copyright (C) 2011,2017 Igalia S.L. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, + * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR + * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS + * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF + * THE POSSIBILITY OF SUCH DAMAGE. + */ + +#pragma once + +#include "Attachment.h" +#include + +namespace IPC { + +class MessageInfo { +public: + MessageInfo() = default; + + MessageInfo(size_t bodySize, size_t initialAttachmentCount) + : m_bodySize(bodySize) + , m_attachmentCount(initialAttachmentCount) + { + } + + void setBodyOutOfLine() + { + ASSERT(!isBodyOutOfLine()); + + m_isBodyOutOfLine = true; + m_attachmentCount++; + } + + bool isBodyOutOfLine() const { return m_isBodyOutOfLine; } + size_t bodySize() const { return m_bodySize; } + size_t attachmentCount() const { return m_attachmentCount; } + +private: + size_t m_bodySize { 0 }; + size_t m_attachmentCount { 0 }; + bool m_isBodyOutOfLine { false }; +}; + +class UnixMessage { + WTF_MAKE_FAST_ALLOCATED; +public: + UnixMessage(Encoder* encoder) + : m_attachments(encoder->releaseAttachments()) + , m_messageInfo(encoder->bufferSize(), m_attachments.size()) + , m_body(encoder->buffer()) + { + } + + UnixMessage(UnixMessage&& other) + { + m_attachments = WTFMove(other.m_attachments); + m_messageInfo = WTFMove(other.m_messageInfo); + if (other.m_bodyOwned) { + std::swap(m_body, other.m_body); + std::swap(m_bodyOwned, other.m_bodyOwned); + } else if (!m_messageInfo.isBodyOutOfLine()) { + m_body = static_cast(fastMalloc(m_messageInfo.bodySize())); + memcpy(m_body, other.m_body, m_messageInfo.bodySize()); + m_bodyOwned = true; + other.m_body = nullptr; + other.m_bodyOwned = false; + } + } + + ~UnixMessage() + { + if (m_bodyOwned) + fastFree(m_body); + } + + const Vector& attachments() const { return m_attachments; } + MessageInfo& messageInfo() { return m_messageInfo; } + + uint8_t* body() const { return m_body; } + size_t bodySize() const { return m_messageInfo.bodySize(); } + + void appendAttachment(Attachment&& attachment) + { + m_attachments.append(WTFMove(attachment)); + } + +private: + Vector m_attachments; + MessageInfo m_messageInfo; + uint8_t* m_body { nullptr }; + bool m_bodyOwned { false }; +}; + +} // namespace IPC diff --git a/Source/WebKit2/PlatformGTK.cmake b/Source/WebKit2/PlatformGTK.cmake index ebede75..2c6bc5f 100644 --- a/Source/WebKit2/PlatformGTK.cmake +++ b/Source/WebKit2/PlatformGTK.cmake @@ -850,6 +850,7 @@ list(APPEND WebKit2_INCLUDE_DIRECTORIES "${WEBKIT2_DIR}/NetworkProcess/soup" "${WEBKIT2_DIR}/NetworkProcess/unix" "${WEBKIT2_DIR}/Platform/IPC/glib" + "${WEBKIT2_DIR}/Platform/IPC/unix" "${WEBKIT2_DIR}/Shared/API/c/gtk" "${WEBKIT2_DIR}/Shared/Plugins/unix" "${WEBKIT2_DIR}/Shared/glib" -- 2.11.1