Branch: refs/heads/main
Home: https://github.com/WebKit/WebKit
Commit: b2362a5d6b0dcee521bbc38c226ea2216e2422e7
https://github.com/WebKit/WebKit/commit/b2362a5d6b0dcee521bbc38c226ea2216e2422e7
Author: Wenson Hsieh <wenson_hs...@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@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
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
