heap-use-after-free in WebCore::ScrollingTreeScrollingNode::scrollbar…

…VisibilityDidChange

https://bugs.webkit.org/show_bug.cgi?id=267788
<rdar://119677269>

Reviewed by Chris Dumez.

We register a `WebScrollerImpPairDelegateMac` with a `NSScrollerImpPair` (an AppKit class), so its lifetime
is somewhat unknown. The `WebScrollerImpPairDelegateMac` references a `ScrollerPairMac` by raw pointer, which
should be cleaned up by `-invalidate` but convert it to a `ThreadSafeWeakPtr<>` for safety.

The larger issue is that the `ScrollerPairMac` stored a `ScrollingTreeScrollingNode` by reference, so
nothing guaranteed that the `ScrollingTreeScrollingNode` outlived the `ScrollerPairMac`. Convert this to
a `ThreadSafeWeakPtr<>` also.

I was not able to reproduce the bug with the provided testcase.

* Source/WebCore/page/scrolling/mac/ScrollerMac.mm:
(WebCore::ScrollerMac::visibilityChanged):
(WebCore::ScrollerMac::updateMinimumKnobLength):
* Source/WebCore/page/scrolling/mac/ScrollerPairMac.h:
(WebCore::ScrollerPairMac::protectedNode const):
(WebCore::ScrollerPairMac::node const): Deleted.
* Source/WebCore/page/scrolling/mac/ScrollerPairMac.mm:
(-[WebScrollerImpPairDelegateMac contentAreaRectForScrollerImpPair:]):
(-[WebScrollerImpPairDelegateMac inLiveResizeForScrollerImpPair:]):
(-[WebScrollerImpPairDelegateMac scrollerImpPair:convertContentPoint:toScrollerImp:]):
(-[WebScrollerImpPairDelegateMac scrollerImpPair:updateScrollerStyleForNewRecommendedScrollerStyle:]):
(WebCore::ScrollerPairMac::updateValues):
(WebCore::ScrollerPairMac::visibleSize const):
(WebCore::ScrollerPairMac::useDarkAppearance const):
(WebCore::ScrollerPairMac::scrollbarWidthStyle const):
(WebCore::ScrollerPairMac::valuesForOrientation):
(WebCore::ScrollerPairMac::mouseEnteredContentArea):
(WebCore::ScrollerPairMac::mouseExitedContentArea):

Originally-landed-as: 272448.348@safari-7618-branch (5572fc8). rdar://124555073
Canonical link: https://commits.webkit.org/276166@main

Source/WebCore/page/scrolling/mac/ScrollerMac.mm

@@ -423,15 +423,19 @@ - (void)invalidate
     if (m_isVisible == isVisible)
         return;
     m_isVisible = isVisible;
-    m_pair.node().scrollbarVisibilityDidChange(m_orientation, isVisible);
+
+    if (RefPtr node = m_pair.protectedNode())
+        node->scrollbarVisibilityDidChange(m_orientation, isVisible);
 }
 
 void ScrollerMac::updateMinimumKnobLength(int minimumKnobLength)
 {
     if (m_minimumKnobLength == minimumKnobLength)
         return;
     m_minimumKnobLength = minimumKnobLength;
-    m_pair.node().scrollbarMinimumThumbLengthDidChange(m_orientation, m_minimumKnobLength);
+
+    if (RefPtr node = m_pair.protectedNode())
+        node->scrollbarMinimumThumbLengthDidChange(m_orientation, m_minimumKnobLength);
 }
 
 void ScrollerMac::setScrollerImp(NSScrollerImp *imp)

Source/WebCore/page/scrolling/mac/ScrollerPairMac.h

@@ -108,8 +108,8 @@ class ScrollerPairMac : public ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<S
 
     NSScrollerImpPair *scrollerImpPair() const { return m_scrollerImpPair.get(); }
     void ensureOnMainThreadWithProtectedThis(Function<void()>&&);
-    ScrollingTreeScrollingNode& node() const { return m_scrollingNode; }
-    
+    RefPtr<ScrollingTreeScrollingNode> protectedNode() const { return m_scrollingNode.get(); }
+
     bool mouseInContentArea() const { return m_mouseInContentArea; }
 
 private:
@@ -118,8 +118,7 @@ class ScrollerPairMac : public ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<S
     NSScrollerImp *scrollerImpHorizontal() { return horizontalScroller().scrollerImp(); }
     NSScrollerImp *scrollerImpVertical() { return verticalScroller().scrollerImp(); }
 
-
-    ScrollingTreeScrollingNode& m_scrollingNode;
+    ThreadSafeWeakPtr<ScrollingTreeScrollingNode> m_scrollingNode;
 
     ScrollbarHoverState m_scrollbarHoverState;
 

Source/WebCore/page/scrolling/mac/ScrollerPairMac.mm

@@ -41,7 +41,7 @@
 #import <pal/spi/mac/NSScrollerImpSPI.h>
 
 @interface WebScrollerImpPairDelegateMac : NSObject <NSScrollerImpPairDelegate> {
-    WebCore::ScrollerPairMac* _scrollerPair;
+    ThreadSafeWeakPtr<WebCore::ScrollerPairMac> _scrollerPair;
 }
 - (id)initWithScrollerPair:(WebCore::ScrollerPairMac*)scrollerPair;
 @end
@@ -66,16 +66,21 @@ - (void)invalidate
 - (NSRect)contentAreaRectForScrollerImpPair:(NSScrollerImpPair *)scrollerImpPair
 {
     UNUSED_PARAM(scrollerImpPair);
-    if (!_scrollerPair)
+    RefPtr scrollerPair = _scrollerPair.get();
+    if (!scrollerPair)
         return NSZeroRect;
 
-    auto size = _scrollerPair->visibleSize();
+    auto size = scrollerPair->visibleSize();
     return NSMakeRect(0, 0, size.width(), size.height());
 }
 
 - (BOOL)inLiveResizeForScrollerImpPair:(NSScrollerImpPair *)scrollerImpPair
 {
-    return _scrollerPair->inLiveResize();
+    RefPtr scrollerPair = _scrollerPair.get();
+    if (!scrollerPair)
+        return NO;
+
+    return scrollerPair->inLiveResize();
 }
 
 - (NSPoint)mouseLocationInContentAreaForScrollerImpPair:(NSScrollerImpPair *)scrollerImpPair
@@ -91,14 +96,18 @@ - (NSPoint)scrollerImpPair:(NSScrollerImpPair *)scrollerImpPair convertContentPo
     UNUSED_PARAM(scrollerImpPair);
     UNUSED_PARAM(pointInContentArea);
 
-    if (!_scrollerPair || !scrollerImp)
+    RefPtr scrollerPair = _scrollerPair.get();
+    if (!scrollerPair)
+        return NSZeroPoint;
+
+    if (!scrollerPair || !scrollerImp)
         return NSZeroPoint;
 
     WebCore::ScrollerMac* scroller = nullptr;
     if ([scrollerImp isHorizontal])
-        scroller = &_scrollerPair->horizontalScroller();
+        scroller = &scrollerPair->horizontalScroller();
     else
-        scroller = &_scrollerPair->verticalScroller();
+        scroller = &scrollerPair->verticalScroller();
 
     ASSERT(scrollerImp == scroller->scrollerImp());
 
@@ -115,7 +124,9 @@ - (void)scrollerImpPair:(NSScrollerImpPair *)scrollerImpPair updateScrollerStyle
 {
     UNUSED_PARAM(scrollerImpPair);
 
-    _scrollerPair->setScrollbarStyle(WebCore::scrollbarStyle(newRecommendedScrollerStyle));
+    RefPtr scrollerPair = _scrollerPair.get();
+    if (scrollerPair)
+        scrollerPair->setScrollbarStyle(WebCore::scrollbarStyle(newRecommendedScrollerStyle));
 }
 
 @end
@@ -235,7 +246,11 @@ - (void)scrollerImpPair:(NSScrollerImpPair *)scrollerImpPair updateScrollerStyle
 
 void ScrollerPairMac::updateValues()
 {
-    auto offset = m_scrollingNode.currentScrollOffset();
+    RefPtr node = m_scrollingNode.get();
+    if (!node)
+        return;
+
+    auto offset = node->currentScrollOffset();
 
     if (offset != m_lastScrollOffset) {
         if (m_lastScrollOffset) {
@@ -252,32 +267,48 @@ - (void)scrollerImpPair:(NSScrollerImpPair *)scrollerImpPair updateScrollerStyle
 
 FloatSize ScrollerPairMac::visibleSize() const
 {
-    return m_scrollingNode.scrollableAreaSize();
+    RefPtr node = m_scrollingNode.get();
+    if (!node)
+        return { };
+
+    return node->scrollableAreaSize();
 }
 
 bool ScrollerPairMac::useDarkAppearance() const
 {
-    return m_scrollingNode.useDarkAppearanceForScrollbars();
+    RefPtr node = m_scrollingNode.get();
+    if (!node)
+        return false;
+
+    return node->useDarkAppearanceForScrollbars();
 }
 
 ScrollbarWidth ScrollerPairMac::scrollbarWidthStyle() const
 {
-    return m_scrollingNode.scrollbarWidthStyle();
+    RefPtr node = m_scrollingNode.get();
+    if (!node)
+        return ScrollbarWidth::Auto;
+
+    return node->scrollbarWidthStyle();
 }
 
 ScrollerPairMac::Values ScrollerPairMac::valuesForOrientation(ScrollbarOrientation orientation)
 {
+    RefPtr node = m_scrollingNode.get();
+    if (!node)
+        return { };
+
     float position;
     float totalSize;
     float visibleSize;
     if (orientation == ScrollbarOrientation::Vertical) {
-        position = m_scrollingNode.currentScrollOffset().y();
-        totalSize = m_scrollingNode.totalContentsSize().height();
-        visibleSize = m_scrollingNode.scrollableAreaSize().height();
+        position = node->currentScrollOffset().y();
+        totalSize = node->totalContentsSize().height();
+        visibleSize = node->scrollableAreaSize().height();
     } else {
-        position = m_scrollingNode.currentScrollOffset().x();
-        totalSize = m_scrollingNode.totalContentsSize().width();
-        visibleSize = m_scrollingNode.scrollableAreaSize().width();
+        position = node->currentScrollOffset().x();
+        totalSize = node->totalContentsSize().width();
+        visibleSize = node->scrollableAreaSize().width();
     }
 
     float value;
@@ -343,7 +374,7 @@ - (void)scrollerImpPair:(NSScrollerImpPair *)scrollerImpPair updateScrollerStyle
 
 void ScrollerPairMac::mouseEnteredContentArea()
 {
-    LOG_WITH_STREAM(OverlayScrollbars, stream << "ScrollerPairMac for [" << m_scrollingNode.scrollingNodeID() << "] mouseEnteredContentArea");
+    LOG_WITH_STREAM(OverlayScrollbars, stream << "ScrollerPairMac for [" << protectedNode()->scrollingNodeID() << "] mouseEnteredContentArea");
 
     ensureOnMainThreadWithProtectedThis([this] {
         if ([m_scrollerImpPair overlayScrollerStateIsLocked])
@@ -356,8 +387,8 @@ - (void)scrollerImpPair:(NSScrollerImpPair *)scrollerImpPair updateScrollerStyle
 void ScrollerPairMac::mouseExitedContentArea()
 {
     m_mouseInContentArea = false;
-    LOG_WITH_STREAM(OverlayScrollbars, stream << "ScrollerPairMac for [" << m_scrollingNode.scrollingNodeID() << "] mouseExitedContentArea");
-    
+    LOG_WITH_STREAM(OverlayScrollbars, stream << "ScrollerPairMac for [" << protectedNode()->scrollingNodeID() << "] mouseExitedContentArea");
+
     ensureOnMainThreadWithProtectedThis([this] {
         if ([m_scrollerImpPair overlayScrollerStateIsLocked])
             return;