Changeset 173201 in webkit


Ignore:
Timestamp:
Sep 3, 2014 12:26:52 AM (10 years ago)
Author:
zandobersek@gmail.com
Message:

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):

Location:
trunk
Files:
1 added
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WTF/ChangeLog

    r173192 r173201  
     12014-09-03  Zan Dobersek  <zdobersek@igalia.com>
     2
     3        GMainLoopSource is exposed to race conditions
     4        https://bugs.webkit.org/show_bug.cgi?id=135800
     5
     6        Reviewed by Carlos Garcia Campos.
     7
     8        GMainLoopSource objects can be dispatching tasks on one thread
     9        while having a new task scheduled on a different thread. This
     10        can for instance occur in WebKitVideoSink, where the timeout
     11        callback can be called on main thread while at the same time
     12        it is being rescheduled on a different thread (created through
     13        GStreamer).
     14
     15        The initial solution is to use GMutex to prevent parallel data
     16        access from different threads. In the future I plan to add better
     17        assertions, some meaningful comments and look at the possibility
     18        of creating thread-specific GMainLoopSource objects that wouldn't
     19        require the use of GMutex.
     20
     21        GSource, GCancellable and std::function<> objects are now packed
     22        into an internal Context structure. Using the C++11 move semantics
     23        it's simple to, at the time of dispatch, move the current context
     24        out of the GMainLoopSource object in case the dispatch causes a
     25        rescheduling on that same object.
     26
     27        All the schedule*() methods and the cancelInternal() method callers
     28        now lock the GMutex to ensure no one else is accessing the data at
     29        that moment. Similar goes for the dispatch methods, but those do
     30        the dispatch and possible destruction duties with the mutex unlocked.
     31        The dispatch can cause rescheduling on the same GMainLoopSource object,
     32        which must not be done with a locked mutex.
     33
     34        * wtf/gobject/GMainLoopSource.cpp:
     35        (WTF::GMainLoopSource::GMainLoopSource):
     36        (WTF::GMainLoopSource::~GMainLoopSource):
     37        (WTF::GMainLoopSource::cancel):
     38        (WTF::GMainLoopSource::cancelInternal):
     39        (WTF::GMainLoopSource::scheduleIdleSource):
     40        (WTF::GMainLoopSource::schedule):
     41        (WTF::GMainLoopSource::scheduleTimeoutSource):
     42        (WTF::GMainLoopSource::scheduleAfterDelay):
     43        (WTF::GMainLoopSource::voidCallback):
     44        (WTF::GMainLoopSource::boolCallback):
     45        (WTF::GMainLoopSource::socketCallback):
     46        (WTF::GMainLoopSource::destroy):
     47        (WTF::GMainLoopSource::reset): Deleted.
     48        * wtf/gobject/GMainLoopSource.h:
     49
    1502014-09-02  Daniel Bates  <dabates@apple.com>
    251

  • trunk/Source/WTF/wtf/gobject/GMainLoopSource.cpp

    r170774 r173201  
    2929
    3030#include "GMainLoopSource.h"
    31 
    3231#include <gio/gio.h>
     32#include <wtf/gobject/GMutexLocker.h>
    3333
    3434namespace WTF {
    … …  
    4343    , m_status(Ready)
    4444{
     45    g_mutex_init(&m_mutex);
    4546}
    4647
    … …  
    4950    , m_status(Ready)
    5051{
     52    g_mutex_init(&m_mutex);
    5153}
    5254
    … …  
    5456{
    5557    cancel();
     58    g_mutex_clear(&m_mutex);
    5659}
    5760
    … …  
    6871void GMainLoopSource::cancel()
    6972{
    70     if (!m_source)
     73    GMutexLocker locker(m_mutex);
     74    cancelWithoutLocking();
     75}
     76
     77void GMainLoopSource::cancelWithoutLocking()
     78{
     79    if (!m_context.source) {
     80        m_status = Ready;
    7181        return;
    72 
    73     GRefPtr<GSource> source;
    74     m_source.swap(source);
    75 
    76     if (m_cancellable)
    77         g_cancellable_cancel(m_cancellable.get());
    78     g_source_destroy(source.get());
    79     destroy();
    80 }
    81 
    82 void GMainLoopSource::reset()
    83 {
    84     m_status = Ready;
    85     m_source = nullptr;
    86     m_cancellable = nullptr;
    87     m_voidCallback = nullptr;
    88     m_boolCallback = nullptr;
    89     m_destroyCallback = nullptr;
     82    }
     83
     84    Context context = WTF::move(m_context);
     85
     86    if (context.cancellable)
     87        g_cancellable_cancel(context.cancellable.get());
     88
     89    g_source_destroy(context.source.get());
     90    destroy(context.destroyCallback);
    9091}
    9192
    … …  
    9596    m_status = Scheduled;
    9697
    97     m_source = adoptGRef(g_idle_source_new());
    98     g_source_set_name(m_source.get(), name);
     98    m_context.source = adoptGRef(g_idle_source_new());
     99    g_source_set_name(m_context.source.get(), name);
    99100    if (priority != G_PRIORITY_DEFAULT_IDLE)
    100         g_source_set_priority(m_source.get(), priority);
    101     g_source_set_callback(m_source.get(), sourceFunction, this, nullptr);
    102     g_source_attach(m_source.get(), context);
     101        g_source_set_priority(m_context.source.get(), priority);
     102    g_source_set_callback(m_context.source.get(), sourceFunction, this, nullptr);
     103    g_source_attach(m_context.source.get(), context);
    103104}
    104105
    105106void GMainLoopSource::schedule(const char* name, std::function<void ()> function, int priority, std::function<void ()> destroyFunction, GMainContext* context)
    106107{
    107     cancel();
    108     m_voidCallback = WTF::move(function);
    109     m_destroyCallback = WTF::move(destroyFunction);
     108    GMutexLocker locker(m_mutex);
     109    cancelWithoutLocking();
     110    m_context.voidCallback = WTF::move(function);
     111    m_context.destroyCallback = WTF::move(destroyFunction);
    110112    scheduleIdleSource(name, reinterpret_cast<GSourceFunc>(voidSourceCallback), priority, context);
    111113}
    … …  
    113115void GMainLoopSource::schedule(const char* name, std::function<bool ()> function, int priority, std::function<void ()> destroyFunction, GMainContext* context)
    114116{
    115     cancel();
    116     m_boolCallback = WTF::move(function);
    117     m_destroyCallback = WTF::move(destroyFunction);
     117    GMutexLocker locker(m_mutex);
     118    cancelWithoutLocking();
     119    m_context.boolCallback = WTF::move(function);
     120    m_context.destroyCallback = WTF::move(destroyFunction);
    118121    scheduleIdleSource(name, reinterpret_cast<GSourceFunc>(boolSourceCallback), priority, context);
    119122}
    … …  
    121124void GMainLoopSource::schedule(const char* name, std::function<bool (GIOCondition)> function, GSocket* socket, GIOCondition condition, std::function<void ()> destroyFunction, GMainContext* context)
    122125{
    123     cancel();
     126    GMutexLocker locker(m_mutex);
     127    cancelWithoutLocking();
    124128    ASSERT(m_status == Ready);
    125129    m_status = Scheduled;
    126130
    127     m_socketCallback = WTF::move(function);
    128     m_destroyCallback = WTF::move(destroyFunction);
    129     m_cancellable = adoptGRef(g_cancellable_new());
    130     m_source = adoptGRef(g_socket_create_source(socket, condition, m_cancellable.get()));
    131     g_source_set_name(m_source.get(), name);
    132     g_source_set_callback(m_source.get(), reinterpret_cast<GSourceFunc>(socketSourceCallback), this, nullptr);
    133     g_source_attach(m_source.get(), context);
     131    m_context.socketCallback = WTF::move(function);
     132    m_context.destroyCallback = WTF::move(destroyFunction);
     133    m_context.cancellable = adoptGRef(g_cancellable_new());
     134    m_context.source = adoptGRef(g_socket_create_source(socket, condition, m_context.cancellable.get()));
     135    g_source_set_name(m_context.source.get(), name);
     136    g_source_set_callback(m_context.source.get(), reinterpret_cast<GSourceFunc>(socketSourceCallback), this, nullptr);
     137    g_source_attach(m_context.source.get(), context);
    134138}
    135139
    … …  
    139143    m_status = Scheduled;
    140144
    141     ASSERT(m_source);
    142     g_source_set_name(m_source.get(), name);
     145    ASSERT(m_context.source);
     146    g_source_set_name(m_context.source.get(), name);
    143147    if (priority != G_PRIORITY_DEFAULT)
    144         g_source_set_priority(m_source.get(), priority);
    145     g_source_set_callback(m_source.get(), sourceFunction, this, nullptr);
    146     g_source_attach(m_source.get(), context);
     148        g_source_set_priority(m_context.source.get(), priority);
     149    g_source_set_callback(m_context.source.get(), sourceFunction, this, nullptr);
     150    g_source_attach(m_context.source.get(), context);
    147151}
    148152
    149153void GMainLoopSource::scheduleAfterDelay(const char* name, std::function<void ()> function, std::chrono::milliseconds delay, int priority, std::function<void ()> destroyFunction, GMainContext* context)
    150154{
    151     cancel();
    152     m_source = adoptGRef(g_timeout_source_new(delay.count()));
    153     m_voidCallback = WTF::move(function);
    154     m_destroyCallback = WTF::move(destroyFunction);
     155    GMutexLocker locker(m_mutex);
     156    cancelWithoutLocking();
     157    m_context.source = adoptGRef(g_timeout_source_new(delay.count()));
     158    m_context.voidCallback = WTF::move(function);
     159    m_context.destroyCallback = WTF::move(destroyFunction);
    155160    scheduleTimeoutSource(name, reinterpret_cast<GSourceFunc>(voidSourceCallback), priority, context);
    156161}
    … …  
    158163void GMainLoopSource::scheduleAfterDelay(const char* name, std::function<bool ()> function, std::chrono::milliseconds delay, int priority, std::function<void ()> destroyFunction, GMainContext* context)
    159164{
    160     cancel();
    161     m_source = adoptGRef(g_timeout_source_new(delay.count()));
    162     m_boolCallback = WTF::move(function);
    163     m_destroyCallback = WTF::move(destroyFunction);
     165    GMutexLocker locker(m_mutex);
     166    cancelWithoutLocking();
     167    m_context.source = adoptGRef(g_timeout_source_new(delay.count()));
     168    m_context.boolCallback = WTF::move(function);
     169    m_context.destroyCallback = WTF::move(destroyFunction);
    164170    scheduleTimeoutSource(name, reinterpret_cast<GSourceFunc>(boolSourceCallback), priority, context);
    165171}
    … …  
    167173void GMainLoopSource::scheduleAfterDelay(const char* name, std::function<void ()> function, std::chrono::seconds delay, int priority, std::function<void ()> destroyFunction, GMainContext* context)
    168174{
    169     cancel();
    170     m_source = adoptGRef(g_timeout_source_new_seconds(delay.count()));
    171     m_voidCallback = WTF::move(function);
    172     m_destroyCallback = WTF::move(destroyFunction);
     175    GMutexLocker locker(m_mutex);
     176    cancelWithoutLocking();
     177    m_context.source = adoptGRef(g_timeout_source_new_seconds(delay.count()));
     178    m_context.voidCallback = WTF::move(function);
     179    m_context.destroyCallback = WTF::move(destroyFunction);
    173180    scheduleTimeoutSource(name, reinterpret_cast<GSourceFunc>(voidSourceCallback), priority, context);
    174181}
    … …  
    176183void GMainLoopSource::scheduleAfterDelay(const char* name, std::function<bool ()> function, std::chrono::seconds delay, int priority, std::function<void ()> destroyFunction, GMainContext* context)
    177184{
    178     cancel();
    179     m_source = adoptGRef(g_timeout_source_new_seconds(delay.count()));
    180     m_boolCallback = WTF::move(function);
    181     m_destroyCallback = WTF::move(destroyFunction);
     185    GMutexLocker locker(m_mutex);
     186    cancelWithoutLocking();
     187    m_context.source = adoptGRef(g_timeout_source_new_seconds(delay.count()));
     188    m_context.boolCallback = WTF::move(function);
     189    m_context.destroyCallback = WTF::move(destroyFunction);
    182190    scheduleTimeoutSource(name, reinterpret_cast<GSourceFunc>(boolSourceCallback), priority, context);
    183191}
    … …  
    185193void GMainLoopSource::voidCallback()
    186194{
    187     if (!m_source)
    188         return;
    189 
    190     ASSERT(m_voidCallback);
    191     ASSERT(m_status == Scheduled);
    192     m_status = Dispatched;
    193 
    194     GSource* source = m_source.get();
    195     m_voidCallback();
    196     if (source == m_source.get())
    197         destroy();
     195    Context context;
     196
     197    {
     198        GMutexLocker locker(m_mutex);
     199        if (!m_context.source)
     200            return;
     201
     202        context = WTF::move(m_context);
     203
     204        ASSERT(context.voidCallback);
     205        ASSERT(m_status == Scheduled);
     206        m_status = Dispatched;
     207    }
     208
     209    context.voidCallback();
     210
     211    bool shouldDestroy = false;
     212    {
     213        GMutexLocker locker(m_mutex);
     214        shouldDestroy = !m_context.source;
     215    }
     216
     217    if (shouldDestroy)
     218        destroy(context.destroyCallback);
    198219}
    199220
    200221bool GMainLoopSource::boolCallback()
    201222{
    202     if (!m_source)
    203         return false;
    204 
    205     ASSERT(m_boolCallback);
    206     ASSERT(m_status == Scheduled || m_status == Dispatched);
    207     m_status = Dispatched;
    208 
    209     GSource* source = m_source.get();
    210     bool retval = m_boolCallback();
    211     if (!retval && source == m_source.get())
    212         destroy();
    213 
     223    Context context;
     224
     225    {
     226        GMutexLocker locker(m_mutex);
     227        if (!m_context.source)
     228            return Stop;
     229
     230        context = WTF::move(m_context);
     231
     232        ASSERT(context.boolCallback);
     233        ASSERT(m_status == Scheduled || m_status == Dispatched);
     234        m_status = Dispatched;
     235    }
     236
     237    bool retval = context.boolCallback();
     238
     239    bool shouldDestroy = false;
     240    {
     241        GMutexLocker locker(m_mutex);
     242        if (retval && !m_context.source)
     243            m_context = WTF::move(context);
     244        else
     245            shouldDestroy = !m_context.source;
     246    }
     247
     248    if (shouldDestroy)
     249        destroy(context.destroyCallback);
    214250    return retval;
    215251}
    … …  
    217253bool GMainLoopSource::socketCallback(GIOCondition condition)
    218254{
    219     if (!m_source)
    220         return false;
    221 
    222     ASSERT(m_socketCallback);
    223     ASSERT(m_status == Scheduled || m_status == Dispatched);
    224     m_status = Dispatched;
    225 
    226     if (g_cancellable_is_cancelled(m_cancellable.get())) {
    227         destroy();
    228         return false;
    229     }
    230 
    231     GSource* source = m_source.get();
    232     bool retval = m_socketCallback(condition);
    233     if (!retval && source == m_source.get())
    234         destroy();
    235 
     255    Context context;
     256
     257    {
     258        GMutexLocker locker(m_mutex);
     259        if (!m_context.source)
     260            return Stop;
     261
     262        context = WTF::move(m_context);
     263
     264        ASSERT(context.socketCallback);
     265        ASSERT(m_status == Scheduled || m_status == Dispatched);
     266        m_status = Dispatched;
     267    }
     268
     269    if (g_cancellable_is_cancelled(context.cancellable.get())) {
     270        destroy(context.destroyCallback);
     271        return Stop;
     272    }
     273
     274    bool retval = context.socketCallback(condition);
     275
     276    bool shouldDestroy = false;
     277    {
     278        GMutexLocker locker(m_mutex);
     279        if (retval && !m_context.source)
     280            m_context = WTF::move(context);
     281        else
     282            shouldDestroy = !m_context.source;
     283    }
     284
     285    if (shouldDestroy)
     286        destroy(context.destroyCallback);
    236287    return retval;
    237288}
    238289
    239 void GMainLoopSource::destroy()
    240 {
    241     auto destroyCallback = WTF::move(m_destroyCallback);
    242     auto deleteOnDestroy = m_deleteOnDestroy;
    243     reset();
     290void GMainLoopSource::destroy(const std::function<void ()>& destroyCallback)
     291{
     292    // Nothing should be scheduled on this object at this point.
     293    ASSERT(!m_context.source);
     294    m_status = Ready;
     295    DeleteOnDestroyType deleteOnDestroy = m_deleteOnDestroy;
     296
    244297    if (destroyCallback)
    245298        destroyCallback();

  • trunk/Source/WTF/wtf/gobject/GMainLoopSource.h

    r166636 r173201  
    3333
    3434typedef struct _GSocket GSocket;
     35typedef union _GMutex GMutex;
    3536
    3637namespace WTF {
    … …  
    6667    enum Status { Ready, Scheduled, Dispatched };
    6768
    68     void reset();
     69    void cancelWithoutLocking();
    6970    void scheduleIdleSource(const char* name, GSourceFunc, int priority, GMainContext*);
    7071    void scheduleTimeoutSource(const char* name, GSourceFunc, int priority, GMainContext*);
    … …  
    7273    bool boolCallback();
    7374    bool socketCallback(GIOCondition);
    74     void destroy();
     75    void destroy(const std::function<void ()>&);
    7576
    7677    static gboolean voidSourceCallback(GMainLoopSource*);
    … …  
    8081    DeleteOnDestroyType m_deleteOnDestroy;
    8182    Status m_status;
    82     GRefPtr<GSource> m_source;
    83     GRefPtr<GCancellable> m_cancellable;
    84     std::function<void ()> m_voidCallback;
    85     std::function<bool ()> m_boolCallback;
    86     std::function<bool (GIOCondition)> m_socketCallback;
    87     std::function<void ()> m_destroyCallback;
     83    GMutex m_mutex;
     84
     85    struct Context {
     86        Context() = default;
     87        Context(Context&&) = default;
     88        Context& operator=(Context&&) = default;
     89
     90        GRefPtr<GSource> source;
     91        GRefPtr<GCancellable> cancellable;
     92        std::function<void ()> voidCallback;
     93        std::function<bool ()> boolCallback;
     94        std::function<bool (GIOCondition)> socketCallback;
     95        std::function<void ()> destroyCallback;
     96    } m_context;
    8897};
    8998

  • trunk/Tools/ChangeLog

    r173187 r173201  
     12014-09-03  Zan Dobersek  <zdobersek@igalia.com>
     2
     3        GMainLoopSource is exposed to race conditions
     4        https://bugs.webkit.org/show_bug.cgi?id=135800
     5
     6        Reviewed by Carlos Garcia Campos.
     7
     8        Add a unit test for GMainLoopSource that tests different
     9        types of rescheduling tasks on already-active sources.
     10
     11        * TestWebKitAPI/PlatformGTK.cmake:
     12        * TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp: Added.
     13        (GMainLoopSourceTest::GMainLoopSourceTest):
     14        (GMainLoopSourceTest::~GMainLoopSourceTest):
     15        (GMainLoopSourceTest::runLoop):
     16        (GMainLoopSourceTest::finish):
     17        (GMainLoopSourceTest::source):
     18        (testGMainLoopSourceBasicRescheduling):
     19        (testGMainLoopSourceReentrantRescheduling):
     20        (testGMainLoopSourceDifferentThreadRescheduling):
     21        (beforeAll):
     22        (afterAll):
     23        (TestWebKitAPI::GMainLoopSourceTest::GMainLoopSourceTest):
     24        (TestWebKitAPI::GMainLoopSourceTest::~GMainLoopSourceTest):
     25        (TestWebKitAPI::GMainLoopSourceTest::runLoop):
     26        (TestWebKitAPI::GMainLoopSourceTest::finish):
     27        (TestWebKitAPI::GMainLoopSourceTest::source):
     28        (TestWebKitAPI::TEST):
     29
    1302014-09-02  Simon Fraser  <simon.fraser@apple.com>
    231

  • trunk/Tools/TestWebKitAPI/PlatformGTK.cmake

    r172586 r173201  
    137137
    138138list(APPEND TestWTF_SOURCES
     139    ${TESTWEBKITAPI_DIR}/Tests/WTF/gobject/GMainLoopSource.cpp
    139140    ${TESTWEBKITAPI_DIR}/Tests/WTF/gobject/GUniquePtr.cpp
    140141)
Note: See TracChangeset for help on using the changeset viewer.