<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[173201] trunk</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/173201">173201</a></dd>
<dt>Author</dt> <dd>zandobersek@gmail.com</dd>
<dt>Date</dt> <dd>2014-09-03 00:26:52 -0700 (Wed, 03 Sep 2014)</dd>
</dl>
<h3>Log Message</h3>
<pre>GMainLoopSource is exposed to race conditions
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 add better
assertions, some meaningful comments and 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.
All the schedule*() methods and the cancelInternal() 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::cancelInternal):
(WTF::GMainLoopSource::scheduleIdleSource):
(WTF::GMainLoopSource::schedule):
(WTF::GMainLoopSource::scheduleTimeoutSource):
(WTF::GMainLoopSource::scheduleAfterDelay):
(WTF::GMainLoopSource::voidCallback):
(WTF::GMainLoopSource::boolCallback):
(WTF::GMainLoopSource::socketCallback):
(WTF::GMainLoopSource::destroy):
(WTF::GMainLoopSource::reset): Deleted.
* wtf/gobject/GMainLoopSource.h:
Tools:
Add a unit test for GMainLoopSource that tests different
types of rescheduling tasks on already-active sources.
* TestWebKitAPI/PlatformGTK.cmake:
* TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp: Added.
(GMainLoopSourceTest::GMainLoopSourceTest):
(GMainLoopSourceTest::~GMainLoopSourceTest):
(GMainLoopSourceTest::runLoop):
(GMainLoopSourceTest::finish):
(GMainLoopSourceTest::source):
(testGMainLoopSourceBasicRescheduling):
(testGMainLoopSourceReentrantRescheduling):
(testGMainLoopSourceDifferentThreadRescheduling):
(beforeAll):
(afterAll):
(TestWebKitAPI::GMainLoopSourceTest::GMainLoopSourceTest):
(TestWebKitAPI::GMainLoopSourceTest::~GMainLoopSourceTest):
(TestWebKitAPI::GMainLoopSourceTest::runLoop):
(TestWebKitAPI::GMainLoopSourceTest::finish):
(TestWebKitAPI::GMainLoopSourceTest::source):
(TestWebKitAPI::TEST):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceWTFChangeLog">trunk/Source/WTF/ChangeLog</a></li>
<li><a href="#trunkSourceWTFwtfgobjectGMainLoopSourcecpp">trunk/Source/WTF/wtf/gobject/GMainLoopSource.cpp</a></li>
<li><a href="#trunkSourceWTFwtfgobjectGMainLoopSourceh">trunk/Source/WTF/wtf/gobject/GMainLoopSource.h</a></li>
<li><a href="#trunkToolsChangeLog">trunk/Tools/ChangeLog</a></li>
<li><a href="#trunkToolsTestWebKitAPIPlatformGTKcmake">trunk/Tools/TestWebKitAPI/PlatformGTK.cmake</a></li>
</ul>
<h3>Added Paths</h3>
<ul>
<li><a href="#trunkToolsTestWebKitAPITestsWTFgobjectGMainLoopSourcecpp">trunk/Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceWTFChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WTF/ChangeLog (173200 => 173201)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WTF/ChangeLog 2014-09-03 06:04:44 UTC (rev 173200)
+++ trunk/Source/WTF/ChangeLog 2014-09-03 07:26:52 UTC (rev 173201)
</span><span class="lines">@@ -1,3 +1,52 @@
</span><ins>+2014-09-03 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 add better
+ assertions, some meaningful comments and 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.
+
+ All the schedule*() methods and the cancelInternal() 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::cancelInternal):
+ (WTF::GMainLoopSource::scheduleIdleSource):
+ (WTF::GMainLoopSource::schedule):
+ (WTF::GMainLoopSource::scheduleTimeoutSource):
+ (WTF::GMainLoopSource::scheduleAfterDelay):
+ (WTF::GMainLoopSource::voidCallback):
+ (WTF::GMainLoopSource::boolCallback):
+ (WTF::GMainLoopSource::socketCallback):
+ (WTF::GMainLoopSource::destroy):
+ (WTF::GMainLoopSource::reset): Deleted.
+ * wtf/gobject/GMainLoopSource.h:
+
</ins><span class="cx"> 2014-09-02 Daniel Bates <dabates@apple.com>
</span><span class="cx">
</span><span class="cx"> [iOS] Support using Foundation networking code
</span></span></pre></div>
<a id="trunkSourceWTFwtfgobjectGMainLoopSourcecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WTF/wtf/gobject/GMainLoopSource.cpp (173200 => 173201)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WTF/wtf/gobject/GMainLoopSource.cpp 2014-09-03 06:04:44 UTC (rev 173200)
+++ trunk/Source/WTF/wtf/gobject/GMainLoopSource.cpp 2014-09-03 07:26:52 UTC (rev 173201)
</span><span class="lines">@@ -28,8 +28,8 @@
</span><span class="cx"> #if USE(GLIB)
</span><span class="cx">
</span><span class="cx"> #include "GMainLoopSource.h"
</span><del>-
</del><span class="cx"> #include <gio/gio.h>
</span><ins>+#include <wtf/gobject/GMutexLocker.h>
</ins><span class="cx">
</span><span class="cx"> namespace WTF {
</span><span class="cx">
</span><span class="lines">@@ -42,17 +42,20 @@
</span><span class="cx"> : m_deleteOnDestroy(DoNotDeleteOnDestroy)
</span><span class="cx"> , m_status(Ready)
</span><span class="cx"> {
</span><ins>+ g_mutex_init(&m_mutex);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> GMainLoopSource::GMainLoopSource(DeleteOnDestroyType deleteOnDestroy)
</span><span class="cx"> : m_deleteOnDestroy(deleteOnDestroy)
</span><span class="cx"> , m_status(Ready)
</span><span class="cx"> {
</span><ins>+ g_mutex_init(&m_mutex);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> GMainLoopSource::~GMainLoopSource()
</span><span class="cx"> {
</span><span class="cx"> cancel();
</span><ins>+ g_mutex_clear(&m_mutex);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> bool GMainLoopSource::isScheduled() const
</span><span class="lines">@@ -67,26 +70,24 @@
</span><span class="cx">
</span><span class="cx"> void GMainLoopSource::cancel()
</span><span class="cx"> {
</span><del>- if (!m_source)
</del><ins>+ GMutexLocker locker(m_mutex);
+ cancelWithoutLocking();
+}
+
+void GMainLoopSource::cancelWithoutLocking()
+{
+ if (!m_context.source) {
+ m_status = Ready;
</ins><span class="cx"> return;
</span><ins>+ }
</ins><span class="cx">
</span><del>- GRefPtr<GSource> source;
- m_source.swap(source);
</del><ins>+ Context context = WTF::move(m_context);
</ins><span class="cx">
</span><del>- if (m_cancellable)
- g_cancellable_cancel(m_cancellable.get());
- g_source_destroy(source.get());
- destroy();
-}
</del><ins>+ if (context.cancellable)
+ g_cancellable_cancel(context.cancellable.get());
</ins><span class="cx">
</span><del>-void GMainLoopSource::reset()
-{
- m_status = Ready;
- m_source = nullptr;
- m_cancellable = nullptr;
- m_voidCallback = nullptr;
- m_boolCallback = nullptr;
- m_destroyCallback = nullptr;
</del><ins>+ g_source_destroy(context.source.get());
+ destroy(context.destroyCallback);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void GMainLoopSource::scheduleIdleSource(const char* name, GSourceFunc sourceFunction, int priority, GMainContext* context)
</span><span class="lines">@@ -94,43 +95,46 @@
</span><span class="cx"> ASSERT(m_status == Ready);
</span><span class="cx"> m_status = Scheduled;
</span><span class="cx">
</span><del>- m_source = adoptGRef(g_idle_source_new());
- g_source_set_name(m_source.get(), name);
</del><ins>+ m_context.source = adoptGRef(g_idle_source_new());
+ g_source_set_name(m_context.source.get(), name);
</ins><span class="cx"> if (priority != G_PRIORITY_DEFAULT_IDLE)
</span><del>- 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);
</del><ins>+ 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);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void GMainLoopSource::schedule(const char* name, std::function<void ()> function, int priority, std::function<void ()> destroyFunction, GMainContext* context)
</span><span class="cx"> {
</span><del>- cancel();
- m_voidCallback = WTF::move(function);
- m_destroyCallback = WTF::move(destroyFunction);
</del><ins>+ GMutexLocker locker(m_mutex);
+ cancelWithoutLocking();
+ m_context.voidCallback = WTF::move(function);
+ m_context.destroyCallback = WTF::move(destroyFunction);
</ins><span class="cx"> scheduleIdleSource(name, reinterpret_cast<GSourceFunc>(voidSourceCallback), priority, context);
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void GMainLoopSource::schedule(const char* name, std::function<bool ()> function, int priority, std::function<void ()> destroyFunction, GMainContext* context)
</span><span class="cx"> {
</span><del>- cancel();
- m_boolCallback = WTF::move(function);
- m_destroyCallback = WTF::move(destroyFunction);
</del><ins>+ GMutexLocker locker(m_mutex);
+ cancelWithoutLocking();
+ m_context.boolCallback = WTF::move(function);
+ m_context.destroyCallback = WTF::move(destroyFunction);
</ins><span class="cx"> scheduleIdleSource(name, reinterpret_cast<GSourceFunc>(boolSourceCallback), priority, context);
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void GMainLoopSource::schedule(const char* name, std::function<bool (GIOCondition)> function, GSocket* socket, GIOCondition condition, std::function<void ()> destroyFunction, GMainContext* context)
</span><span class="cx"> {
</span><del>- cancel();
</del><ins>+ GMutexLocker locker(m_mutex);
+ cancelWithoutLocking();
</ins><span class="cx"> ASSERT(m_status == Ready);
</span><span class="cx"> m_status = Scheduled;
</span><span class="cx">
</span><del>- 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);
</del><ins>+ m_context.socketCallback = WTF::move(function);
+ m_context.destroyCallback = WTF::move(destroyFunction);
+ m_context.cancellable = adoptGRef(g_cancellable_new());
+ m_context.source = adoptGRef(g_socket_create_source(socket, condition, m_context.cancellable.get()));
+ 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);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void GMainLoopSource::scheduleTimeoutSource(const char* name, GSourceFunc sourceFunction, int priority, GMainContext* context)
</span><span class="lines">@@ -138,109 +142,158 @@
</span><span class="cx"> ASSERT(m_status == Ready);
</span><span class="cx"> m_status = Scheduled;
</span><span class="cx">
</span><del>- ASSERT(m_source);
- g_source_set_name(m_source.get(), name);
</del><ins>+ ASSERT(m_context.source);
+ g_source_set_name(m_context.source.get(), name);
</ins><span class="cx"> if (priority != G_PRIORITY_DEFAULT)
</span><del>- 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);
</del><ins>+ 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);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void GMainLoopSource::scheduleAfterDelay(const char* name, std::function<void ()> function, std::chrono::milliseconds delay, int priority, std::function<void ()> destroyFunction, GMainContext* context)
</span><span class="cx"> {
</span><del>- cancel();
- m_source = adoptGRef(g_timeout_source_new(delay.count()));
- m_voidCallback = WTF::move(function);
- m_destroyCallback = WTF::move(destroyFunction);
</del><ins>+ GMutexLocker locker(m_mutex);
+ cancelWithoutLocking();
+ m_context.source = adoptGRef(g_timeout_source_new(delay.count()));
+ m_context.voidCallback = WTF::move(function);
+ m_context.destroyCallback = WTF::move(destroyFunction);
</ins><span class="cx"> scheduleTimeoutSource(name, reinterpret_cast<GSourceFunc>(voidSourceCallback), priority, context);
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void GMainLoopSource::scheduleAfterDelay(const char* name, std::function<bool ()> function, std::chrono::milliseconds delay, int priority, std::function<void ()> destroyFunction, GMainContext* context)
</span><span class="cx"> {
</span><del>- cancel();
- m_source = adoptGRef(g_timeout_source_new(delay.count()));
- m_boolCallback = WTF::move(function);
- m_destroyCallback = WTF::move(destroyFunction);
</del><ins>+ GMutexLocker locker(m_mutex);
+ cancelWithoutLocking();
+ m_context.source = adoptGRef(g_timeout_source_new(delay.count()));
+ m_context.boolCallback = WTF::move(function);
+ m_context.destroyCallback = WTF::move(destroyFunction);
</ins><span class="cx"> scheduleTimeoutSource(name, reinterpret_cast<GSourceFunc>(boolSourceCallback), priority, context);
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void GMainLoopSource::scheduleAfterDelay(const char* name, std::function<void ()> function, std::chrono::seconds delay, int priority, std::function<void ()> destroyFunction, GMainContext* context)
</span><span class="cx"> {
</span><del>- cancel();
- m_source = adoptGRef(g_timeout_source_new_seconds(delay.count()));
- m_voidCallback = WTF::move(function);
- m_destroyCallback = WTF::move(destroyFunction);
</del><ins>+ GMutexLocker locker(m_mutex);
+ cancelWithoutLocking();
+ m_context.source = adoptGRef(g_timeout_source_new_seconds(delay.count()));
+ m_context.voidCallback = WTF::move(function);
+ m_context.destroyCallback = WTF::move(destroyFunction);
</ins><span class="cx"> scheduleTimeoutSource(name, reinterpret_cast<GSourceFunc>(voidSourceCallback), priority, context);
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void GMainLoopSource::scheduleAfterDelay(const char* name, std::function<bool ()> function, std::chrono::seconds delay, int priority, std::function<void ()> destroyFunction, GMainContext* context)
</span><span class="cx"> {
</span><del>- cancel();
- m_source = adoptGRef(g_timeout_source_new_seconds(delay.count()));
- m_boolCallback = WTF::move(function);
- m_destroyCallback = WTF::move(destroyFunction);
</del><ins>+ GMutexLocker locker(m_mutex);
+ cancelWithoutLocking();
+ m_context.source = adoptGRef(g_timeout_source_new_seconds(delay.count()));
+ m_context.boolCallback = WTF::move(function);
+ m_context.destroyCallback = WTF::move(destroyFunction);
</ins><span class="cx"> scheduleTimeoutSource(name, reinterpret_cast<GSourceFunc>(boolSourceCallback), priority, context);
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void GMainLoopSource::voidCallback()
</span><span class="cx"> {
</span><del>- if (!m_source)
- return;
</del><ins>+ Context context;
</ins><span class="cx">
</span><del>- ASSERT(m_voidCallback);
- ASSERT(m_status == Scheduled);
- m_status = Dispatched;
</del><ins>+ {
+ GMutexLocker locker(m_mutex);
+ if (!m_context.source)
+ return;
</ins><span class="cx">
</span><del>- GSource* source = m_source.get();
- m_voidCallback();
- if (source == m_source.get())
- destroy();
</del><ins>+ context = WTF::move(m_context);
+
+ ASSERT(context.voidCallback);
+ ASSERT(m_status == Scheduled);
+ m_status = Dispatched;
+ }
+
+ context.voidCallback();
+
+ bool shouldDestroy = false;
+ {
+ GMutexLocker locker(m_mutex);
+ shouldDestroy = !m_context.source;
+ }
+
+ if (shouldDestroy)
+ destroy(context.destroyCallback);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> bool GMainLoopSource::boolCallback()
</span><span class="cx"> {
</span><del>- if (!m_source)
- return false;
</del><ins>+ Context context;
</ins><span class="cx">
</span><del>- ASSERT(m_boolCallback);
- ASSERT(m_status == Scheduled || m_status == Dispatched);
- m_status = Dispatched;
</del><ins>+ {
+ GMutexLocker locker(m_mutex);
+ if (!m_context.source)
+ return Stop;
</ins><span class="cx">
</span><del>- GSource* source = m_source.get();
- bool retval = m_boolCallback();
- if (!retval && source == m_source.get())
- destroy();
</del><ins>+ context = WTF::move(m_context);
</ins><span class="cx">
</span><ins>+ ASSERT(context.boolCallback);
+ ASSERT(m_status == Scheduled || m_status == Dispatched);
+ m_status = Dispatched;
+ }
+
+ bool retval = context.boolCallback();
+
+ bool shouldDestroy = false;
+ {
+ GMutexLocker locker(m_mutex);
+ if (retval && !m_context.source)
+ m_context = WTF::move(context);
+ else
+ shouldDestroy = !m_context.source;
+ }
+
+ if (shouldDestroy)
+ destroy(context.destroyCallback);
</ins><span class="cx"> return retval;
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> bool GMainLoopSource::socketCallback(GIOCondition condition)
</span><span class="cx"> {
</span><del>- if (!m_source)
- return false;
</del><ins>+ Context context;
</ins><span class="cx">
</span><del>- ASSERT(m_socketCallback);
- ASSERT(m_status == Scheduled || m_status == Dispatched);
- m_status = Dispatched;
</del><ins>+ {
+ GMutexLocker locker(m_mutex);
+ if (!m_context.source)
+ return Stop;
</ins><span class="cx">
</span><del>- if (g_cancellable_is_cancelled(m_cancellable.get())) {
- destroy();
- return false;
</del><ins>+ context = WTF::move(m_context);
+
+ ASSERT(context.socketCallback);
+ ASSERT(m_status == Scheduled || m_status == Dispatched);
+ m_status = Dispatched;
</ins><span class="cx"> }
</span><span class="cx">
</span><del>- GSource* source = m_source.get();
- bool retval = m_socketCallback(condition);
- if (!retval && source == m_source.get())
- destroy();
</del><ins>+ if (g_cancellable_is_cancelled(context.cancellable.get())) {
+ destroy(context.destroyCallback);
+ return Stop;
+ }
</ins><span class="cx">
</span><ins>+ bool retval = context.socketCallback(condition);
+
+ bool shouldDestroy = false;
+ {
+ GMutexLocker locker(m_mutex);
+ if (retval && !m_context.source)
+ m_context = WTF::move(context);
+ else
+ shouldDestroy = !m_context.source;
+ }
+
+ if (shouldDestroy)
+ destroy(context.destroyCallback);
</ins><span class="cx"> return retval;
</span><span class="cx"> }
</span><span class="cx">
</span><del>-void GMainLoopSource::destroy()
</del><ins>+void GMainLoopSource::destroy(const std::function<void ()>& destroyCallback)
</ins><span class="cx"> {
</span><del>- auto destroyCallback = WTF::move(m_destroyCallback);
- auto deleteOnDestroy = m_deleteOnDestroy;
- reset();
</del><ins>+ // Nothing should be scheduled on this object at this point.
+ ASSERT(!m_context.source);
+ m_status = Ready;
+ DeleteOnDestroyType deleteOnDestroy = m_deleteOnDestroy;
+
</ins><span class="cx"> if (destroyCallback)
</span><span class="cx"> destroyCallback();
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceWTFwtfgobjectGMainLoopSourceh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WTF/wtf/gobject/GMainLoopSource.h (173200 => 173201)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WTF/wtf/gobject/GMainLoopSource.h 2014-09-03 06:04:44 UTC (rev 173200)
+++ trunk/Source/WTF/wtf/gobject/GMainLoopSource.h 2014-09-03 07:26:52 UTC (rev 173201)
</span><span class="lines">@@ -32,6 +32,7 @@
</span><span class="cx"> #include <wtf/gobject/GRefPtr.h>
</span><span class="cx">
</span><span class="cx"> typedef struct _GSocket GSocket;
</span><ins>+typedef union _GMutex GMutex;
</ins><span class="cx">
</span><span class="cx"> namespace WTF {
</span><span class="cx">
</span><span class="lines">@@ -65,13 +66,13 @@
</span><span class="cx">
</span><span class="cx"> enum Status { Ready, Scheduled, Dispatched };
</span><span class="cx">
</span><del>- void reset();
</del><ins>+ void cancelWithoutLocking();
</ins><span class="cx"> void scheduleIdleSource(const char* name, GSourceFunc, int priority, GMainContext*);
</span><span class="cx"> void scheduleTimeoutSource(const char* name, GSourceFunc, int priority, GMainContext*);
</span><span class="cx"> void voidCallback();
</span><span class="cx"> bool boolCallback();
</span><span class="cx"> bool socketCallback(GIOCondition);
</span><del>- void destroy();
</del><ins>+ void destroy(const std::function<void ()>&);
</ins><span class="cx">
</span><span class="cx"> static gboolean voidSourceCallback(GMainLoopSource*);
</span><span class="cx"> static gboolean boolSourceCallback(GMainLoopSource*);
</span><span class="lines">@@ -79,12 +80,20 @@
</span><span class="cx">
</span><span class="cx"> DeleteOnDestroyType m_deleteOnDestroy;
</span><span class="cx"> Status m_status;
</span><del>- GRefPtr<GSource> m_source;
- 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;
</del><ins>+ GMutex m_mutex;
+
+ struct Context {
+ Context() = default;
+ Context(Context&&) = default;
+ Context& operator=(Context&&) = default;
+
+ GRefPtr<GSource> source;
+ GRefPtr<GCancellable> cancellable;
+ std::function<void ()> voidCallback;
+ std::function<bool ()> boolCallback;
+ std::function<bool (GIOCondition)> socketCallback;
+ std::function<void ()> destroyCallback;
+ } m_context;
</ins><span class="cx"> };
</span><span class="cx">
</span><span class="cx"> } // namespace WTF
</span></span></pre></div>
<a id="trunkToolsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Tools/ChangeLog (173200 => 173201)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Tools/ChangeLog 2014-09-03 06:04:44 UTC (rev 173200)
+++ trunk/Tools/ChangeLog 2014-09-03 07:26:52 UTC (rev 173201)
</span><span class="lines">@@ -1,3 +1,32 @@
</span><ins>+2014-09-03 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 a unit test for GMainLoopSource that tests different
+ types of rescheduling tasks on already-active sources.
+
+ * TestWebKitAPI/PlatformGTK.cmake:
+ * TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp: Added.
+ (GMainLoopSourceTest::GMainLoopSourceTest):
+ (GMainLoopSourceTest::~GMainLoopSourceTest):
+ (GMainLoopSourceTest::runLoop):
+ (GMainLoopSourceTest::finish):
+ (GMainLoopSourceTest::source):
+ (testGMainLoopSourceBasicRescheduling):
+ (testGMainLoopSourceReentrantRescheduling):
+ (testGMainLoopSourceDifferentThreadRescheduling):
+ (beforeAll):
+ (afterAll):
+ (TestWebKitAPI::GMainLoopSourceTest::GMainLoopSourceTest):
+ (TestWebKitAPI::GMainLoopSourceTest::~GMainLoopSourceTest):
+ (TestWebKitAPI::GMainLoopSourceTest::runLoop):
+ (TestWebKitAPI::GMainLoopSourceTest::finish):
+ (TestWebKitAPI::GMainLoopSourceTest::source):
+ (TestWebKitAPI::TEST):
+
</ins><span class="cx"> 2014-09-02 Simon Fraser <simon.fraser@apple.com>
</span><span class="cx">
</span><span class="cx"> Make sure WK1 prefs are initialized in MiniBrowser
</span></span></pre></div>
<a id="trunkToolsTestWebKitAPIPlatformGTKcmake"></a>
<div class="modfile"><h4>Modified: trunk/Tools/TestWebKitAPI/PlatformGTK.cmake (173200 => 173201)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Tools/TestWebKitAPI/PlatformGTK.cmake 2014-09-03 06:04:44 UTC (rev 173200)
+++ trunk/Tools/TestWebKitAPI/PlatformGTK.cmake 2014-09-03 07:26:52 UTC (rev 173201)
</span><span class="lines">@@ -136,5 +136,6 @@
</span><span class="cx"> set_target_properties(TestWebCore PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${TESTWEBKITAPI_RUNTIME_OUTPUT_DIRECTORY}/WebCore)
</span><span class="cx">
</span><span class="cx"> list(APPEND TestWTF_SOURCES
</span><ins>+ ${TESTWEBKITAPI_DIR}/Tests/WTF/gobject/GMainLoopSource.cpp
</ins><span class="cx"> ${TESTWEBKITAPI_DIR}/Tests/WTF/gobject/GUniquePtr.cpp
</span><span class="cx"> )
</span></span></pre></div>
<a id="trunkToolsTestWebKitAPITestsWTFgobjectGMainLoopSourcecpp"></a>
<div class="addfile"><h4>Added: trunk/Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp (0 => 173201)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp (rev 0)
+++ trunk/Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp 2014-09-03 07:26:52 UTC (rev 173201)
</span><span class="lines">@@ -0,0 +1,137 @@
</span><ins>+/*
+ * 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 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;
+
+ 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;
+ });
+
+ context.test.source().schedule("[Test] SecondTask", [&] {
+ context.finishedSecondTask = true;
+ context.test.finish();
+ });
+
+ context.test.runLoop();
+ EXPECT_FALSE(context.finishedFirstTask);
+ EXPECT_TRUE(context.finishedSecondTask);
+}
+
+TEST(WTF_GMainLoopSource, ReentrantRescheduling)
+{
+ struct TestingContext {
+ GMainLoopSourceTest test;
+ bool finishedFirstTask = false;
+ bool finishedSecondTask = false;
+ } context;
+
+ context.test.source().schedule("[Test] FirstTask", [&] {
+ context.test.source().schedule("[Test] SecondTask", [&] {
+ ASSERT(context.finishedFirstTask);
+ context.finishedSecondTask = true;
+ context.test.finish();
+ });
+
+ context.finishedFirstTask = true;
+ });
+
+ context.test.runLoop();
+ EXPECT_TRUE(context.finishedFirstTask);
+ EXPECT_TRUE(context.finishedSecondTask);
+}
+
+TEST(WTF_GMainLoopSource, ReschedulingFromDifferentThread)
+{
+ struct TestingContext {
+ GMainLoopSourceTest test;
+ bool finishedFirstTask;
+ bool finishedSecondTask;
+ } context;
+
+ context.test.source().schedule("[Test] FirstTask", [&] {
+ g_usleep(1 * G_USEC_PER_SEC);
+ context.finishedFirstTask = true;
+ });
+
+ g_thread_new(nullptr, [](gpointer data) -> gpointer {
+ g_usleep(0.25 * G_USEC_PER_SEC);
+
+ TestingContext& context = *static_cast<TestingContext*>(data);
+ EXPECT_FALSE(context.finishedFirstTask);
+
+ context.test.source().schedule("[Test] SecondTask", [&] {
+ EXPECT_TRUE(context.finishedFirstTask);
+ context.finishedSecondTask = true;
+ context.test.finish();
+ });
+
+ g_thread_exit(nullptr);
+ return nullptr;
+ }, &context);
+
+ context.test.runLoop();
+ EXPECT_TRUE(context.finishedFirstTask);
+ EXPECT_TRUE(context.finishedSecondTask);
+}
+
+} // namespace TestWebKitAPI
</ins></span></pre>
</div>
</div>
</body>
</html>