Diff
Modified: trunk/Source/WebCore/ChangeLog (277892 => 277893)
--- trunk/Source/WebCore/ChangeLog 2021-05-21 22:14:58 UTC (rev 277892)
+++ trunk/Source/WebCore/ChangeLog 2021-05-21 22:17:33 UTC (rev 277893)
@@ -1,5 +1,46 @@
2021-05-21 Chris Dumez <cdu...@apple.com>
+ Adopt CheckedLock in ScrollingTree and fix threading bug
+ https://bugs.webkit.org/show_bug.cgi?id=226109
+
+ Reviewed by Darin Adler.
+
+ Adopt CheckedLock in ScrollingTree and fix threading bug found by Clang Thread Safety
+ Analysis. In particular, ThreadedScrollingTree::willStartRenderingUpdate() was failing
+ to grab the lock before updating m_state. m_state definitely get modified from several
+ threads so synchronization is important.
+
+ * page/scrolling/ScrollingTree.cpp:
+ (WebCore::ScrollingTree::handleWheelEvent):
+ (WebCore::ScrollingTree::traverseScrollingTree):
+ (WebCore::ScrollingTree::commitTreeState):
+ (WebCore::ScrollingTree::applyLayerPositions):
+ (WebCore::ScrollingTree::applyLayerPositionsInternal):
+ (WebCore::ScrollingTree::scrollBySimulatingWheelEventForTesting):
+ * page/scrolling/ScrollingTree.h:
+ (WebCore::ScrollingTree::WTF_RETURNS_LOCK):
+ (WebCore::ScrollingTree::WTF_REQUIRES_LOCK):
+ * page/scrolling/ThreadedScrollingTree.cpp:
+ (WebCore::ThreadedScrollingTree::ThreadedScrollingTree):
+ (WebCore::ThreadedScrollingTree::handleWheelEventAfterMainThread):
+ (WebCore::ThreadedScrollingTree::wheelEventWasProcessedByMainThread):
+ (WebCore::ThreadedScrollingTree::willSendEventToMainThread):
+ (WebCore::ThreadedScrollingTree::waitForEventToBeProcessedByMainThread):
+ (WebCore::ThreadedScrollingTree::invalidate):
+ (WebCore::ThreadedScrollingTree::willStartRenderingUpdate):
+ (WebCore::ThreadedScrollingTree::waitForRenderingUpdateCompletionOrTimeout):
+ (WebCore::ThreadedScrollingTree::didCompleteRenderingUpdate):
+ (WebCore::ThreadedScrollingTree::scheduleDelayedRenderingUpdateDetectionTimer):
+ (WebCore::ThreadedScrollingTree::delayedRenderingUpdateDetectionTimerFired):
+ (WebCore::ThreadedScrollingTree::displayDidRefreshOnScrollingThread):
+ * page/scrolling/ThreadedScrollingTree.h:
+ (WebCore::ThreadedScrollingTree::WTF_RETURNS_LOCK):
+ (WebCore::ThreadedScrollingTree::WTF_GUARDED_BY_LOCK):
+ * page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm:
+ (WebCore::ScrollingTreeScrollingNodeDelegateMac::createTimer):
+
+2021-05-21 Chris Dumez <cdu...@apple.com>
+
Adopt CheckedLock in AudioMediaStreamTrackRendererUnit
https://bugs.webkit.org/show_bug.cgi?id=226095
Modified: trunk/Source/WebCore/page/scrolling/ScrollingTree.cpp (277892 => 277893)
--- trunk/Source/WebCore/page/scrolling/ScrollingTree.cpp 2021-05-21 22:14:58 UTC (rev 277892)
+++ trunk/Source/WebCore/page/scrolling/ScrollingTree.cpp 2021-05-21 22:17:33 UTC (rev 277893)
@@ -125,7 +125,7 @@
{
LOG_WITH_STREAM(Scrolling, stream << "\nScrollingTree " << this << " handleWheelEvent " << wheelEvent);
- LockHolder locker(m_treeMutex);
+ Locker locker { m_treeLock };
if (isMonitoringWheelEvents())
receivedWheelEvent(wheelEvent);
@@ -224,7 +224,7 @@
void ScrollingTree::traverseScrollingTree(VisitorFunction&& visitorFunction)
{
- LockHolder locker(m_treeMutex);
+ Locker locker { m_treeLock };
if (!m_rootNode)
return;
@@ -265,7 +265,7 @@
void ScrollingTree::commitTreeState(std::unique_ptr<ScrollingStateTree>&& scrollingStateTree)
{
SetForScope<bool> inCommitTreeState(m_inCommitTreeState, true);
- LockHolder locker(m_treeMutex);
+ Locker locker { m_treeLock };
bool rootStateNodeChanged = scrollingStateTree->hasNewRootStateNode();
@@ -417,7 +417,7 @@
void ScrollingTree::applyLayerPositions()
{
ASSERT(isMainThread());
- LockHolder locker(m_treeMutex);
+ Locker locker { m_treeLock };
applyLayerPositionsInternal();
}
@@ -424,7 +424,7 @@
void ScrollingTree::applyLayerPositionsInternal()
{
- ASSERT(m_treeMutex.isLocked());
+ ASSERT(m_treeLock.isLocked());
if (!m_rootNode)
return;
@@ -664,7 +664,7 @@
void ScrollingTree::scrollBySimulatingWheelEventForTesting(ScrollingNodeID nodeID, FloatSize delta)
{
- LockHolder locker(m_treeMutex);
+ Locker locker { m_treeLock };
auto* node = nodeForID(nodeID);
if (!is<ScrollingTreeScrollingNode>(node))
Modified: trunk/Source/WebCore/page/scrolling/ScrollingTree.h (277892 => 277893)
--- trunk/Source/WebCore/page/scrolling/ScrollingTree.h 2021-05-21 22:14:58 UTC (rev 277892)
+++ trunk/Source/WebCore/page/scrolling/ScrollingTree.h 2021-05-21 22:17:33 UTC (rev 277893)
@@ -217,7 +217,7 @@
virtual void willSendEventToMainThread(const PlatformWheelEvent&) { }
virtual void waitForEventToBeProcessedByMainThread(const PlatformWheelEvent&) { };
- Lock& treeMutex() { return m_treeMutex; }
+ CheckedLock& treeLock() WTF_RETURNS_LOCK(m_treeLock) { return m_treeLock; }
void windowScreenDidChange(PlatformDisplayID, Optional<FramesPerSecond> nominalFramesPerSecond);
PlatformDisplayID displayID();
@@ -257,18 +257,18 @@
Optional<FramesPerSecond> nominalFramesPerSecond();
- void applyLayerPositionsInternal();
- void removeAllNodes();
+ void applyLayerPositionsInternal() WTF_REQUIRES_LOCK(m_treeLock);
+ void removeAllNodes() WTF_REQUIRES_LOCK(m_treeLock);
- Lock m_treeMutex; // Protects the scrolling tree.
+ CheckedLock m_treeLock; // Protects the scrolling tree.
private:
- void updateTreeFromStateNodeRecursive(const ScrollingStateNode*, struct CommitTreeState&);
- virtual void propagateSynchronousScrollingReasons(const HashSet<ScrollingNodeID>&) { }
+ void updateTreeFromStateNodeRecursive(const ScrollingStateNode*, struct CommitTreeState&) WTF_REQUIRES_LOCK(m_treeLock);
+ virtual void propagateSynchronousScrollingReasons(const HashSet<ScrollingNodeID>&) WTF_REQUIRES_LOCK(m_treeLock) { }
- void applyLayerPositionsRecursive(ScrollingTreeNode&);
+ void applyLayerPositionsRecursive(ScrollingTreeNode&) WTF_REQUIRES_LOCK(m_treeLock);
void notifyRelatedNodesRecursive(ScrollingTreeNode&);
- void traverseScrollingTreeRecursive(ScrollingTreeNode&, const VisitorFunction&);
+ void traverseScrollingTreeRecursive(ScrollingTreeNode&, const VisitorFunction&) WTF_REQUIRES_LOCK(m_treeLock);
WEBCORE_EXPORT virtual RefPtr<ScrollingTreeNode> scrollingNodeForPoint(FloatPoint);
#if ENABLE(WHEEL_EVENT_REGIONS)
Modified: trunk/Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp (277892 => 277893)
--- trunk/Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp 2021-05-21 22:14:58 UTC (rev 277892)
+++ trunk/Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp 2021-05-21 22:17:33 UTC (rev 277893)
@@ -46,10 +46,10 @@
ThreadedScrollingTree::ThreadedScrollingTree(AsyncScrollingCoordinator& scrollingCoordinator)
: m_scrollingCoordinator(&scrollingCoordinator)
-{
#if ENABLE(SMOOTH_SCROLLING)
- m_scrollAnimatorEnabled = scrollingCoordinator.scrollAnimatorEnabled();
+ , m_scrollAnimatorEnabled(scrollingCoordinator.scrollAnimatorEnabled())
#endif
+{
}
ThreadedScrollingTree::~ThreadedScrollingTree()
@@ -70,7 +70,7 @@
LOG_WITH_STREAM(Scrolling, stream << "ThreadedScrollingTree::handleWheelEventAfterMainThread " << wheelEvent << " node " << targetNodeID << " gestureState " << gestureState);
- LockHolder locker(m_treeMutex);
+ Locker locker { m_treeLock };
bool allowLatching = false;
OptionSet<WheelEventProcessingSteps> processingSteps;
@@ -91,7 +91,7 @@
ASSERT(isMainThread());
- LockHolder locker(m_treeMutex);
+ Locker locker { m_treeLock };
if (m_receivedBeganEventFromMainThread || !wheelEvent.isGestureStart())
return;
@@ -105,7 +105,7 @@
{
ASSERT(ScrollingThread::isCurrentThread());
- LockHolder locker(m_treeMutex);
+ Locker locker { m_treeLock };
m_receivedBeganEventFromMainThread = false;
}
@@ -116,13 +116,14 @@
if (!wheelEvent.isGestureStart())
return;
- LockHolder locker(m_treeMutex);
+ Locker locker { m_treeLock };
static constexpr auto maxAllowableMainThreadDelay = 50_ms;
auto startTime = MonotonicTime::now();
auto timeoutTime = startTime + maxAllowableMainThreadDelay;
- bool receivedEvent = m_waitingForBeganEventCondition.waitUntil(m_treeMutex, timeoutTime, [&] {
+ bool receivedEvent = m_waitingForBeganEventCondition.waitUntil(m_treeLock, timeoutTime, [&] {
+ assertIsHeld(m_treeLock);
return m_receivedBeganEventFromMainThread;
});
@@ -141,7 +142,7 @@
// ScrollingCoordinator's page is destroyed.
ASSERT(ScrollingThread::isCurrentThread());
- LockHolder treeLocker(m_treeMutex);
+ Locker locker { m_treeLock };
removeAllNodes();
m_delayedRenderingUpdateDetectionTimer = nullptr;
@@ -279,14 +280,16 @@
tracePoint(ScrollingThreadRenderUpdateSyncStart);
- // Wait for the scrolling thread to acquire m_treeMutex. This ensures that any pending wheel events are processed.
+ // Wait for the scrolling thread to acquire m_treeLock. This ensures that any pending wheel events are processed.
BinarySemaphore semaphore;
ScrollingThread::dispatch([protectedThis = makeRef(*this), &semaphore]() {
- LockHolder treeLocker(protectedThis->m_treeMutex);
+ Locker treeLocker { protectedThis->m_treeLock };
semaphore.signal();
protectedThis->waitForRenderingUpdateCompletionOrTimeout();
});
semaphore.wait();
+
+ Locker locker { m_treeLock };
m_state = SynchronizationState::InRenderingUpdate;
}
@@ -301,13 +304,13 @@
// This code allows the main thread about half a frame to complete its rendering udpate. If the main thread
// is responsive (i.e. managing to render every frame), then we expect to get a didCompleteRenderingUpdate()
// within 8ms of willStartRenderingUpdate(). We time this via m_stateCondition, which blocks the scrolling
-// thread (with m_treeMutex locked at the start and end) so that we don't handle wheel events while waiting.
+// thread (with m_treeLock locked at the start and end) so that we don't handle wheel events while waiting.
// If the condition times out, we know the main thread is being slow, and allow the scrolling thread to
// commit layer positions.
void ThreadedScrollingTree::waitForRenderingUpdateCompletionOrTimeout()
{
ASSERT(ScrollingThread::isCurrentThread());
- ASSERT(m_treeMutex.isLocked());
+ ASSERT(m_treeLock.isLocked());
if (m_delayedRenderingUpdateDetectionTimer)
m_delayedRenderingUpdateDetectionTimer->stop();
@@ -315,11 +318,12 @@
auto startTime = MonotonicTime::now();
auto timeoutTime = startTime + maxAllowableRenderingUpdateDurationForSynchronization();
- bool becameIdle = m_stateCondition.waitUntil(m_treeMutex, timeoutTime, [&] {
+ bool becameIdle = m_stateCondition.waitUntil(m_treeLock, timeoutTime, [&] {
+ assertIsHeld(m_treeLock);
return m_state == SynchronizationState::Idle;
});
- ASSERT(m_treeMutex.isLocked());
+ ASSERT(m_treeLock.isLocked());
if (!becameIdle) {
m_state = SynchronizationState::Desynchronized;
@@ -335,7 +339,7 @@
void ThreadedScrollingTree::didCompleteRenderingUpdate()
{
ASSERT(isMainThread());
- LockHolder treeLocker(m_treeMutex);
+ Locker locker { m_treeLock };
if (m_state == SynchronizationState::InRenderingUpdate)
m_stateCondition.notifyOne();
@@ -346,7 +350,7 @@
void ThreadedScrollingTree::scheduleDelayedRenderingUpdateDetectionTimer(Seconds delay)
{
ASSERT(ScrollingThread::isCurrentThread());
- ASSERT(m_treeMutex.isLocked());
+ ASSERT(m_treeLock.isLocked());
if (!m_delayedRenderingUpdateDetectionTimer)
m_delayedRenderingUpdateDetectionTimer = makeUnique<RunLoop::Timer<ThreadedScrollingTree>>(RunLoop::current(), this, &ThreadedScrollingTree::delayedRenderingUpdateDetectionTimerFired);
@@ -358,7 +362,7 @@
{
ASSERT(ScrollingThread::isCurrentThread());
- LockHolder treeLocker(m_treeMutex);
+ Locker locker { m_treeLock };
if (canUpdateLayersOnScrollingThread())
applyLayerPositionsInternal();
m_state = SynchronizationState::Desynchronized;
@@ -369,7 +373,7 @@
TraceScope tracingScope(ScrollingThreadDisplayDidRefreshStart, ScrollingThreadDisplayDidRefreshEnd, displayID());
ASSERT(ScrollingThread::isCurrentThread());
- LockHolder treeLocker(m_treeMutex);
+ Locker locker { m_treeLock };
if (m_state != SynchronizationState::Idle && canUpdateLayersOnScrollingThread())
applyLayerPositionsInternal();
Modified: trunk/Source/WebCore/page/scrolling/ThreadedScrollingTree.h (277892 => 277893)
--- trunk/Source/WebCore/page/scrolling/ThreadedScrollingTree.h 2021-05-21 22:14:58 UTC (rev 277892)
+++ trunk/Source/WebCore/page/scrolling/ThreadedScrollingTree.h 2021-05-21 22:17:33 UTC (rev 277893)
@@ -29,7 +29,7 @@
#include "ScrollingStateTree.h"
#include "ScrollingTree.h"
-#include <wtf/Condition.h>
+#include <wtf/CheckedCondition.h>
#include <wtf/RefPtr.h>
#include <wtf/RunLoop.h>
@@ -60,7 +60,7 @@
void willStartRenderingUpdate();
void didCompleteRenderingUpdate();
- Lock& treeMutex() { return m_treeMutex; }
+ CheckedLock& treeLock() WTF_RETURNS_LOCK(m_treeLock) { return m_treeLock; }
bool scrollAnimatorEnabled() const { return m_scrollAnimatorEnabled; }
@@ -84,14 +84,14 @@
private:
bool isThreadedScrollingTree() const override { return true; }
- void propagateSynchronousScrollingReasons(const HashSet<ScrollingNodeID>&) override;
+ void propagateSynchronousScrollingReasons(const HashSet<ScrollingNodeID>&) WTF_REQUIRES_LOCK(m_treeLock) override;
void displayDidRefreshOnScrollingThread();
- void waitForRenderingUpdateCompletionOrTimeout();
+ void waitForRenderingUpdateCompletionOrTimeout() WTF_REQUIRES_LOCK(m_treeLock);
- bool canUpdateLayersOnScrollingThread() const;
+ bool canUpdateLayersOnScrollingThread() const WTF_REQUIRES_LOCK(m_treeLock);
- void scheduleDelayedRenderingUpdateDetectionTimer(Seconds);
+ void scheduleDelayedRenderingUpdateDetectionTimer(Seconds) WTF_REQUIRES_LOCK(m_treeLock);
void delayedRenderingUpdateDetectionTimerFired();
Seconds maxAllowableRenderingUpdateDurationForSynchronization();
@@ -103,17 +103,17 @@
Desynchronized,
};
- SynchronizationState m_state { SynchronizationState::Idle };
- Condition m_stateCondition;
+ SynchronizationState m_state WTF_GUARDED_BY_LOCK(m_treeLock) { SynchronizationState::Idle };
+ CheckedCondition m_stateCondition;
- bool m_receivedBeganEventFromMainThread { false };
- Condition m_waitingForBeganEventCondition;
+ bool m_receivedBeganEventFromMainThread WTF_GUARDED_BY_LOCK(m_treeLock) { false };
+ CheckedCondition m_waitingForBeganEventCondition;
// Dynamically allocated because it has to use the ScrollingThread's runloop.
- std::unique_ptr<RunLoop::Timer<ThreadedScrollingTree>> m_delayedRenderingUpdateDetectionTimer;
+ std::unique_ptr<RunLoop::Timer<ThreadedScrollingTree>> m_delayedRenderingUpdateDetectionTimer WTF_GUARDED_BY_LOCK(m_treeLock);
- bool m_scrollAnimatorEnabled { false };
- bool m_hasNodesWithSynchronousScrollingReasons { false };
+ const bool m_scrollAnimatorEnabled { false };
+ bool m_hasNodesWithSynchronousScrollingReasons WTF_GUARDED_BY_LOCK(m_treeLock) { false };
};
} // namespace WebCore
Modified: trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm (277892 => 277893)
--- trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm 2021-05-21 22:14:58 UTC (rev 277892)
+++ trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm 2021-05-21 22:17:33 UTC (rev 277893)
@@ -220,7 +220,7 @@
std::unique_ptr<ScrollControllerTimer> ScrollingTreeScrollingNodeDelegateMac::createTimer(Function<void()>&& function)
{
return WTF::makeUnique<ScrollControllerTimer>(RunLoop::current(), [function = WTFMove(function), protectedNode = makeRef(scrollingNode())] {
- LockHolder locker(protectedNode->scrollingTree().treeMutex());
+ Locker locker { protectedNode->scrollingTree().treeLock() };
function();
});
}
