Changeset 259879 in webkit


Ignore:
Timestamp:
Apr 10, 2020 9:46:49 AM (4 years ago)
Author:
aboya@igalia.com
Message:

[WTF] DataMutex: Add runUnlocked()
​https://bugs.webkit.org/show_bug.cgi?id=209811

Reviewed by Xabier Rodriguez-Calvar.

Source/WTF:

This patch introduces a runUnlocked() method in WTF::DataMutex::LockedWrapper
to run a lambda function without the lock. This is intended to be used for
small sections of the code that need to be unlocked, in cases where using
scoping would prove non-ergonomic or where running the unlocked section is only
necessary or desired when a certain condition is met -- something that cannot
be done with C++ scoping.

Safety mechanisms are provided. First, because this is used with a lambda, all
variables to be used in the unlocked section have to be specified in the
capture (global capture is possible but not recommended to simplify analysis).
Second, additional checks have been added to DataMutex to detect unlocked
accesses among other conditions. This will detect among other things naive
access to protected members by means of capturing the LockedWrapper by
reference.

  • wtf/DataMutex.h:

(WTF::OwnerAwareLockAdapter::lock):
(WTF::OwnerAwareLockAdapter::unlock):
(WTF::OwnerAwareLockAdapter::tryLock):
(WTF::OwnerAwareLockAdapter::isLocked const):
(WTF::DataMutex::LockedWrapper::operator->):
(WTF::DataMutex::LockedWrapper::operator*):
(WTF::DataMutex::LockedWrapper::mutex):
(WTF::DataMutex::LockedWrapper::lockHolder):
(WTF::DataMutex::LockedWrapper::runUnlocked):

Tools:

Tests for runUnlocked() and DataMutex checks are introduced.

  • TestWebKitAPI/Tests/WTF/DataMutex.cpp:

(TestWebKitAPI::TEST):

Location:
trunk
Files:
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WTF/ChangeLog

    r259878 r259879  
     12020-04-10  Alicia Boya García  <aboya@igalia.com>
     2
     3        [WTF] DataMutex: Add runUnlocked()
     4        https://bugs.webkit.org/show_bug.cgi?id=209811
     5
     6        Reviewed by Xabier Rodriguez-Calvar.
     7
     8        This patch introduces a runUnlocked() method in WTF::DataMutex::LockedWrapper
     9        to run a lambda function without the lock. This is intended to be used for
     10        small sections of the code that need to be unlocked, in cases where using
     11        scoping would prove non-ergonomic or where running the unlocked section is only
     12        necessary or desired when a certain condition is met -- something that cannot
     13        be done with C++ scoping.
     14
     15        Safety mechanisms are provided. First, because this is used with a lambda, all
     16        variables to be used in the unlocked section have to be specified in the
     17        capture (global capture is possible but not recommended to simplify analysis).
     18        Second, additional checks have been added to DataMutex to detect unlocked
     19        accesses among other conditions. This will detect among other things naive
     20        access to protected members by means of capturing the LockedWrapper by
     21        reference.
     22
     23        * wtf/DataMutex.h:
     24        (WTF::OwnerAwareLockAdapter::lock):
     25        (WTF::OwnerAwareLockAdapter::unlock):
     26        (WTF::OwnerAwareLockAdapter::tryLock):
     27        (WTF::OwnerAwareLockAdapter::isLocked const):
     28        (WTF::DataMutex::LockedWrapper::operator->):
     29        (WTF::DataMutex::LockedWrapper::operator*):
     30        (WTF::DataMutex::LockedWrapper::mutex):
     31        (WTF::DataMutex::LockedWrapper::lockHolder):
     32        (WTF::DataMutex::LockedWrapper::runUnlocked):
     33
    1342020-04-10  David Kilzer  <ddkilzer@apple.com>
    235

  • trunk/Source/WTF/wtf/DataMutex.h

    r248546 r259879  
    2222
    2323#include <wtf/Lock.h>
     24#include <wtf/Threading.h>
    2425
    2526namespace WTF {
    2627
    27 template<typename T>
     28// By default invalid access checks are only done in Debug builds.
     29#if !defined(ENABLE_DATA_MUTEX_CHECKS)
     30#if defined(NDEBUG)
     31#define ENABLE_DATA_MUTEX_CHECKS 0
     32#else
     33#define ENABLE_DATA_MUTEX_CHECKS 1
     34#endif
     35#endif
     36
     37#if ENABLE_DATA_MUTEX_CHECKS
     38#define DATA_MUTEX_CHECK(expr) RELEASE_ASSERT(expr)
     39#else
     40#define DATA_MUTEX_CHECK(expr)
     41#endif
     42
     43template<typename LockType>
     44class OwnerAwareLockAdapter {
     45public:
     46    void lock()
     47    {
     48        DATA_MUTEX_CHECK(m_owner != &Thread::current()); // Thread attempted recursive lock (unsupported).
     49        m_lock.lock();
     50#if ENABLE_DATA_MUTEX_CHECKS
     51        ASSERT(!m_owner);
     52        m_owner = &Thread::current();
     53#endif
     54    }
     55
     56    void unlock()
     57    {
     58#if ENABLE_DATA_MUTEX_CHECKS
     59        m_owner = nullptr;
     60#endif
     61        m_lock.unlock();
     62    }
     63
     64    bool tryLock()
     65    {
     66        DATA_MUTEX_CHECK(m_owner != &Thread::current()); // Thread attempted recursive lock (unsupported).
     67        if (!m_lock.tryLock())
     68            return false;
     69
     70#if ENABLE_DATA_MUTEX_CHECKS
     71        ASSERT(!m_owner);
     72        m_owner = &Thread::current();
     73#endif
     74        return true;
     75    }
     76
     77    bool isLocked() const
     78    {
     79        return m_lock.isLocked();
     80    }
     81
     82private:
     83#if ENABLE_DATA_MUTEX_CHECKS
     84    Thread* m_owner { nullptr }; // Use Thread* instead of RefPtr<Thread> since m_owner thread is always alive while m_owner is set.
     85#endif
     86    LockType m_lock;
     87};
     88
     89using OwnerAwareLock = OwnerAwareLockAdapter<Lock>;
     90
     91template<typename T, typename LockType = OwnerAwareLock>
    2892class DataMutex {
    2993    WTF_MAKE_FAST_ALLOCATED;
    … …  
    45109        T* operator->()
    46110        {
     111            DATA_MUTEX_CHECK(m_mutex.isLocked());
    47112            return &m_data;
    48113        }
    … …  
    50115        T& operator*()
    51116        {
     117            DATA_MUTEX_CHECK(m_mutex.isLocked());
    52118            return m_data;
    53119        }
    54120
    55         Lock& mutex()
     121        LockType& mutex()
    56122        {
    57123            return m_mutex;
    58124        }
    59125
    60         LockHolder& lockHolder()
     126        Locker<LockType>& lockHolder()
    61127        {
    62128            return m_lockHolder;
    63129        }
    64130
     131        // Used to avoid excessive brace scoping when only small parts of the code need to be run unlocked.
     132        // Please be mindful that accessing the wrapped data from the callback is unsafe and will fail on assertions.
     133        // It's helpful to use a minimal lambda capture to be conscious of what data you're having access to in these sections.
     134        void runUnlocked(WTF::Function<void()> callback)
     135        {
     136            m_mutex.unlock();
     137            callback();
     138            m_mutex.lock();
     139        }
     140
    65141    private:
    66         Lock& m_mutex;
    67         LockHolder m_lockHolder;
     142        LockType& m_mutex;
     143        Locker<LockType> m_lockHolder;
    68144        T& m_data;
    69145    };
    70146
    71147private:
    72     Lock m_mutex;
     148    LockType m_mutex;
    73149    T m_data;
    74150};

  • trunk/Tools/ChangeLog

    r259873 r259879  
     12020-04-10  Alicia Boya García  <aboya@igalia.com>
     2
     3        [WTF] DataMutex: Add runUnlocked()
     4        https://bugs.webkit.org/show_bug.cgi?id=209811
     5
     6        Reviewed by Xabier Rodriguez-Calvar.
     7
     8        Tests for runUnlocked() and DataMutex checks are introduced.
     9
     10        * TestWebKitAPI/Tests/WTF/DataMutex.cpp:
     11        (TestWebKitAPI::TEST):
     12
    113== Rolled over to ChangeLog-2020-04-10 ==

  • trunk/Tools/TestWebKitAPI/Tests/WTF/DataMutex.cpp

    r247723 r259879  
    2626
    2727#include "config.h"
     28
     29// For this file, we force checks even if we're running on release.
     30#define ENABLE_DATA_MUTEX_CHECKS 1
    2831#include <wtf/DataMutex.h>
    2932
    … …  
    3538    int number;
    3639};
    37 DataMutex<MyStructure> myDataMutex;
    3840
    3941TEST(WTF_DataMutex, TakingTheMutex)
    4042{
    41     Lock* mutex;
     43    DataMutex<MyStructure> myDataMutex;
     44
     45    OwnerAwareLock* mutex;
    4246    {
    4347        DataMutex<MyStructure>::LockedWrapper wrapper(myDataMutex);
    4448        mutex = &wrapper.mutex();
    45         ASSERT_TRUE(mutex->isHeld());
     49        ASSERT_TRUE(mutex->isLocked());
    4650        wrapper->number = 5;
     51
     52        wrapper.runUnlocked([mutex]() {
     53            ASSERT_FALSE(mutex->isLocked());
     54        });
     55        ASSERT_TRUE(mutex->isLocked());
    4756    }
    48     ASSERT_FALSE(mutex->isHeld());
     57    ASSERT_FALSE(mutex->isLocked());
    4958
    5059    {
    … …  
    5463}
    5564
     65TEST(WTF_DataMutex, RunUnlockedIllegalAccessDeathTest)
     66{
     67    ::testing::FLAGS_gtest_death_test_style = "threadsafe";
     68    DataMutex<MyStructure> myDataMutex;
     69    DataMutex<MyStructure>::LockedWrapper wrapper(myDataMutex);
     70    wrapper->number = 5;
     71
     72    ASSERT_DEATH(wrapper.runUnlocked([&]() {
     73        wrapper->number++;
     74    }), "");
     75}
     76
     77TEST(WTF_DataMutex, DoubleLockDeathTest)
     78{
     79    ::testing::FLAGS_gtest_death_test_style = "threadsafe";
     80    DataMutex<MyStructure> myDataMutex;
     81    DataMutex<MyStructure>::LockedWrapper wrapper1(myDataMutex);
     82    ASSERT_DEATH(DataMutex<MyStructure>::LockedWrapper wrapper2(myDataMutex), "");
     83}
     84
    5685} // namespace TestWebKitAPI
Note: See TracChangeset for help on using the changeset viewer.