Changeset 290109 in webkit

Timestamp:

Feb 17, 2022, 7:55:27 PM (5 years ago)

Author:

Simon Fraser

Message:

Crash under ScrollingCoordinatorMac::hasNodeWithAnimatedScrollChanged()
​https://bugs.webkit.org/show_bug.cgi?id=236810
<rdar://89072235>

Reviewed by Dean Jackson.

Crash data suggests that r288933 didn't fix the crash entirely.

Although the code paths I can find appear to hold the lock, there may be an
opportunity for m_scrollingCoordinator to get nulled out after we null-check it
but before the copy for the main thread dispatch.

So make a RefPtr on the stack, null-check it, and then move it into the callback.

• page/scrolling/ThreadedScrollingTree.cpp:

(WebCore::ThreadedScrollingTree::reportSynchronousScrollingReasonsChanged):
(WebCore::ThreadedScrollingTree::reportExposedUnfilledArea):
(WebCore::ThreadedScrollingTree::currentSnapPointIndicesDidChange):
(WebCore::ThreadedScrollingTree::handleWheelEventPhase):
(WebCore::ThreadedScrollingTree::setActiveScrollSnapIndices):
(WebCore::ThreadedScrollingTree::hasNodeWithAnimatedScrollChanged):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• page/scrolling/ThreadedScrollingTree.cpp (modified) (6 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2022-02-17  Simon Fraser  <simon.fraser@apple.com>
+
+        Crash under ScrollingCoordinatorMac::hasNodeWithAnimatedScrollChanged()
+        https://bugs.webkit.org/show_bug.cgi?id=236810
+        <rdar://89072235>
+
+        Reviewed by Dean Jackson.
+
+        Crash data suggests that r288933 didn't fix the crash entirely.
+
+        Although the code paths I can find appear to hold the lock, there may be an
+        opportunity for m_scrollingCoordinator to get nulled out after we null-check it
+        but before the copy for the main thread dispatch.
+
+        So make a RefPtr on the stack, null-check it, and then move it into the callback.
+
+        * page/scrolling/ThreadedScrollingTree.cpp:
+        (WebCore::ThreadedScrollingTree::reportSynchronousScrollingReasonsChanged):
+        (WebCore::ThreadedScrollingTree::reportExposedUnfilledArea):
+        (WebCore::ThreadedScrollingTree::currentSnapPointIndicesDidChange):
+        (WebCore::ThreadedScrollingTree::handleWheelEventPhase):
+        (WebCore::ThreadedScrollingTree::setActiveScrollSnapIndices):
+        (WebCore::ThreadedScrollingTree::hasNodeWithAnimatedScrollChanged):
+
 2022-02-17  Chris Dumez  <cdumez@apple.com>
 

trunk/Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp

@@ -284 +284 @@
 void ThreadedScrollingTree::reportSynchronousScrollingReasonsChanged(MonotonicTime timestamp, OptionSet<SynchronousScrollingReason> reasons)
 {
-    RunLoop::main().dispatch([scrollingCoordinator = m_scrollingCoordinator, timestamp, reasons] {
+    auto scrollingCoordinator = m_scrollingCoordinator;
+    if (!scrollingCoordinator)
+        return;
+
+    RunLoop::main().dispatch([scrollingCoordinator = WTFMove(scrollingCoordinator), timestamp, reasons] {
         scrollingCoordinator->reportSynchronousScrollingReasonsChanged(timestamp, reasons);
     });
@@ -291 +295 @@
 void ThreadedScrollingTree::reportExposedUnfilledArea(MonotonicTime timestamp, unsigned unfilledArea)
 {
-    RunLoop::main().dispatch([scrollingCoordinator = m_scrollingCoordinator, timestamp, unfilledArea] {
+    auto scrollingCoordinator = m_scrollingCoordinator;
+    if (!scrollingCoordinator)
+        return;
+    RunLoop::main().dispatch([scrollingCoordinator = WTFMove(scrollingCoordinator), timestamp, unfilledArea] {
         scrollingCoordinator->reportExposedUnfilledArea(timestamp, unfilledArea);
     });
@@ -299 +306 @@
 void ThreadedScrollingTree::currentSnapPointIndicesDidChange(ScrollingNodeID nodeID, std::optional<unsigned> horizontal, std::optional<unsigned> vertical)
 {
-    if (!m_scrollingCoordinator)
-        return;
-
-    RunLoop::main().dispatch([scrollingCoordinator = m_scrollingCoordinator, nodeID, horizontal, vertical] {
+    auto scrollingCoordinator = m_scrollingCoordinator;
+    if (!scrollingCoordinator)
+        return;
+
+    RunLoop::main().dispatch([scrollingCoordinator = WTFMove(scrollingCoordinator), nodeID, horizontal, vertical] {
         scrollingCoordinator->setActiveScrollSnapIndices(nodeID, horizontal, vertical);
     });
@@ -311 +319 @@
 void ThreadedScrollingTree::handleWheelEventPhase(ScrollingNodeID nodeID, PlatformWheelEventPhase phase)
 {
-    if (!m_scrollingCoordinator)
-        return;
-
-    RunLoop::main().dispatch([scrollingCoordinator = m_scrollingCoordinator, nodeID, phase] {
+    auto scrollingCoordinator = m_scrollingCoordinator;
+    if (!scrollingCoordinator)
+        return;
+
+    RunLoop::main().dispatch([scrollingCoordinator = WTFMove(scrollingCoordinator), nodeID, phase] {
         scrollingCoordinator->handleWheelEventPhase(nodeID, phase);
     });
@@ -321 +330 @@
 void ThreadedScrollingTree::setActiveScrollSnapIndices(ScrollingNodeID nodeID, std::optional<unsigned> horizontalIndex, std::optional<unsigned> verticalIndex)
 {
-    if (!m_scrollingCoordinator)
+    auto scrollingCoordinator = m_scrollingCoordinator;
+    if (!scrollingCoordinator)
         return;
     
-    RunLoop::main().dispatch([scrollingCoordinator = m_scrollingCoordinator, nodeID, horizontalIndex, verticalIndex] {
+    RunLoop::main().dispatch([scrollingCoordinator = WTFMove(scrollingCoordinator), nodeID, horizontalIndex, verticalIndex] {
         scrollingCoordinator->setActiveScrollSnapIndices(nodeID, horizontalIndex, verticalIndex);
     });
@@ -378 +388 @@
 void ThreadedScrollingTree::hasNodeWithAnimatedScrollChanged(bool hasNodeWithAnimatedScroll)
 {
-    if (!m_scrollingCoordinator)
-        return;
-
-    RunLoop::main().dispatch([scrollingCoordinator = m_scrollingCoordinator, hasNodeWithAnimatedScroll] {
+    auto scrollingCoordinator = m_scrollingCoordinator;
+    if (!scrollingCoordinator)
+        return;
+
+    RunLoop::main().dispatch([scrollingCoordinator = WTFMove(scrollingCoordinator), hasNodeWithAnimatedScroll] {
         scrollingCoordinator->hasNodeWithAnimatedScrollChanged(hasNodeWithAnimatedScroll);
     });