Skip to content

Commit

Permalink
Accessing null RemoteScrollingCoordinatorProxy in [WKWebViewIOS _didF…
Browse files Browse the repository at this point in the history
…inishScrolling]

https://bugs.webkit.org/show_bug.cgi?id=255162
rdar://106894608

Reviewed by Wenson Hsieh.

We're seeing null derefs of a `RemoteScrollingCoordinatorProxy` at
[WKWebViewIOS _didFinishScrolling] in situations where a web view is
closed out during a scroll operation.

This regression surfaced from https://commits.webkit.org/260975@main
because it (correctly) changed the relative order of destruction between
the `DrawingAreaProxy` and the `RemoteScrollingCoordinatorProxy` (and
the `RemoteScrollingTree` it encompasses), which meant that there could
be situations where closing or switching out a web view in the middle of
a scroll operation would lead to a null deref of the
`RemoteScrollingCoordinatorProxy` held by the `WebPageProxy`.

In this patch, we add a null check on the
`RemoteScrollingCoordinatorProxy` instance held by `WebPageProxy` in two
different places:

- In `_didFinishScrolling`. This is OK to do because the web view has
  closed out by this point anyway (and `RemoteScrollingTree` instance
  has been destructed), so the `setRootNodeIsInUserScroll` method call
  would be a no-op anyway.
- In `scrollViewWillBeginDragging`. This is OK for much the same reason.

* Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:
(-[WKWebView scrollViewWillBeginDragging:]):
(-[WKWebView _didFinishScrolling:]):

Canonical link: https://commits.webkit.org/262748@main
  • Loading branch information
aprotyas authored and rr-codes committed Apr 8, 2023
1 parent 8341e05 commit 2aa252b
Showing 1 changed file with 6 additions and 5 deletions.
11 changes: 6 additions & 5 deletions Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm
Expand Up @@ -1818,11 +1818,11 @@ - (void)scrollViewWillBeginDragging:(UIScrollView *)scrollView

#if ENABLE(ASYNC_SCROLLING)
// FIXME: We will want to detect whether snapping will occur before beginning to drag. See WebPageProxy::didCommitLayerTree.
auto* coordinator = downcast<WebKit::RemoteScrollingCoordinatorProxyIOS>(_page->scrollingCoordinatorProxy());
ASSERT(scrollView == _scrollView.get());
[_scrollView _setDecelerationRateInternal:(coordinator && coordinator->shouldSetScrollViewDecelerationRateFast()) ? UIScrollViewDecelerationRateFast : UIScrollViewDecelerationRateNormal];

coordinator->setRootNodeIsInUserScroll(true);
if (auto* coordinator = downcast<WebKit::RemoteScrollingCoordinatorProxyIOS>(_page->scrollingCoordinatorProxy())) {
[_scrollView _setDecelerationRateInternal:(coordinator->shouldSetScrollViewDecelerationRateFast()) ? UIScrollViewDecelerationRateFast : UIScrollViewDecelerationRateNormal];
coordinator->setRootNodeIsInUserScroll(true);
}
#endif
}

Expand All @@ -1838,7 +1838,8 @@ - (void)_didFinishScrolling:(UIScrollView *)scrollView
[_contentView didFinishScrolling];

#if ENABLE(ASYNC_SCROLLING)
_page->scrollingCoordinatorProxy()->setRootNodeIsInUserScroll(false);
if (auto* coordinator = _page->scrollingCoordinatorProxy())
coordinator->setRootNodeIsInUserScroll(false);
#endif
}

Expand Down

0 comments on commit 2aa252b

Please sign in to comment.