GMainLoopSource is exposed to race conditions
authorzandobersek@gmail.com <zandobersek@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 18 Sep 2014 13:06:59 +0000 (13:06 +0000)
committerzandobersek@gmail.com <zandobersek@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 18 Sep 2014 13:06:59 +0000 (13:06 +0000)
https://bugs.webkit.org/show_bug.cgi?id=135800

Reviewed by Carlos Garcia Campos.

Source/WTF:

GMainLoopSource objects can be dispatching tasks on one thread
while having a new task scheduled on a different thread. This
can for instance occur in WebKitVideoSink, where the timeout
callback can be called on main thread while at the same time
it is being rescheduled on a different thread (created through
GStreamer).

The initial solution is to use GMutex to prevent parallel data
access from different threads. In the future I plan to look at
the possibility of creating thread-specific GMainLoopSource
objects that wouldn't require the use of GMutex.

GSource, GCancellable and std::function<> objects are now packed
into an internal Context structure. Using the C++11 move semantics
it's simple to, at the time of dispatch, move the current context
out of the GMainLoopSource object in case the dispatch causes a
rescheduling on that same object.

Also added in the Context struct is a new GCancellable. The pointer
of that object is shared with the GMainLoopSource before the Context
is moved out for the callback dispatch. This makes it safe to cancel
or even delete the GMainLoopSource during the dispatch and prevents
use-after-delete on GMainLoopSource once the dispatch is done in
the GMainLoopSource::*Callback() methods.

All the schedule*() methods and the cancelWithoutLocking() method
callers now lock the GMutex to ensure no one else is accessing the
data at that moment. Similar goes for the dispatch methods, but those
do the dispatch and possible destruction duties with the mutex unlocked.
The dispatch can cause rescheduling on the same GMainLoopSource object,
which must not be done with a locked mutex.

* wtf/gobject/GMainLoopSource.cpp:
(WTF::GMainLoopSource::GMainLoopSource):
(WTF::GMainLoopSource::~GMainLoopSource):
(WTF::GMainLoopSource::cancel):
(WTF::GMainLoopSource::cancelWithoutLocking):
(WTF::GMainLoopSource::scheduleIdleSource):
(WTF::GMainLoopSource::schedule):
(WTF::GMainLoopSource::scheduleTimeoutSource):
(WTF::GMainLoopSource::scheduleAfterDelay):
(WTF::GMainLoopSource::voidCallback):
(WTF::GMainLoopSource::boolCallback):
(WTF::GMainLoopSource::socketCallback):
(WTF::GMainLoopSource::socketSourceCallback):
(WTF::GMainLoopSource::Context::destroySource):
(WTF::GMainLoopSource::reset): Deleted.
(WTF::GMainLoopSource::destroy): Deleted.
* wtf/gobject/GMainLoopSource.h:

Tools:

Add unit tests for GMainLoopSource.

The tests check correct behavior of GMainLoopSource in various conditions --
from the most simple rescheduling to rescheduling during dispatch, cancelling
or destroying the GMainLoopSource during dispatch, proper destroy callback
dispatching etc.

Scheduling both void (one-time) and bool (repeatable) callbacks is tested.
State of the GMainLoopSource object (either ready, sheduled or active) is
thoroughly tested throughout the lifetime of that object.

Still missing are tests for socket callbacks, which are a bit trickier because
they rely on a GSocket object. The delete-on-destroy GMainLoopSource objects
are also not tested thoroughly, simply because it is at the moment impossible
to test that the objects are actually destroyed when the corresponding source
is finally deleted.

* TestWebKitAPI/PlatformGTK.cmake:
* TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp: Added.
(TestWebKitAPI::GMainLoopSourceTest::GMainLoopSourceTest):
(TestWebKitAPI::GMainLoopSourceTest::~GMainLoopSourceTest):
(TestWebKitAPI::GMainLoopSourceTest::runLoop):
(TestWebKitAPI::GMainLoopSourceTest::delayedFinish):
(TestWebKitAPI::GMainLoopSourceTest::finish):
(TestWebKitAPI::GMainLoopSourceTest::source):
(TestWebKitAPI::TEST):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@173720 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WTF/ChangeLog
Source/WTF/wtf/gobject/GMainLoopSource.cpp
Source/WTF/wtf/gobject/GMainLoopSource.h
Tools/ChangeLog
Tools/TestWebKitAPI/PlatformGTK.cmake
Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp [new file with mode: 0644]

index 6fae1c3..62cc5dc 100644 (file)
@@ -1,3 +1,60 @@
+2014-09-18  Zan Dobersek  <zdobersek@igalia.com>
+
+        GMainLoopSource is exposed to race conditions
+        https://bugs.webkit.org/show_bug.cgi?id=135800
+
+        Reviewed by Carlos Garcia Campos.
+
+        GMainLoopSource objects can be dispatching tasks on one thread
+        while having a new task scheduled on a different thread. This
+        can for instance occur in WebKitVideoSink, where the timeout
+        callback can be called on main thread while at the same time
+        it is being rescheduled on a different thread (created through
+        GStreamer).
+
+        The initial solution is to use GMutex to prevent parallel data
+        access from different threads. In the future I plan to look at
+        the possibility of creating thread-specific GMainLoopSource
+        objects that wouldn't require the use of GMutex.
+
+        GSource, GCancellable and std::function<> objects are now packed
+        into an internal Context structure. Using the C++11 move semantics
+        it's simple to, at the time of dispatch, move the current context
+        out of the GMainLoopSource object in case the dispatch causes a
+        rescheduling on that same object.
+
+        Also added in the Context struct is a new GCancellable. The pointer
+        of that object is shared with the GMainLoopSource before the Context
+        is moved out for the callback dispatch. This makes it safe to cancel
+        or even delete the GMainLoopSource during the dispatch and prevents
+        use-after-delete on GMainLoopSource once the dispatch is done in
+        the GMainLoopSource::*Callback() methods.
+
+        All the schedule*() methods and the cancelWithoutLocking() method
+        callers now lock the GMutex to ensure no one else is accessing the
+        data at that moment. Similar goes for the dispatch methods, but those
+        do the dispatch and possible destruction duties with the mutex unlocked.
+        The dispatch can cause rescheduling on the same GMainLoopSource object,
+        which must not be done with a locked mutex.
+
+        * wtf/gobject/GMainLoopSource.cpp:
+        (WTF::GMainLoopSource::GMainLoopSource):
+        (WTF::GMainLoopSource::~GMainLoopSource):
+        (WTF::GMainLoopSource::cancel):
+        (WTF::GMainLoopSource::cancelWithoutLocking):
+        (WTF::GMainLoopSource::scheduleIdleSource):
+        (WTF::GMainLoopSource::schedule):
+        (WTF::GMainLoopSource::scheduleTimeoutSource):
+        (WTF::GMainLoopSource::scheduleAfterDelay):
+        (WTF::GMainLoopSource::voidCallback):
+        (WTF::GMainLoopSource::boolCallback):
+        (WTF::GMainLoopSource::socketCallback):
+        (WTF::GMainLoopSource::socketSourceCallback):
+        (WTF::GMainLoopSource::Context::destroySource):
+        (WTF::GMainLoopSource::reset): Deleted.
+        (WTF::GMainLoopSource::destroy): Deleted.
+        * wtf/gobject/GMainLoopSource.h:
+
 2014-09-17  Daniel Bates  <dabates@apple.com>
 
         Unreviewed, rolling out r173695.
index 6d8c50b..e1d24e0 100644 (file)
@@ -28,8 +28,8 @@
 #if USE(GLIB)
 
 #include "GMainLoopSource.h"
-
 #include <gio/gio.h>
+#include <wtf/gobject/GMutexLocker.h>
 
 namespace WTF {
 
@@ -42,17 +42,20 @@ GMainLoopSource::GMainLoopSource()
     : m_deleteOnDestroy(DoNotDeleteOnDestroy)
     , m_status(Ready)
 {
+    g_mutex_init(&m_mutex);
 }
 
 GMainLoopSource::GMainLoopSource(DeleteOnDestroyType deleteOnDestroy)
     : m_deleteOnDestroy(deleteOnDestroy)
     , m_status(Ready)
 {
+    g_mutex_init(&m_mutex);
 }
 
 GMainLoopSource::~GMainLoopSource()
 {
     cancel();
+    g_mutex_clear(&m_mutex);
 }
 
 bool GMainLoopSource::isScheduled() const
@@ -67,26 +70,34 @@ bool GMainLoopSource::isActive() const
 
 void GMainLoopSource::cancel()
 {
-    if (!m_source)
-        return;
-
-    GRefPtr<GSource> source;
-    m_source.swap(source);
-
-    if (m_cancellable)
-        g_cancellable_cancel(m_cancellable.get());
-    g_source_destroy(source.get());
-    destroy();
+    GMutexLocker locker(m_mutex);
+    cancelWithoutLocking();
 }
 
-void GMainLoopSource::reset()
+void GMainLoopSource::cancelWithoutLocking()
 {
+    // A valid context should only be present if GMainLoopSource is in the Scheduled or Dispatching state.
+    ASSERT(!m_context.source || m_status == Scheduled || m_status == Dispatching);
+    // The general cancellable object should only be present if we're currently dispatching this GMainLoopSource.
+    ASSERT(!m_cancellable || m_status == Dispatching);
+    // Delete-on-destroy GMainLoopSource objects can only be cancelled when there's callback either scheduled
+    // or in the middle of dispatch. At that point cancellation will have no effect.
+    ASSERT(m_deleteOnDestroy != DeleteOnDestroy || (m_status == Ready && !m_context.source));
+
     m_status = Ready;
-    m_source = nullptr;
+
+    // The source is perhaps being cancelled in the middle of a callback dispatch.
+    // Cancelling this GCancellable object will convey this information to the
+    // current execution context when the callback dispatch is finished.
+    g_cancellable_cancel(m_cancellable.get());
     m_cancellable = nullptr;
-    m_voidCallback = nullptr;
-    m_boolCallback = nullptr;
-    m_destroyCallback = nullptr;
+    g_cancellable_cancel(m_context.socketCancellable.get());
+
+    if (!m_context.source)
+        return;
+
+    Context context = WTF::move(m_context);
+    context.destroySource();
 }
 
 void GMainLoopSource::scheduleIdleSource(const char* name, GSourceFunc sourceFunction, int priority, GMainContext* context)
@@ -94,43 +105,74 @@ void GMainLoopSource::scheduleIdleSource(const char* name, GSourceFunc sourceFun
     ASSERT(m_status == Ready);
     m_status = Scheduled;
 
-    m_source = adoptGRef(g_idle_source_new());
-    g_source_set_name(m_source.get(), name);
+    g_source_set_name(m_context.source.get(), name);
     if (priority != G_PRIORITY_DEFAULT_IDLE)
-        g_source_set_priority(m_source.get(), priority);
-    g_source_set_callback(m_source.get(), sourceFunction, this, nullptr);
-    g_source_attach(m_source.get(), context);
+        g_source_set_priority(m_context.source.get(), priority);
+    g_source_set_callback(m_context.source.get(), sourceFunction, this, nullptr);
+    g_source_attach(m_context.source.get(), context);
 }
 
 void GMainLoopSource::schedule(const char* name, std::function<void ()> function, int priority, std::function<void ()> destroyFunction, GMainContext* context)
 {
-    cancel();
-    m_voidCallback = WTF::move(function);
-    m_destroyCallback = WTF::move(destroyFunction);
+    GMutexLocker locker(m_mutex);
+    cancelWithoutLocking();
+
+    ASSERT(!m_context.source);
+    m_context = {
+        adoptGRef(g_idle_source_new()),
+        adoptGRef(g_cancellable_new()),
+        nullptr, // socketCancellable
+        WTF::move(function),
+        nullptr, // boolCallback
+        nullptr, // socketCallback
+        WTF::move(destroyFunction)
+    };
     scheduleIdleSource(name, reinterpret_cast<GSourceFunc>(voidSourceCallback), priority, context);
 }
 
 void GMainLoopSource::schedule(const char* name, std::function<bool ()> function, int priority, std::function<void ()> destroyFunction, GMainContext* context)
 {
-    cancel();
-    m_boolCallback = WTF::move(function);
-    m_destroyCallback = WTF::move(destroyFunction);
+    GMutexLocker locker(m_mutex);
+    cancelWithoutLocking();
+
+    ASSERT(!m_context.source);
+    m_context = {
+        adoptGRef(g_idle_source_new()),
+        adoptGRef(g_cancellable_new()),
+        nullptr, // socketCancellable
+        nullptr, // voidCallback
+        WTF::move(function),
+        nullptr, // socketCallback
+        WTF::move(destroyFunction)
+    };
     scheduleIdleSource(name, reinterpret_cast<GSourceFunc>(boolSourceCallback), priority, context);
 }
 
 void GMainLoopSource::schedule(const char* name, std::function<bool (GIOCondition)> function, GSocket* socket, GIOCondition condition, std::function<void ()> destroyFunction, GMainContext* context)
 {
-    cancel();
+    GMutexLocker locker(m_mutex);
+    cancelWithoutLocking();
+
+    // Don't allow scheduling GIOCondition callbacks on delete-on-destroy GMainLoopSources.
+    ASSERT(m_deleteOnDestroy == DoNotDeleteOnDestroy);
+
+    ASSERT(!m_context.source);
+    GCancellable* socketCancellable = g_cancellable_new();
+    m_context = {
+        adoptGRef(g_socket_create_source(socket, condition, socketCancellable)),
+        adoptGRef(g_cancellable_new()),
+        adoptGRef(socketCancellable),
+        nullptr, // voidCallback
+        nullptr, // boolCallback
+        WTF::move(function),
+        WTF::move(destroyFunction)
+    };
+
     ASSERT(m_status == Ready);
     m_status = Scheduled;
-
-    m_socketCallback = WTF::move(function);
-    m_destroyCallback = WTF::move(destroyFunction);
-    m_cancellable = adoptGRef(g_cancellable_new());
-    m_source = adoptGRef(g_socket_create_source(socket, condition, m_cancellable.get()));
-    g_source_set_name(m_source.get(), name);
-    g_source_set_callback(m_source.get(), reinterpret_cast<GSourceFunc>(socketSourceCallback), this, nullptr);
-    g_source_attach(m_source.get(), context);
+    g_source_set_name(m_context.source.get(), name);
+    g_source_set_callback(m_context.source.get(), reinterpret_cast<GSourceFunc>(socketSourceCallback), this, nullptr);
+    g_source_attach(m_context.source.get(), context);
 }
 
 void GMainLoopSource::scheduleTimeoutSource(const char* name, GSourceFunc sourceFunction, int priority, GMainContext* context)
@@ -138,116 +180,218 @@ void GMainLoopSource::scheduleTimeoutSource(const char* name, GSourceFunc source
     ASSERT(m_status == Ready);
     m_status = Scheduled;
 
-    ASSERT(m_source);
-    g_source_set_name(m_source.get(), name);
+    g_source_set_name(m_context.source.get(), name);
     if (priority != G_PRIORITY_DEFAULT)
-        g_source_set_priority(m_source.get(), priority);
-    g_source_set_callback(m_source.get(), sourceFunction, this, nullptr);
-    g_source_attach(m_source.get(), context);
+        g_source_set_priority(m_context.source.get(), priority);
+    g_source_set_callback(m_context.source.get(), sourceFunction, this, nullptr);
+    g_source_attach(m_context.source.get(), context);
 }
 
 void GMainLoopSource::scheduleAfterDelay(const char* name, std::function<void ()> function, std::chrono::milliseconds delay, int priority, std::function<void ()> destroyFunction, GMainContext* context)
 {
-    cancel();
-    m_source = adoptGRef(g_timeout_source_new(delay.count()));
-    m_voidCallback = WTF::move(function);
-    m_destroyCallback = WTF::move(destroyFunction);
+    GMutexLocker locker(m_mutex);
+    cancelWithoutLocking();
+
+    ASSERT(!m_context.source);
+    m_context = {
+        adoptGRef(g_timeout_source_new(delay.count())),
+        adoptGRef(g_cancellable_new()),
+        nullptr, // socketCancellable
+        WTF::move(function),
+        nullptr, // boolCallback
+        nullptr, // socketCallback
+        WTF::move(destroyFunction)
+    };
     scheduleTimeoutSource(name, reinterpret_cast<GSourceFunc>(voidSourceCallback), priority, context);
 }
 
 void GMainLoopSource::scheduleAfterDelay(const char* name, std::function<bool ()> function, std::chrono::milliseconds delay, int priority, std::function<void ()> destroyFunction, GMainContext* context)
 {
-    cancel();
-    m_source = adoptGRef(g_timeout_source_new(delay.count()));
-    m_boolCallback = WTF::move(function);
-    m_destroyCallback = WTF::move(destroyFunction);
+    GMutexLocker locker(m_mutex);
+    cancelWithoutLocking();
+
+    ASSERT(!m_context.source);
+    m_context = {
+        adoptGRef(g_timeout_source_new(delay.count())),
+        adoptGRef(g_cancellable_new()),
+        nullptr, // socketCancellable
+        nullptr, // voidCallback
+        WTF::move(function),
+        nullptr, // socketCallback
+        WTF::move(destroyFunction)
+    };
     scheduleTimeoutSource(name, reinterpret_cast<GSourceFunc>(boolSourceCallback), priority, context);
 }
 
 void GMainLoopSource::scheduleAfterDelay(const char* name, std::function<void ()> function, std::chrono::seconds delay, int priority, std::function<void ()> destroyFunction, GMainContext* context)
 {
-    cancel();
-    m_source = adoptGRef(g_timeout_source_new_seconds(delay.count()));
-    m_voidCallback = WTF::move(function);
-    m_destroyCallback = WTF::move(destroyFunction);
+    GMutexLocker locker(m_mutex);
+    cancelWithoutLocking();
+
+    ASSERT(!m_context.source);
+    m_context = {
+        adoptGRef(g_timeout_source_new_seconds(delay.count())),
+        adoptGRef(g_cancellable_new()),
+        nullptr, // socketCancellable
+        WTF::move(function),
+        nullptr, // boolCallback
+        nullptr, // socketCallback
+        WTF::move(destroyFunction)
+    };
     scheduleTimeoutSource(name, reinterpret_cast<GSourceFunc>(voidSourceCallback), priority, context);
 }
 
 void GMainLoopSource::scheduleAfterDelay(const char* name, std::function<bool ()> function, std::chrono::seconds delay, int priority, std::function<void ()> destroyFunction, GMainContext* context)
 {
-    cancel();
-    m_source = adoptGRef(g_timeout_source_new_seconds(delay.count()));
-    m_boolCallback = WTF::move(function);
-    m_destroyCallback = WTF::move(destroyFunction);
+    GMutexLocker locker(m_mutex);
+    cancelWithoutLocking();
+
+    ASSERT(!m_context.source);
+    m_context = {
+        adoptGRef(g_timeout_source_new_seconds(delay.count())),
+        adoptGRef(g_cancellable_new()),
+        nullptr, // socketCancellable
+        nullptr, // voidCallback
+        WTF::move(function),
+        nullptr, // socketCallback
+        WTF::move(destroyFunction)
+    };
     scheduleTimeoutSource(name, reinterpret_cast<GSourceFunc>(boolSourceCallback), priority, context);
 }
 
 void GMainLoopSource::voidCallback()
 {
-    if (!m_source)
+    Context context;
+
+    {
+        GMutexLocker locker(m_mutex);
+        if (!m_context.source)
+            return;
+
+        context = WTF::move(m_context);
+
+        ASSERT(context.voidCallback);
+        ASSERT(m_status == Scheduled);
+        m_status = Dispatching;
+
+        m_cancellable = context.cancellable;
+    }
+
+    context.voidCallback();
+
+    if (g_cancellable_is_cancelled(context.cancellable.get())) {
+        context.destroySource();
         return;
+    }
 
-    ASSERT(m_voidCallback);
-    ASSERT(m_status == Scheduled);
-    m_status = Dispatched;
+    bool shouldSelfDestruct = false;
+    {
+        GMutexLocker locker(m_mutex);
+        m_status = Ready;
+        m_cancellable = nullptr;
+        shouldSelfDestruct = m_deleteOnDestroy == DeleteOnDestroy;
+    }
 
-    GSource* source = m_source.get();
-    m_voidCallback();
-    if (source == m_source.get())
-        destroy();
+    context.destroySource();
+    if (shouldSelfDestruct)
+        delete this;
 }
 
 bool GMainLoopSource::boolCallback()
 {
-    if (!m_source)
-        return false;
+    Context context;
+
+    {
+        GMutexLocker locker(m_mutex);
+        if (!m_context.source)
+            return Stop;
 
-    ASSERT(m_boolCallback);
-    ASSERT(m_status == Scheduled || m_status == Dispatched);
-    m_status = Dispatched;
+        context = WTF::move(m_context);
 
-    GSource* source = m_source.get();
-    bool retval = m_boolCallback();
-    if (!retval && source == m_source.get())
-        destroy();
+        ASSERT(context.boolCallback);
+        ASSERT(m_status == Scheduled || m_status == Dispatching);
+        m_status = Dispatching;
+
+        m_cancellable = context.cancellable;
+    }
+
+    bool retval = context.boolCallback();
+
+    if (g_cancellable_is_cancelled(context.cancellable.get())) {
+        context.destroySource();
+        return Stop;
+    }
+
+    bool shouldSelfDestruct = false;
+    {
+        GMutexLocker locker(m_mutex);
+        m_cancellable = nullptr;
+        shouldSelfDestruct = m_deleteOnDestroy == DeleteOnDestroy;
+
+        // m_status should reflect whether the GMainLoopSource has been rescheduled during dispatch.
+        ASSERT((!m_context.source && m_status == Dispatching) || m_status == Scheduled);
+        if (retval && !m_context.source)
+            m_context = WTF::move(context);
+        else if (!retval)
+            m_status = Ready;
+    }
+
+    if (context.source) {
+        context.destroySource();
+        if (shouldSelfDestruct)
+            delete this;
+    }
 
     return retval;
 }
 
 bool GMainLoopSource::socketCallback(GIOCondition condition)
 {
-    if (!m_source)
-        return false;
+    Context context;
 
-    ASSERT(m_socketCallback);
-    ASSERT(m_status == Scheduled || m_status == Dispatched);
-    m_status = Dispatched;
+    {
+        GMutexLocker locker(m_mutex);
+        if (!m_context.source)
+            return Stop;
 
-    if (g_cancellable_is_cancelled(m_cancellable.get())) {
-        destroy();
-        return false;
+        context = WTF::move(m_context);
+
+        ASSERT(context.socketCallback);
+        ASSERT(m_status == Scheduled || m_status == Dispatching);
+        m_status = Dispatching;
+
+        m_cancellable = context.cancellable;
     }
 
-    GSource* source = m_source.get();
-    bool retval = m_socketCallback(condition);
-    if (!retval && source == m_source.get())
-        destroy();
+    if (g_cancellable_is_cancelled(context.socketCancellable.get())) {
+        context.destroySource();
+        return Stop;
+    }
 
-    return retval;
-}
+    bool retval = context.socketCallback(condition);
 
-void GMainLoopSource::destroy()
-{
-    auto destroyCallback = WTF::move(m_destroyCallback);
-    auto deleteOnDestroy = m_deleteOnDestroy;
-    reset();
-    if (destroyCallback)
-        destroyCallback();
+    if (g_cancellable_is_cancelled(context.cancellable.get())) {
+        context.destroySource();
+        return Stop;
+    }
 
-    if (deleteOnDestroy == DoNotDeleteOnDestroy)
-        return;
+    {
+        GMutexLocker locker(m_mutex);
+        m_cancellable = nullptr;
+
+        // m_status should reflect whether the GMainLoopSource has been rescheduled during dispatch.
+        ASSERT((!m_context.source && m_status == Dispatching) || m_status == Scheduled);
+
+        if (retval && !m_context.source)
+            m_context = WTF::move(context);
+        else if (!retval)
+            m_status = Ready;
+    }
+
+    if (context.source)
+        context.destroySource();
 
-    delete this;
+    return retval;
 }
 
 gboolean GMainLoopSource::voidSourceCallback(GMainLoopSource* source)
@@ -266,6 +410,13 @@ gboolean GMainLoopSource::socketSourceCallback(GSocket*, GIOCondition condition,
     return source->socketCallback(condition) == Continue;
 }
 
+void GMainLoopSource::Context::destroySource()
+{
+    g_source_destroy(source.get());
+    if (destroyCallback)
+        destroyCallback();
+}
+
 } // namespace WTF
 
 #endif // USE(GLIB)
index 93ee482..ef05afc 100644 (file)
@@ -32,6 +32,7 @@
 #include <wtf/gobject/GRefPtr.h>
 
 typedef struct _GSocket GSocket;
+typedef union _GMutex GMutex;
 
 namespace WTF {
 
@@ -63,14 +64,15 @@ private:
     enum DeleteOnDestroyType { DeleteOnDestroy, DoNotDeleteOnDestroy };
     GMainLoopSource(DeleteOnDestroyType);
 
-    enum Status { Ready, Scheduled, Dispatched };
+    enum Status { Ready, Scheduled, Dispatching };
 
-    void reset();
+    void cancelWithoutLocking();
     void scheduleIdleSource(const char* name, GSourceFunc, int priority, GMainContext*);
     void scheduleTimeoutSource(const char* name, GSourceFunc, int priority, GMainContext*);
     void voidCallback();
     bool boolCallback();
     bool socketCallback(GIOCondition);
+
     void destroy();
 
     static gboolean voidSourceCallback(GMainLoopSource*);
@@ -79,12 +81,24 @@ private:
 
     DeleteOnDestroyType m_deleteOnDestroy;
     Status m_status;
-    GRefPtr<GSource> m_source;
+    GMutex m_mutex;
     GRefPtr<GCancellable> m_cancellable;
-    std::function<void ()> m_voidCallback;
-    std::function<bool ()> m_boolCallback;
-    std::function<bool (GIOCondition)> m_socketCallback;
-    std::function<void ()> m_destroyCallback;
+
+    struct Context {
+        Context() = default;
+        Context(Context&&) = default;
+        Context& operator=(Context&&) = default;
+
+        void destroySource();
+
+        GRefPtr<GSource> source;
+        GRefPtr<GCancellable> cancellable;
+        GRefPtr<GCancellable> socketCancellable;
+        std::function<void ()> voidCallback;
+        std::function<bool ()> boolCallback;
+        std::function<bool (GIOCondition)> socketCallback;
+        std::function<void ()> destroyCallback;
+    } m_context;
 };
 
 } // namespace WTF
index 3d2dfce..18e68b6 100644 (file)
@@ -1,3 +1,37 @@
+2014-09-18  Zan Dobersek  <zdobersek@igalia.com>
+
+        GMainLoopSource is exposed to race conditions
+        https://bugs.webkit.org/show_bug.cgi?id=135800
+
+        Reviewed by Carlos Garcia Campos.
+
+        Add unit tests for GMainLoopSource.
+
+        The tests check correct behavior of GMainLoopSource in various conditions --
+        from the most simple rescheduling to rescheduling during dispatch, cancelling
+        or destroying the GMainLoopSource during dispatch, proper destroy callback
+        dispatching etc.
+
+        Scheduling both void (one-time) and bool (repeatable) callbacks is tested.
+        State of the GMainLoopSource object (either ready, sheduled or active) is
+        thoroughly tested throughout the lifetime of that object.
+
+        Still missing are tests for socket callbacks, which are a bit trickier because
+        they rely on a GSocket object. The delete-on-destroy GMainLoopSource objects
+        are also not tested thoroughly, simply because it is at the moment impossible
+        to test that the objects are actually destroyed when the corresponding source
+        is finally deleted.
+
+        * TestWebKitAPI/PlatformGTK.cmake:
+        * TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp: Added.
+        (TestWebKitAPI::GMainLoopSourceTest::GMainLoopSourceTest):
+        (TestWebKitAPI::GMainLoopSourceTest::~GMainLoopSourceTest):
+        (TestWebKitAPI::GMainLoopSourceTest::runLoop):
+        (TestWebKitAPI::GMainLoopSourceTest::delayedFinish):
+        (TestWebKitAPI::GMainLoopSourceTest::finish):
+        (TestWebKitAPI::GMainLoopSourceTest::source):
+        (TestWebKitAPI::TEST):
+
 2014-09-17  Ryuan Choi  <ryuan.choi@gmail.com>
 
         Unreviewed, Update my email in contributors.json
index 23a35df..2a48048 100644 (file)
@@ -136,5 +136,6 @@ set_tests_properties(TestWebCore PROPERTIES TIMEOUT 60)
 set_target_properties(TestWebCore PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${TESTWEBKITAPI_RUNTIME_OUTPUT_DIRECTORY}/WebCore)
 
 list(APPEND TestWTF_SOURCES
+    ${TESTWEBKITAPI_DIR}/Tests/WTF/gobject/GMainLoopSource.cpp
     ${TESTWEBKITAPI_DIR}/Tests/WTF/gobject/GUniquePtr.cpp
 )
diff --git a/Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp b/Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp
new file mode 100644 (file)
index 0000000..d1b5dfe
--- /dev/null
@@ -0,0 +1,489 @@
+/*
+ * Copyright (C) 2014 Igalia S.L.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Library General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Library General Public License for more details.
+ *
+ * You should have received a copy of the GNU Library General Public License
+ * along with this library; see the file COPYING.LIB.  If not, write to
+ * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
+ * Boston, MA 02110-1301, USA.
+ */
+
+#include "config.h"
+
+#include <wtf/gobject/GMainLoopSource.h>
+#include <stdio.h>
+
+namespace TestWebKitAPI {
+
+class GMainLoopSourceTest {
+public:
+    GMainLoopSourceTest()
+        : m_mainLoop(g_main_loop_new(nullptr, TRUE))
+    {
+    }
+
+    ~GMainLoopSourceTest()
+    {
+        g_main_loop_unref(m_mainLoop);
+    }
+
+    void runLoop()
+    {
+        g_main_loop_run(m_mainLoop);
+    }
+
+    void delayedFinish()
+    {
+        g_timeout_add(250,
+            [](gpointer data) {
+                GMainLoopSourceTest& test = *static_cast<GMainLoopSourceTest*>(data);
+                test.finish();
+                return G_SOURCE_REMOVE;
+            }, this);
+    }
+
+    void finish()
+    {
+        g_main_loop_quit(m_mainLoop);
+    }
+
+    GMainLoopSource& source() { return m_source; }
+
+private:
+    GMainLoop* m_mainLoop;
+    GMainLoopSource m_source;
+};
+
+TEST(WTF_GMainLoopSource, BasicRescheduling)
+{
+    struct TestingContext {
+        GMainLoopSourceTest test;
+        bool finishedFirstTask = false;
+        bool finishedSecondTask = false;
+    } context;
+
+    EXPECT_TRUE(!context.test.source().isActive());
+
+    context.test.source().schedule("[Test] FirstTask", [&] {
+        // This should never be called. That's why we assert
+        // that the variable is false a few lines later.
+        context.finishedFirstTask = true;
+    });
+    EXPECT_TRUE(context.test.source().isScheduled());
+
+    context.test.source().schedule("[Test] SecondTask", [&] {
+        EXPECT_TRUE(context.test.source().isActive() && !context.test.source().isScheduled());
+        context.finishedSecondTask = true;
+        context.test.finish();
+    });
+    EXPECT_TRUE(context.test.source().isScheduled());
+
+    context.test.runLoop();
+
+    EXPECT_TRUE(!context.test.source().isActive());
+    EXPECT_FALSE(context.finishedFirstTask);
+    EXPECT_TRUE(context.finishedSecondTask);
+}
+
+TEST(WTF_GMainLoopSource, ReentrantRescheduling)
+{
+    struct TestingContext {
+        GMainLoopSourceTest test;
+        bool finishedFirstTask = false;
+        bool finishedSecondTask = false;
+    } context;
+
+    EXPECT_TRUE(!context.test.source().isActive());
+
+    context.test.source().schedule("[Test] FirstTask", [&] {
+        EXPECT_TRUE(context.test.source().isActive() && !context.test.source().isScheduled());
+
+        context.test.source().schedule("[Test] SecondTask", [&] {
+            EXPECT_TRUE(context.test.source().isActive() && !context.test.source().isScheduled());
+            EXPECT_TRUE(context.finishedFirstTask);
+
+            context.finishedSecondTask = true;
+            context.test.finish();
+        });
+        EXPECT_TRUE(context.test.source().isScheduled());
+
+        context.finishedFirstTask = true;
+    });
+    EXPECT_TRUE(context.test.source().isScheduled());
+
+    context.test.runLoop();
+
+    EXPECT_TRUE(!context.test.source().isActive());
+    EXPECT_TRUE(context.finishedFirstTask);
+    EXPECT_TRUE(context.finishedSecondTask);
+}
+
+TEST(WTF_GMainLoopSource, ReschedulingFromDifferentThread)
+{
+    struct TestingContext {
+        GMainLoopSourceTest test;
+        bool finishedFirstTask;
+        bool finishedSecondTask;
+    } context;
+
+    EXPECT_TRUE(!context.test.source().isActive());
+
+    context.test.source().schedule("[Test] FirstTask", [&] {
+        EXPECT_TRUE(context.test.source().isActive() && !context.test.source().isScheduled());
+
+        g_usleep(1 * G_USEC_PER_SEC);
+        context.finishedFirstTask = true;
+    });
+    EXPECT_TRUE(context.test.source().isScheduled());
+
+    GThread* helperThread = g_thread_new(nullptr, [](gpointer data) -> gpointer {
+        g_usleep(0.25 * G_USEC_PER_SEC);
+
+        TestingContext& context = *static_cast<TestingContext*>(data);
+        EXPECT_TRUE(context.test.source().isActive() && !context.test.source().isScheduled());
+        EXPECT_FALSE(context.finishedFirstTask);
+
+        context.test.source().schedule("[Test] SecondTask", [&] {
+            EXPECT_TRUE(context.finishedFirstTask);
+
+            context.finishedSecondTask = true;
+            context.test.finish();
+        });
+        EXPECT_TRUE(context.test.source().isScheduled());
+
+        g_thread_exit(nullptr);
+        return nullptr;
+    }, &context);
+
+    context.test.runLoop();
+    g_thread_unref(helperThread);
+
+    EXPECT_TRUE(!context.test.source().isActive());
+    EXPECT_TRUE(context.finishedFirstTask);
+    EXPECT_TRUE(context.finishedSecondTask);
+}
+
+TEST(WTF_GMainLoopSource, DestructionDuringDispatch)
+{
+    // This is just a raw test that ensures deleting the GMainLoopSource object during
+    // dispatch does not cause problems. This test succeeds if it doesn't crash.
+
+    GMainLoopSource* source;
+    GMainLoop* loop = g_main_loop_new(nullptr, TRUE);
+
+    source = new GMainLoopSource;
+    source->schedule("[Test] DestroySourceTask", [&] {
+        delete source;
+        g_main_loop_quit(loop);
+    });
+    g_main_loop_run(loop);
+
+    source = new GMainLoopSource;
+    source->schedule("[Test] DestroySourceTask", std::function<bool ()>([&] {
+        delete source;
+        g_main_loop_quit(loop);
+        return false;
+    }));
+    g_main_loop_run(loop);
+
+    g_main_loop_unref(loop);
+}
+
+TEST(WTF_GMainLoopSource, CancelRepeatingSourceDuringDispatch)
+{
+    struct TestingContext {
+        GMainLoopSourceTest test;
+        unsigned callCount = 0;
+    } context;
+
+    EXPECT_TRUE(!context.test.source().isActive());
+
+    context.test.source().schedule("[Test] RepeatingTask",
+        std::function<bool ()>([&] {
+            EXPECT_TRUE(context.test.source().isActive() && !context.test.source().isScheduled());
+
+            context.callCount++;
+            if (context.callCount == 3)
+                context.test.source().cancel();
+            return true;
+        }));
+    EXPECT_TRUE(context.test.source().isScheduled());
+
+    context.test.delayedFinish();
+    context.test.runLoop();
+
+    EXPECT_TRUE(!context.test.source().isActive());
+    EXPECT_EQ(3, context.callCount);
+}
+
+TEST(WTF_GMainLoopSource, BasicDestroyCallbacks)
+{
+    struct TestingContext {
+        GMainLoopSourceTest test;
+        bool callbackCalled = false;
+        bool destroyCallbackCalled = false;
+    };
+
+    {
+        TestingContext context;
+        EXPECT_TRUE(!context.test.source().isActive());
+        context.test.source().schedule("[Test] DestroyCallback",
+            [&] {
+                EXPECT_TRUE(context.test.source().isActive() && !context.test.source().isScheduled());
+                context.callbackCalled = true;
+            }, G_PRIORITY_DEFAULT,
+            [&] {
+                EXPECT_TRUE(!context.test.source().isActive());
+                context.destroyCallbackCalled = true;
+                context.test.finish();
+            });
+        EXPECT_TRUE(context.test.source().isScheduled());
+
+        context.test.runLoop();
+
+        EXPECT_TRUE(!context.test.source().isActive());
+        EXPECT_TRUE(context.callbackCalled);
+        EXPECT_TRUE(context.destroyCallbackCalled);
+    }
+
+    {
+        TestingContext context;
+        EXPECT_TRUE(!context.test.source().isActive());
+        context.test.source().schedule("[Test] DestroyCallback",
+            std::function<bool ()>([&] {
+                EXPECT_TRUE(context.test.source().isActive() && !context.test.source().isScheduled());
+                context.callbackCalled = true;
+                return false;
+            }), G_PRIORITY_DEFAULT,
+            [&] {
+                EXPECT_TRUE(!context.test.source().isActive());
+                context.destroyCallbackCalled = true;
+                context.test.finish();
+            });
+        EXPECT_TRUE(context.test.source().isScheduled());
+
+        context.test.runLoop();
+
+        EXPECT_TRUE(!context.test.source().isActive());
+        EXPECT_TRUE(context.callbackCalled);
+        EXPECT_TRUE(context.destroyCallbackCalled);
+    }
+}
+
+TEST(WTF_GMainLoopSource, DestroyCallbacksAfterCancellingDuringDispatch)
+{
+    struct TestingContext {
+        GMainLoopSourceTest test;
+        unsigned callbackCallCount= 0;
+        bool destroyCallbackCalled = false;
+    };
+
+    {
+        TestingContext context;
+        EXPECT_TRUE(!context.test.source().isActive());
+        context.test.source().schedule("[Test] DestroyCallback",
+            [&] {
+                EXPECT_TRUE(context.test.source().isActive() && !context.test.source().isScheduled());
+                context.callbackCallCount++;
+                context.test.source().cancel();
+            }, G_PRIORITY_DEFAULT,
+            [&] {
+                EXPECT_TRUE(!context.test.source().isActive());
+                context.destroyCallbackCalled = true;
+                context.test.finish();
+            });
+        EXPECT_TRUE(context.test.source().isScheduled());
+
+        context.test.delayedFinish();
+        context.test.runLoop();
+
+        EXPECT_TRUE(!context.test.source().isActive());
+        EXPECT_EQ(1, context.callbackCallCount);
+        EXPECT_TRUE(context.destroyCallbackCalled);
+    }
+
+    {
+        TestingContext context;
+        EXPECT_TRUE(!context.test.source().isActive());
+        context.test.source().schedule("[Test] DestroyCallback",
+            std::function<bool ()>([&] {
+                EXPECT_TRUE(context.test.source().isActive() && !context.test.source().isScheduled());
+                context.callbackCallCount++;
+                if (context.callbackCallCount == 3)
+                    context.test.source().cancel();
+                return true;
+            }), G_PRIORITY_DEFAULT,
+            [&] {
+                EXPECT_TRUE(!context.test.source().isActive());
+                context.destroyCallbackCalled = true;
+            });
+        EXPECT_TRUE(context.test.source().isScheduled());
+
+        context.test.delayedFinish();
+        context.test.runLoop();
+
+        EXPECT_TRUE(!context.test.source().isActive());
+        EXPECT_EQ(3, context.callbackCallCount);
+        EXPECT_TRUE(context.destroyCallbackCalled);
+    }
+}
+
+TEST(WTF_GMainLoopSource, DestroyCallbacksAfterReschedulingDuringDispatch)
+{
+    struct TestingContext {
+        GMainLoopSourceTest test;
+        unsigned firstCallbackCallCount = 0;
+        bool firstDestroyCallbackCalled = false;
+        unsigned secondCallbackCallCount = 0;
+        bool secondDestroyCallbackCalled = false;
+    };
+
+    {
+        TestingContext context;
+        EXPECT_TRUE(!context.test.source().isActive());
+        context.test.source().schedule("[Test] BaseCallback",
+            [&] {
+                EXPECT_TRUE(context.test.source().isActive() && !context.test.source().isScheduled());
+                context.firstCallbackCallCount++;
+                context.test.source().schedule("[Test] ReschedulingCallback",
+                    [&] {
+                        EXPECT_TRUE(context.test.source().isActive() && !context.test.source().isScheduled());
+                        context.secondCallbackCallCount++;
+                    }, G_PRIORITY_DEFAULT,
+                    [&] {
+                        EXPECT_TRUE(!context.test.source().isActive());
+                        context.secondDestroyCallbackCalled = true;
+                    });
+                EXPECT_TRUE(context.test.source().isScheduled());
+            }, G_PRIORITY_DEFAULT,
+            [&] {
+                // At this point the GMainLoopSource has been rescheduled, ergo the Scheduled status.
+                EXPECT_TRUE(context.test.source().isScheduled());
+                context.firstDestroyCallbackCalled = true;
+            });
+        EXPECT_TRUE(context.test.source().isScheduled());
+
+        context.test.delayedFinish();
+        context.test.runLoop();
+
+        EXPECT_TRUE(!context.test.source().isActive());
+        EXPECT_EQ(1, context.firstCallbackCallCount);
+        EXPECT_TRUE(context.firstDestroyCallbackCalled);
+        EXPECT_EQ(1, context.secondCallbackCallCount);
+        EXPECT_TRUE(context.secondDestroyCallbackCalled);
+    }
+
+    {
+        TestingContext context;
+        EXPECT_TRUE(!context.test.source().isActive());
+        context.test.source().schedule("[Test] BaseCallback",
+            std::function<bool ()>([&] {
+                EXPECT_TRUE(context.test.source().isActive() && !context.test.source().isScheduled());
+                context.firstCallbackCallCount++;
+                context.test.source().schedule("[Test] ReschedulingCallback",
+                    std::function<bool ()>([&] {
+                        EXPECT_TRUE(context.test.source().isActive() && !context.test.source().isScheduled());
+                        context.secondCallbackCallCount++;
+                        return context.secondCallbackCallCount != 3;
+                    }), G_PRIORITY_DEFAULT,
+                    [&] {
+                        EXPECT_TRUE(!context.test.source().isActive());
+                        context.secondDestroyCallbackCalled = true;
+                    });
+                EXPECT_TRUE(context.test.source().isScheduled());
+                return true;
+            }), G_PRIORITY_DEFAULT,
+            [&] {
+                // At this point the GMainLoopSource has been rescheduled, ergo the Scheduled status.
+                EXPECT_TRUE(context.test.source().isScheduled());
+                context.firstDestroyCallbackCalled = true;
+            });
+        EXPECT_TRUE(context.test.source().isScheduled());
+
+        context.test.delayedFinish();
+        context.test.runLoop();
+
+        EXPECT_TRUE(!context.test.source().isActive());
+        EXPECT_EQ(1, context.firstCallbackCallCount);
+        EXPECT_TRUE(context.firstDestroyCallbackCalled);
+        EXPECT_EQ(3, context.secondCallbackCallCount);
+        EXPECT_TRUE(context.secondDestroyCallbackCalled);
+    }
+}
+
+TEST(WTF_GMainLoopSource, DeleteOnDestroySources)
+{
+    // Testing the delete-on-destroy sources is very limited. There's no good way
+    // of testing that the GMainLoopSource objects are deleted when their GSource
+    // is destroyed, and the socket callbacks shouldn't be scheduled on these types
+    // of GMainLoopSources (as we aggressively assert to prevent that).
+
+    struct TestingContext {
+        GMainLoopSourceTest test;
+        unsigned callbackCallCount = 0;
+        bool destroyCallbackCalled = false;
+    } context;
+
+    {
+        TestingContext context;
+
+        // We take a reference to the GMainLoopSource just to perform additional
+        // tests on its status. We shouldn't use the reference after the main loop
+        // exists since at that point the GMainLoopSource will be destroyed and
+        // the reference pointing to an invalid piece of memory.
+        GMainLoopSource& source = GMainLoopSource::createAndDeleteOnDestroy();
+        EXPECT_TRUE(!source.isActive());
+        source.schedule("[Test] DeleteOnDestroy",
+            [&] {
+                EXPECT_TRUE(source.isActive() && !source.isScheduled());
+                context.callbackCallCount++;
+            }, G_PRIORITY_DEFAULT,
+            [&] {
+                EXPECT_TRUE(!source.isActive());
+                EXPECT_FALSE(context.destroyCallbackCalled);
+                context.destroyCallbackCalled = true;
+            });
+        EXPECT_TRUE(source.isScheduled());
+
+        context.test.delayedFinish();
+        context.test.runLoop();
+        EXPECT_EQ(1, context.callbackCallCount);
+        EXPECT_TRUE(context.destroyCallbackCalled);
+    }
+
+    {
+        TestingContext context;
+
+        // As in the previous scope, we need a reference to the GMainLoopSource.
+        GMainLoopSource& source = GMainLoopSource::createAndDeleteOnDestroy();
+        EXPECT_TRUE(!source.isActive());
+        source.schedule("[Test] DeleteOnDestroy",
+            std::function<bool ()>([&] {
+                EXPECT_TRUE(source.isActive() && !source.isScheduled());
+                context.callbackCallCount++;
+                return context.callbackCallCount != 3;
+            }), G_PRIORITY_DEFAULT,
+            [&] {
+                EXPECT_TRUE(!source.isActive());
+                EXPECT_FALSE(context.destroyCallbackCalled);
+                context.destroyCallbackCalled = true;
+            });
+        EXPECT_TRUE(source.isScheduled());
+
+        context.test.delayedFinish();
+        context.test.runLoop();
+        EXPECT_EQ(3, context.callbackCallCount);
+        EXPECT_TRUE(context.destroyCallbackCalled);
+    }
+}
+
+} // namespace TestWebKitAPI