Title: [259879] trunk
- Revision
- 259879
- Author
- ab...@igalia.com
- Date
- 2020-04-10 09:46:49 -0700 (Fri, 10 Apr 2020)
Log 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):
Modified Paths
Diff
Modified: trunk/Source/WTF/ChangeLog (259878 => 259879)
--- trunk/Source/WTF/ChangeLog 2020-04-10 16:22:34 UTC (rev 259878)
+++ trunk/Source/WTF/ChangeLog 2020-04-10 16:46:49 UTC (rev 259879)
@@ -1,3 +1,36 @@
+2020-04-10 Alicia Boya García <ab...@igalia.com>
+
+ [WTF] DataMutex: Add runUnlocked()
+ https://bugs.webkit.org/show_bug.cgi?id=209811
+
+ Reviewed by Xabier Rodriguez-Calvar.
+
+ 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):
+
2020-04-10 David Kilzer <ddkil...@apple.com>
Add WARN_UNUSED_RETURN to decode methods in Source/WTF
Modified: trunk/Source/WTF/wtf/DataMutex.h (259878 => 259879)
--- trunk/Source/WTF/wtf/DataMutex.h 2020-04-10 16:22:34 UTC (rev 259878)
+++ trunk/Source/WTF/wtf/DataMutex.h 2020-04-10 16:46:49 UTC (rev 259879)
@@ -21,10 +21,74 @@
#pragma once
#include <wtf/Lock.h>
+#include <wtf/Threading.h>
namespace WTF {
-template<typename T>
+// By default invalid access checks are only done in Debug builds.
+#if !defined(ENABLE_DATA_MUTEX_CHECKS)
+#if defined(NDEBUG)
+#define ENABLE_DATA_MUTEX_CHECKS 0
+#else
+#define ENABLE_DATA_MUTEX_CHECKS 1
+#endif
+#endif
+
+#if ENABLE_DATA_MUTEX_CHECKS
+#define DATA_MUTEX_CHECK(expr) RELEASE_ASSERT(expr)
+#else
+#define DATA_MUTEX_CHECK(expr)
+#endif
+
+template<typename LockType>
+class OwnerAwareLockAdapter {
+public:
+ void lock()
+ {
+ DATA_MUTEX_CHECK(m_owner != &Thread::current()); // Thread attempted recursive lock (unsupported).
+ m_lock.lock();
+#if ENABLE_DATA_MUTEX_CHECKS
+ ASSERT(!m_owner);
+ m_owner = &Thread::current();
+#endif
+ }
+
+ void unlock()
+ {
+#if ENABLE_DATA_MUTEX_CHECKS
+ m_owner = nullptr;
+#endif
+ m_lock.unlock();
+ }
+
+ bool tryLock()
+ {
+ DATA_MUTEX_CHECK(m_owner != &Thread::current()); // Thread attempted recursive lock (unsupported).
+ if (!m_lock.tryLock())
+ return false;
+
+#if ENABLE_DATA_MUTEX_CHECKS
+ ASSERT(!m_owner);
+ m_owner = &Thread::current();
+#endif
+ return true;
+ }
+
+ bool isLocked() const
+ {
+ return m_lock.isLocked();
+ }
+
+private:
+#if ENABLE_DATA_MUTEX_CHECKS
+ Thread* m_owner { nullptr }; // Use Thread* instead of RefPtr<Thread> since m_owner thread is always alive while m_owner is set.
+#endif
+ LockType m_lock;
+};
+
+using OwnerAwareLock = OwnerAwareLockAdapter<Lock>;
+
+template<typename T, typename LockType = OwnerAwareLock>
class DataMutex {
WTF_MAKE_FAST_ALLOCATED;
WTF_MAKE_NONCOPYABLE(DataMutex);
@@ -44,32 +108,44 @@
T* operator->()
{
+ DATA_MUTEX_CHECK(m_mutex.isLocked());
return &m_data;
}
T& operator*()
{
+ DATA_MUTEX_CHECK(m_mutex.isLocked());
return m_data;
}
- Lock& mutex()
+ LockType& mutex()
{
return m_mutex;
}
- LockHolder& lockHolder()
+ Locker<LockType>& lockHolder()
{
return m_lockHolder;
}
+ // Used to avoid excessive brace scoping when only small parts of the code need to be run unlocked.
+ // Please be mindful that accessing the wrapped data from the callback is unsafe and will fail on assertions.
+ // It's helpful to use a minimal lambda capture to be conscious of what data you're having access to in these sections.
+ void runUnlocked(WTF::Function<void()> callback)
+ {
+ m_mutex.unlock();
+ callback();
+ m_mutex.lock();
+ }
+
private:
- Lock& m_mutex;
- LockHolder m_lockHolder;
+ LockType& m_mutex;
+ Locker<LockType> m_lockHolder;
T& m_data;
};
private:
- Lock m_mutex;
+ LockType m_mutex;
T m_data;
};
Modified: trunk/Tools/ChangeLog (259878 => 259879)
--- trunk/Tools/ChangeLog 2020-04-10 16:22:34 UTC (rev 259878)
+++ trunk/Tools/ChangeLog 2020-04-10 16:46:49 UTC (rev 259879)
@@ -1 +1,13 @@
+2020-04-10 Alicia Boya García <ab...@igalia.com>
+
+ [WTF] DataMutex: Add runUnlocked()
+ https://bugs.webkit.org/show_bug.cgi?id=209811
+
+ Reviewed by Xabier Rodriguez-Calvar.
+
+ Tests for runUnlocked() and DataMutex checks are introduced.
+
+ * TestWebKitAPI/Tests/WTF/DataMutex.cpp:
+ (TestWebKitAPI::TEST):
+
== Rolled over to ChangeLog-2020-04-10 ==
Modified: trunk/Tools/TestWebKitAPI/Tests/WTF/DataMutex.cpp (259878 => 259879)
--- trunk/Tools/TestWebKitAPI/Tests/WTF/DataMutex.cpp 2020-04-10 16:22:34 UTC (rev 259878)
+++ trunk/Tools/TestWebKitAPI/Tests/WTF/DataMutex.cpp 2020-04-10 16:46:49 UTC (rev 259879)
@@ -25,6 +25,9 @@
*/
#include "config.h"
+
+// For this file, we force checks even if we're running on release.
+#define ENABLE_DATA_MUTEX_CHECKS 1
#include <wtf/DataMutex.h>
using namespace WTF;
@@ -34,18 +37,24 @@
struct MyStructure {
int number;
};
-DataMutex<MyStructure> myDataMutex;
TEST(WTF_DataMutex, TakingTheMutex)
{
- Lock* mutex;
+ DataMutex<MyStructure> myDataMutex;
+
+ OwnerAwareLock* mutex;
{
DataMutex<MyStructure>::LockedWrapper wrapper(myDataMutex);
mutex = &wrapper.mutex();
- ASSERT_TRUE(mutex->isHeld());
+ ASSERT_TRUE(mutex->isLocked());
wrapper->number = 5;
+
+ wrapper.runUnlocked([mutex]() {
+ ASSERT_FALSE(mutex->isLocked());
+ });
+ ASSERT_TRUE(mutex->isLocked());
}
- ASSERT_FALSE(mutex->isHeld());
+ ASSERT_FALSE(mutex->isLocked());
{
DataMutex<MyStructure>::LockedWrapper wrapper(myDataMutex);
@@ -53,4 +62,24 @@
}
}
+TEST(WTF_DataMutex, RunUnlockedIllegalAccessDeathTest)
+{
+ ::testing::FLAGS_gtest_death_test_style = "threadsafe";
+ DataMutex<MyStructure> myDataMutex;
+ DataMutex<MyStructure>::LockedWrapper wrapper(myDataMutex);
+ wrapper->number = 5;
+
+ ASSERT_DEATH(wrapper.runUnlocked([&]() {
+ wrapper->number++;
+ }), "");
+}
+
+TEST(WTF_DataMutex, DoubleLockDeathTest)
+{
+ ::testing::FLAGS_gtest_death_test_style = "threadsafe";
+ DataMutex<MyStructure> myDataMutex;
+ DataMutex<MyStructure>::LockedWrapper wrapper1(myDataMutex);
+ ASSERT_DEATH(DataMutex<MyStructure>::LockedWrapper wrapper2(myDataMutex), "");
+}
+
} // namespace TestWebKitAPI
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
