[webkit-changes] [WebKit/WebKit] b2362a: [UI-side compositing] Safari occasionally crashes ...

[Wenson Hsieh]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: b2362a5d6b0dcee521bbc38c226ea2216e2422e7
      https://github.com/WebKit/WebKit/commit/b2362a5d6b0dcee521bbc38c226ea2216e2422e7
  Author: Wenson Hsieh <wenson_hsieh at apple.com>
  Date:   2023-03-27 (Mon, 27 Mar 2023)

  Changed paths:
    M LayoutTests/tiled-drawing/scrolling/fast-scroll-div-latched-mainframe-with-handler.html
    M Source/WebCore/WebCore.xcodeproj/project.pbxproj
    M Source/WebCore/page/scrolling/mac/ScrollerMac.mm
    M Source/WebCore/page/scrolling/mac/ScrollerPairMac.h
    M Source/WebCore/page/scrolling/mac/ScrollerPairMac.mm
    M Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.h
    M Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm
    A Source/WebCore/platform/mac/ScrollTypesMac.h

  Log Message:
  -----------
  [UI-side compositing] Safari occasionally crashes when scrolling underneath `NSScrollerImpPair`
https://bugs.webkit.org/show_bug.cgi?id=254484
rdar://107139674

Reviewed by Simon Fraser.

After the changes in 259592 at main, we now update `NSScrollerImpPair` in response to various scrolling
-related events off of the scrolling thread, with a lock (`m_scrollerImpPairLock`) to prevent us
from concurrently updating the `NSScrollerImpPair`. However, this is actually insufficient to ensure
thread safety since `NSScrollerImpPair` internally schedules work on the main thread underneath
`-[NSScrollerImpPair endScrollGesture]`. As such, it's possible to end up calling methods on an
`NSTimer` that has already been destroyed.

Since we can't prevent `NSScrollerImpPair` from rescheduling its hide timer on the main thread, we
need to instead dispatch all of our other calls to update the imp pair on the main thread. To
achieve this, we introduce a new helper method on `ScrollerPairMac` to asynchronously dispatch to
the main thread from the scrolling thread with the `NSScrollerImpPair`, or call the block
immediately in the case where we're already on the main thread, and deploy this in all the places
where we currently update the `NSScrollerImpPair`. See below for more details.

I wasn't able to reproduce the crash in either a test, or manually in MiniBrowser/Safari, but I was
able to reproduce it and verify this fix by injecting a `sleep()` in `NSScrollerImpPair`, after the
hiding timer is deallocated but before the pointer is reset to nil.

* LayoutTests/tiled-drawing/scrolling/fast-scroll-div-latched-mainframe-with-handler.html:

Adjust a test to avoid flakiness after this change, since we now dispatch to the main thread before
updating the imp pair.

* Source/WebCore/WebCore.xcodeproj/project.pbxproj:
* Source/WebCore/page/scrolling/mac/ScrollerMac.mm:
(WebCore::ScrollerMac::attach):

Make this use the new getter for `scrollbarStyle()`.

(WebCore::ScrollerMac::setHostLayer):

Use another helper method here to avoid mutating `NSScrollerImpPair` from a background thread.

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

Make `ScrollerPairMac` thread-safe ref-counted, so that we can safely create references to it
from the scrolling thread, for blocks that are dispatched to the main thread.

We also remove `m_scrollerImpPairLock`, since all access to the `NSScrollerImpPair` now happens on
the main thread.

(WebCore::ScrollerPairMac::create):
(WebCore::ScrollerPairMac::lastKnownMousePosition const):
(WebCore::ScrollerPairMac::scrollbarStyle const):
(WebCore::ScrollerPairMac::scrollerImpVertical):
(WebCore::ScrollerPairMac::scrollerImpPair): Deleted.
(WebCore::ScrollerPairMac::WTF_RETURNS_LOCK): Deleted.
* Source/WebCore/page/scrolling/mac/ScrollerPairMac.mm:
(-[WebScrollerImpPairDelegateMac scrollerImpPair:updateScrollerStyleForNewRecommendedScrollerStyle:]):

Make this use the new `setScrollbarStyle` helpers, which internally dispatch onto the main thread
before attempting to access `NSScrollerImpPair`.

(WebCore::ScrollerPairMac::ScrollerPairMac):
(WebCore::ScrollerPairMac::init):

Drive-by adjustments: remove some unnecessary `WebCore::` namespacing, now that this entire class is
already in the WebCore namespace.

(WebCore::ScrollerPairMac::~ScrollerPairMac):

Also ensure that the underlying `NSScrollerImpPair` is also destroyed on the main thread.

(WebCore::ScrollerPairMac::handleWheelEventPhase):
(WebCore::ScrollerPairMac::viewWillStartLiveResize):
(WebCore::ScrollerPairMac::viewWillEndLiveResize):
(WebCore::ScrollerPairMac::contentsSizeChanged):
(WebCore::ScrollerPairMac::handleMouseEvent):
(WebCore::ScrollerPairMac::updateValues):

Deploy `ensureOnMainThreadWithProtectedThis` in various places where we update `NSScrollerImpPair`.

(WebCore::ScrollerPairMac::visibleSize const):
(WebCore::ScrollerPairMac::valuesForOrientation):
(WebCore::ScrollerPairMac::setVerticalScrollerImp):
(WebCore::ScrollerPairMac::setHorizontalScrollerImp):
(WebCore::ScrollerPairMac::setScrollbarStyle):
(WebCore::ScrollerPairMac::ensureOnMainThreadWithProtectedThis):

Add a helper here to make it easier to dispatch methods that mutate or update the `NSScrollerImpPair`
onto the main thread. If we're on the scrolling thread, this dispatches asynchronously on to the
main thread; otherwise, we'll run the block right away if we're already on the main thread.

* Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.h:

Turn `m_scrollerPair` into a `Ref`, now that the class is ref-counted.

* Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm:
(WebCore::ScrollingTreeScrollingNodeDelegateMac::ScrollingTreeScrollingNodeDelegateMac):
(WebCore::ScrollingTreeScrollingNodeDelegateMac::~ScrollingTreeScrollingNodeDelegateMac):
(WebCore::ScrollingTreeScrollingNodeDelegateMac::updateFromStateNode):
(WebCore::ScrollingTreeScrollingNodeDelegateMac::handleWheelEvent):
(WebCore::ScrollingTreeScrollingNodeDelegateMac::updateScrollbarPainters):
(WebCore::ScrollingTreeScrollingNodeDelegateMac::initScrollbars):
(WebCore::ScrollingTreeScrollingNodeDelegateMac::updateScrollbarLayers):
(WebCore::ScrollingTreeScrollingNodeDelegateMac::handleWheelEventPhase):
(WebCore::ScrollingTreeScrollingNodeDelegateMac::handleMouseEventForScrollbars):
(WebCore::ScrollingTreeScrollingNodeDelegateMac::viewWillStartLiveResize):
(WebCore::ScrollingTreeScrollingNodeDelegateMac::viewWillEndLiveResize):
(WebCore::ScrollingTreeScrollingNodeDelegateMac::viewSizeDidChange):
(WebCore::ScrollingTreeScrollingNodeDelegateMac::scrollbarStateForOrientation const):
* Source/WebCore/platform/mac/ScrollTypesMac.h: Added.
(WebCore::nsScrollerStyle):
(WebCore::scrollbarStyle):

Update all these call sites to use `->` instead of `.` when accessing `m_scrollerPair`.

Canonical link: https://commits.webkit.org/262194@main