Title: [202370] trunk/Source/WebCore
Revision
202370
Author
[email protected]
Date
2016-06-23 00:23:19 -0700 (Thu, 23 Jun 2016)

Log Message

[Soup] Clean up SocketStreamHandle soup implementation
https://bugs.webkit.org/show_bug.cgi?id=159024

Reviewed by Žan Doberšek.

Stop using a global HashMap to "acivate"/"deactivate" handles, and just take a reference of the handle and
pass the ownership to the callbacks, using a GCancellable to cancel all async operations.

* platform/network/soup/SocketStreamHandle.h:
(WebCore::SocketStreamHandle::create):
(WebCore::SocketStreamHandle::id): Deleted.
* platform/network/soup/SocketStreamHandleSoup.cpp:
(WebCore::SocketStreamHandle::SocketStreamHandle):
(WebCore::SocketStreamHandle::connected):
(WebCore::SocketStreamHandle::connectedCallback):
(WebCore::SocketStreamHandle::readBytes):
(WebCore::SocketStreamHandle::readReadyCallback):
(WebCore::SocketStreamHandle::didFail):
(WebCore::SocketStreamHandle::platformSend):
(WebCore::SocketStreamHandle::platformClose):
(WebCore::SocketStreamHandle::beginWaitingForSocketWritability):
(WebCore::SocketStreamHandle::writeReadyCallback):
(WebCore::getHandleFromId): Deleted.
(WebCore::deactivateHandle): Deleted.
(WebCore::activateHandle): Deleted.
(WebCore::SocketStreamHandle::~SocketStreamHandle): Deleted.
(WebCore::connectedCallback): Deleted.
(WebCore::readReadyCallback): Deleted.
(WebCore::writeReadyCallback): Deleted.

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (202369 => 202370)


--- trunk/Source/WebCore/ChangeLog	2016-06-23 06:44:02 UTC (rev 202369)
+++ trunk/Source/WebCore/ChangeLog	2016-06-23 07:23:19 UTC (rev 202370)
@@ -1,3 +1,35 @@
+2016-06-23  Carlos Garcia Campos  <[email protected]>
+
+        [Soup] Clean up SocketStreamHandle soup implementation
+        https://bugs.webkit.org/show_bug.cgi?id=159024
+
+        Reviewed by Žan Doberšek.
+
+        Stop using a global HashMap to "acivate"/"deactivate" handles, and just take a reference of the handle and
+        pass the ownership to the callbacks, using a GCancellable to cancel all async operations.
+
+        * platform/network/soup/SocketStreamHandle.h:
+        (WebCore::SocketStreamHandle::create):
+        (WebCore::SocketStreamHandle::id): Deleted.
+        * platform/network/soup/SocketStreamHandleSoup.cpp:
+        (WebCore::SocketStreamHandle::SocketStreamHandle):
+        (WebCore::SocketStreamHandle::connected):
+        (WebCore::SocketStreamHandle::connectedCallback):
+        (WebCore::SocketStreamHandle::readBytes):
+        (WebCore::SocketStreamHandle::readReadyCallback):
+        (WebCore::SocketStreamHandle::didFail):
+        (WebCore::SocketStreamHandle::platformSend):
+        (WebCore::SocketStreamHandle::platformClose):
+        (WebCore::SocketStreamHandle::beginWaitingForSocketWritability):
+        (WebCore::SocketStreamHandle::writeReadyCallback):
+        (WebCore::getHandleFromId): Deleted.
+        (WebCore::deactivateHandle): Deleted.
+        (WebCore::activateHandle): Deleted.
+        (WebCore::SocketStreamHandle::~SocketStreamHandle): Deleted.
+        (WebCore::connectedCallback): Deleted.
+        (WebCore::readReadyCallback): Deleted.
+        (WebCore::writeReadyCallback): Deleted.
+
 2016-06-22  Brady Eidson  <[email protected]>
 
         DatabaseProcess doesn't handle WebProcesses going away uncleanly.

Modified: trunk/Source/WebCore/platform/network/soup/SocketStreamHandle.h (202369 => 202370)


--- trunk/Source/WebCore/platform/network/soup/SocketStreamHandle.h	2016-06-23 06:44:02 UTC (rev 202369)
+++ trunk/Source/WebCore/platform/network/soup/SocketStreamHandle.h	2016-06-23 07:23:19 UTC (rev 202370)
@@ -37,45 +37,49 @@
 
 #if USE(SOUP)
 
-#include <wtf/PassRefPtr.h>
 #include <wtf/RefCounted.h>
 #include <wtf/glib/GRefPtr.h>
 
 namespace WebCore {
 
-    class NetworkingContext;
-    class SocketStreamHandleClient;
+class NetworkingContext;
+class SocketStreamError;
+class SocketStreamHandleClient;
 
-    class SocketStreamHandle : public RefCounted<SocketStreamHandle>, public SocketStreamHandleBase {
-    public:
-        static Ref<SocketStreamHandle> create(const URL& url, SocketStreamHandleClient* client, NetworkingContext&, bool) { return adoptRef(*new SocketStreamHandle(url, client)); }
-        static Ref<SocketStreamHandle> create(GSocketConnection* socketConnection, SocketStreamHandleClient* client) { return adoptRef(*new SocketStreamHandle(socketConnection, client)); }
+class SocketStreamHandle final : public RefCounted<SocketStreamHandle>, public SocketStreamHandleBase {
+public:
+    static Ref<SocketStreamHandle> create(const URL& url, SocketStreamHandleClient* client, NetworkingContext&, bool) { return adoptRef(*new SocketStreamHandle(url, client)); }
+    static Ref<SocketStreamHandle> create(GSocketConnection* socketConnection, SocketStreamHandleClient* client) { return adoptRef(*new SocketStreamHandle(socketConnection, client)); }
 
-        virtual ~SocketStreamHandle();
-        void connected(GSocketConnection*, GError*);
-        void readBytes(signed long, GError*);
-        void writeReady();
-        void* id() { return m_id; }
+    virtual ~SocketStreamHandle();
 
-    protected:
-        virtual int platformSend(const char* data, int length);
-        virtual void platformClose();
+private:
+    SocketStreamHandle(const URL&, SocketStreamHandleClient*);
+    SocketStreamHandle(GSocketConnection*, SocketStreamHandleClient*);
 
-    private:
-        GRefPtr<GSocketConnection> m_socketConnection;
-        GRefPtr<GInputStream> m_inputStream;
-        GRefPtr<GPollableOutputStream> m_outputStream;
-        GRefPtr<GSource> m_writeReadySource;
-        std::unique_ptr<char[]> m_readBuffer;
-        void* m_id;
+    int platformSend(const char* data, int length) override;
+    void platformClose() override;
 
-        SocketStreamHandle(const URL&, SocketStreamHandleClient*);
-        SocketStreamHandle(GSocketConnection*, SocketStreamHandleClient*);
+    void beginWaitingForSocketWritability();
+    void stopWaitingForSocketWritability();
 
-        void beginWaitingForSocketWritability();
-        void stopWaitingForSocketWritability();
-    };
+    static void connectedCallback(GSocketClient*, GAsyncResult*, SocketStreamHandle*);
+    static void readReadyCallback(GInputStream*, GAsyncResult*, SocketStreamHandle*);
+    static gboolean writeReadyCallback(GPollableOutputStream*, SocketStreamHandle*);
 
+    void connected(GRefPtr<GSocketConnection>&&);
+    void readBytes(gssize);
+    void didFail(SocketStreamError&&);
+    void writeReady();
+
+    GRefPtr<GSocketConnection> m_socketConnection;
+    GRefPtr<GInputStream> m_inputStream;
+    GRefPtr<GPollableOutputStream> m_outputStream;
+    GRefPtr<GSource> m_writeReadySource;
+    GRefPtr<GCancellable> m_cancellable;
+    std::unique_ptr<char[]> m_readBuffer;
+};
+
 }  // namespace WebCore
 
 #endif

Modified: trunk/Source/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp (202369 => 202370)


--- trunk/Source/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp	2016-06-23 06:44:02 UTC (rev 202369)
+++ trunk/Source/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp	2016-06-23 07:23:19 UTC (rev 202370)
@@ -38,10 +38,8 @@
 #include "Logging.h"
 #include "SocketStreamError.h"
 #include "SocketStreamHandleClient.h"
-
 #include <gio/gio.h>
 #include <glib.h>
-
 #include <wtf/Vector.h>
 #include <wtf/glib/GUniquePtr.h>
 #include <wtf/text/CString.h>
@@ -50,93 +48,74 @@
 
 namespace WebCore {
 
-// These functions immediately call the similarly named SocketStreamHandle methods.
-static void connectedCallback(GSocketClient*, GAsyncResult*, void*);
-static void readReadyCallback(GInputStream*, GAsyncResult*, void*);
-static gboolean writeReadyCallback(GPollableOutputStream*, void*);
-
-// Having a list of active handles means that we do not have to worry about WebCore
-// reference counting in GLib callbacks. Once the handle is off the active handles list
-// we just ignore it in the callback. We avoid a lot of extra checks and tricky
-// situations this way.
-static HashMap<void*, SocketStreamHandle*> gActiveHandles;
-COMPILE_ASSERT(HashTraits<SocketStreamHandle*>::emptyValueIsZero, emptyMapValue_is_0);
-
-static SocketStreamHandle* getHandleFromId(void* id)
-{
-    return gActiveHandles.get(id);
-}
-
-static void deactivateHandle(SocketStreamHandle& handle)
-{
-    gActiveHandles.remove(handle.id());
-}
-
-static void* activateHandle(SocketStreamHandle& handle)
-{
-    // The first id cannot be 0, because it conflicts with the HashMap emptyValue.
-    static gint currentHandleId = 1;
-    void* id = GINT_TO_POINTER(currentHandleId++);
-    gActiveHandles.set(id, &handle);
-    return id;
-}
-
 SocketStreamHandle::SocketStreamHandle(const URL& url, SocketStreamHandleClient* client)
     : SocketStreamHandleBase(url, client)
+    , m_cancellable(adoptGRef(g_cancellable_new()))
 {
     LOG(Network, "SocketStreamHandle %p new client %p", this, m_client);
-    unsigned int port = url.hasPort() ? url.port() : (url.protocolIs("wss") ? 443 : 80);
+    unsigned port = url.hasPort() ? url.port() : (url.protocolIs("wss") ? 443 : 80);
 
-    m_id = activateHandle(*this);
     GRefPtr<GSocketClient> socketClient = adoptGRef(g_socket_client_new());
     if (url.protocolIs("wss"))
         g_socket_client_set_tls(socketClient.get(), TRUE);
-    g_socket_client_connect_to_host_async(socketClient.get(), url.host().utf8().data(), port, 0,
-        reinterpret_cast<GAsyncReadyCallback>(connectedCallback), m_id);
+    RefPtr<SocketStreamHandle> protectedThis(this);
+    g_socket_client_connect_to_host_async(socketClient.get(), url.host().utf8().data(), port, m_cancellable.get(),
+        reinterpret_cast<GAsyncReadyCallback>(connectedCallback), protectedThis.leakRef());
 }
 
 SocketStreamHandle::SocketStreamHandle(GSocketConnection* socketConnection, SocketStreamHandleClient* client)
     : SocketStreamHandleBase(URL(), client)
+    , m_cancellable(adoptGRef(g_cancellable_new()))
 {
     LOG(Network, "SocketStreamHandle %p new client %p", this, m_client);
-    m_id = activateHandle(*this);
-    connected(socketConnection, 0);
+    GRefPtr<GSocketConnection> connection = socketConnection;
+    connected(WTFMove(connection));
 }
 
 SocketStreamHandle::~SocketStreamHandle()
 {
     LOG(Network, "SocketStreamHandle %p delete", this);
-    // If for some reason we were destroyed without closing, ensure that we are deactivated.
-    deactivateHandle(*this);
     setClient(nullptr);
 }
 
-void SocketStreamHandle::connected(GSocketConnection* socketConnection, GError* error)
+void SocketStreamHandle::connected(GRefPtr<GSocketConnection>&& socketConnection)
 {
-    if (error) {
-        m_client->didFailSocketStream(*this, SocketStreamError(error->code, error->message));
-        return;
-    }
-
-    m_socketConnection = socketConnection;
+    m_socketConnection = WTFMove(socketConnection);
     m_outputStream = G_POLLABLE_OUTPUT_STREAM(g_io_stream_get_output_stream(G_IO_STREAM(m_socketConnection.get())));
     m_inputStream = g_io_stream_get_input_stream(G_IO_STREAM(m_socketConnection.get()));
-
     m_readBuffer = std::make_unique<char[]>(READ_BUFFER_SIZE);
-    g_input_stream_read_async(m_inputStream.get(), m_readBuffer.get(), READ_BUFFER_SIZE, G_PRIORITY_DEFAULT, 0,
-        reinterpret_cast<GAsyncReadyCallback>(readReadyCallback), m_id);
 
+    RefPtr<SocketStreamHandle> protectedThis(this);
+    g_input_stream_read_async(m_inputStream.get(), m_readBuffer.get(), READ_BUFFER_SIZE, G_PRIORITY_DEFAULT, m_cancellable.get(),
+        reinterpret_cast<GAsyncReadyCallback>(readReadyCallback), protectedThis.leakRef());
+
     m_state = Open;
     m_client->didOpenSocketStream(*this);
 }
 
-void SocketStreamHandle::readBytes(signed long bytesRead, GError* error)
+void SocketStreamHandle::connectedCallback(GSocketClient* client, GAsyncResult* result, SocketStreamHandle* handle)
 {
-    if (error) {
-        m_client->didFailSocketStream(*this, SocketStreamError(error->code, error->message));
+    RefPtr<SocketStreamHandle> protectedThis = adoptRef(handle);
+
+    // Always finish the connection, even if this SocketStreamHandle was cancelled earlier.
+    GUniqueOutPtr<GError> error;
+    GRefPtr<GSocketConnection> socketConnection = adoptGRef(g_socket_client_connect_to_host_finish(client, result, &error.outPtr()));
+
+    // The SocketStreamHandle has been cancelled, so just close the connection, ignoring errors.
+    if (g_cancellable_is_cancelled(handle->m_cancellable.get())) {
+        if (socketConnection)
+            g_io_stream_close(G_IO_STREAM(socketConnection.get()), nullptr, nullptr);
         return;
     }
 
+    if (error)
+        handle->didFail(SocketStreamError(error->code, error->message));
+    else
+        handle->connected(WTFMove(socketConnection));
+}
+
+void SocketStreamHandle::readBytes(gssize bytesRead)
+{
     if (!bytesRead) {
         close();
         return;
@@ -143,13 +122,36 @@
     }
 
     // The client can close the handle, potentially removing the last reference.
-    Ref<SocketStreamHandle> protectedThis(*this); 
+    RefPtr<SocketStreamHandle> protectedThis(this);
     m_client->didReceiveSocketStreamData(*this, m_readBuffer.get(), bytesRead);
-    if (m_inputStream) // The client may have closed the connection.
-        g_input_stream_read_async(m_inputStream.get(), m_readBuffer.get(), READ_BUFFER_SIZE, G_PRIORITY_DEFAULT, 0,
-            reinterpret_cast<GAsyncReadyCallback>(readReadyCallback), m_id);
+    if (m_inputStream) {
+        g_input_stream_read_async(m_inputStream.get(), m_readBuffer.get(), READ_BUFFER_SIZE, G_PRIORITY_DEFAULT, m_cancellable.get(),
+            reinterpret_cast<GAsyncReadyCallback>(readReadyCallback), protectedThis.leakRef());
+    }
 }
 
+void SocketStreamHandle::readReadyCallback(GInputStream* stream, GAsyncResult* result, SocketStreamHandle* handle)
+{
+    RefPtr<SocketStreamHandle> protectedThis = adoptRef(handle);
+
+    // Always finish the read, even if this SocketStreamHandle was cancelled earlier.
+    GUniqueOutPtr<GError> error;
+    gssize bytesRead = g_input_stream_read_finish(stream, result, &error.outPtr());
+
+    if (g_cancellable_is_cancelled(handle->m_cancellable.get()))
+        return;
+
+    if (error)
+        handle->didFail(SocketStreamError(error->code, error->message));
+    else
+        handle->readBytes(bytesRead);
+}
+
+void SocketStreamHandle::didFail(SocketStreamError&& error)
+{
+    m_client->didFailSocketStream(*this, WTFMove(error));
+}
+
 void SocketStreamHandle::writeReady()
 {
     // We no longer have buffered data, so stop waiting for the socket to be writable.
@@ -168,12 +170,12 @@
         return 0;
 
     GUniqueOutPtr<GError> error;
-    gssize written = g_pollable_output_stream_write_nonblocking(m_outputStream.get(), data, length, 0, &error.outPtr());
+    gssize written = g_pollable_output_stream_write_nonblocking(m_outputStream.get(), data, length, m_cancellable.get(), &error.outPtr());
     if (error) {
         if (g_error_matches(error.get(), G_IO_ERROR, G_IO_ERROR_WOULD_BLOCK))
             beginWaitingForSocketWritability();
         else
-            m_client->didFailSocketStream(*this, SocketStreamError(error->code, error->message));
+            didFail(SocketStreamError(error->code, error->message));
         return 0;
     }
 
@@ -188,20 +190,20 @@
 void SocketStreamHandle::platformClose()
 {
     LOG(Network, "SocketStreamHandle %p platformClose", this);
-    // We remove this handle from the active handles list first, to disable all callbacks.
-    deactivateHandle(*this);
+    // We cancel this handle first to disable all callbacks.
+    g_cancellable_cancel(m_cancellable.get());
     stopWaitingForSocketWritability();
 
     if (m_socketConnection) {
         GUniqueOutPtr<GError> error;
-        g_io_stream_close(G_IO_STREAM(m_socketConnection.get()), 0, &error.outPtr());
+        g_io_stream_close(G_IO_STREAM(m_socketConnection.get()), nullptr, &error.outPtr());
         if (error)
-            m_client->didFailSocketStream(*this, SocketStreamError(error->code, error->message));
-        m_socketConnection = 0;
+            didFail(SocketStreamError(error->code, error->message));
+        m_socketConnection = nullptr;
     }
 
-    m_outputStream = 0;
-    m_inputStream = 0;
+    m_outputStream = nullptr;
+    m_inputStream = nullptr;
     m_readBuffer = nullptr;
 
     m_client->didCloseSocketStream(*this);
@@ -212,8 +214,10 @@
     if (m_writeReadySource) // Already waiting.
         return;
 
-    m_writeReadySource = adoptGRef(g_pollable_output_stream_create_source(m_outputStream.get(), 0));
-    g_source_set_callback(m_writeReadySource.get(), reinterpret_cast<GSourceFunc>(writeReadyCallback), m_id, 0);
+    m_writeReadySource = adoptGRef(g_pollable_output_stream_create_source(m_outputStream.get(), m_cancellable.get()));
+    ref();
+    g_source_set_callback(m_writeReadySource.get(), reinterpret_cast<GSourceFunc>(writeReadyCallback), this,
+        [](gpointer handle) { static_cast<SocketStreamHandle*>(handle)->deref(); });
     g_source_attach(m_writeReadySource.get(), g_main_context_get_thread_default());
 }
 
@@ -226,44 +230,13 @@
     m_writeReadySource = nullptr;
 }
 
-static void connectedCallback(GSocketClient* client, GAsyncResult* result, void* id)
+gboolean SocketStreamHandle::writeReadyCallback(GPollableOutputStream*, SocketStreamHandle* handle)
 {
-    // Always finish the connection, even if this SocketStreamHandle was deactivated earlier.
-    GUniqueOutPtr<GError> error;
-    GSocketConnection* socketConnection = g_socket_client_connect_to_host_finish(client, result, &error.outPtr());
+    if (g_cancellable_is_cancelled(handle->m_cancellable.get()))
+        return G_SOURCE_REMOVE;
 
-    // The SocketStreamHandle has been deactivated, so just close the connection, ignoring errors.
-    SocketStreamHandle* handle = getHandleFromId(id);
-    if (!handle) {
-        if (socketConnection)
-            g_io_stream_close(G_IO_STREAM(socketConnection), 0, 0);
-        return;
-    }
-
-    handle->connected(socketConnection, error.get());
-}
-
-static void readReadyCallback(GInputStream* stream, GAsyncResult* result, void* id)
-{
-    // Always finish the read, even if this SocketStreamHandle was deactivated earlier.
-    GUniqueOutPtr<GError> error;
-    gssize bytesRead = g_input_stream_read_finish(stream, result, &error.outPtr());
-
-    SocketStreamHandle* handle = getHandleFromId(id);
-    if (!handle)
-        return;
-
-    handle->readBytes(bytesRead, error.get());
-}
-
-static gboolean writeReadyCallback(GPollableOutputStream*, void* id)
-{
-    SocketStreamHandle* handle = getHandleFromId(id);
-    if (!handle)
-        return FALSE;
-
     handle->writeReady();
-    return TRUE;
+    return G_SOURCE_CONTINUE;
 }
 
 } // namespace WebCore
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to