225950 – Release assert in Document::updateStyleIfNeeded() via ScrollableArea::updateScrollSnapState

RESOLVED FIXED 225950

Release assert in Document::updateStyleIfNeeded() via ScrollableArea::updateScrollSnapState

https://bugs.webkit.org/show_bug.cgi?id=225950

Summary Release assert in Document::updateStyleIfNeeded() via ScrollableArea::updateS...

Ryosuke Niwa

Reported 2021-05-18 18:13:39 PDT

Created attachment 429012 [details] Test e.g. Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebCore 0x0000000540f3192b WTFCrashWithInfo(int, char const*, char const*, int) + 27 (Assertions.h:695) 1 com.apple.WebCore 0x000000054489b226 WebCore::Document::updateStyleIfNeeded() + 662 (Document.cpp:2177) 2 com.apple.WebCore 0x0000000545acfb9f WebCore::FrameViewLayoutContext::updateStyleForLayout() + 111 (FrameViewLayoutContext.cpp:545) 3 com.apple.WebCore 0x0000000545a9c2d1 WebCore::FrameViewLayoutContext::layout() + 721 (FrameViewLayoutContext.cpp:211) 4 com.apple.WebCore 0x0000000545aba4d3 WebCore::FrameView::updateContentsSize() + 67 (FrameView.cpp:2788) 5 com.apple.WebCore 0x0000000545d74195 WebCore::ScrollView::updateScrollbars(WebCore::IntPoint const&) + 3349 (ScrollView.cpp:711) 6 com.apple.WebCore 0x0000000545d807d4 WebCore::ScrollView::setFrameRect(WebCore::IntRect const&) + 420 (ScrollView.cpp:1119) 7 com.apple.WebCore 0x0000000545aa4219 WebCore::FrameView::setFrameRect(WebCore::IntRect const&) + 345 (FrameView.cpp:450) 8 com.apple.WebCore 0x000000054698fc1a WebCore::RenderWidget::setWidgetGeometry(WebCore::LayoutRect const&) + 762 (RenderWidget.cpp:140) 9 com.apple.WebCore 0x00000005469904d4 WebCore::RenderWidget::updateWidgetGeometry() + 820 10 com.apple.WebCore 0x0000000546992e57 WebCore::RenderWidget::updateWidgetPosition() + 231 (RenderWidget.cpp:354) 11 com.apple.WebCore 0x0000000545ab9866 WebCore::FrameView::updateWidgetPositions() + 278 (FrameView.cpp:5443) 12 com.apple.WebCore 0x0000000545ab8b63 WebCore::FrameView::updateLayerPositionsAfterScrolling() + 83 (FrameView.cpp:2600) 13 com.apple.WebCore 0x0000000545d78f77 WebCore::ScrollView::completeUpdatesAfterScrollTo(WebCore::IntSize const&) + 71 (ScrollView.cpp:513) 14 com.apple.WebCore 0x0000000545d79242 WebCore::ScrollView::scrollTo(WebCore::IntPoint const&) + 450 (ScrollView.cpp:508) 15 com.apple.WebCore 0x0000000545ac0581 WebCore::FrameView::scrollTo(WebCore::IntPoint const&) + 241 (FrameView.cpp:3760) 16 com.apple.WebCore 0x0000000545d78676 WebCore::ScrollView::setScrollOffset(WebCore::IntPoint const&) + 534 (ScrollView.cpp:447) 17 com.apple.WebCore 0x0000000545d86753 WebCore::ScrollableArea::scrollPositionChanged(WebCore::IntPoint const&) + 451 (ScrollableArea.cpp:183) 18 com.apple.WebCore 0x0000000545d8650a WebCore::ScrollableArea::notifyScrollPositionChanged(WebCore::IntPoint const&) + 186 (ScrollableArea.cpp:175) 19 com.apple.WebCore 0x0000000545ca38b1 WebCore::AsyncScrollingCoordinator::reconcileScrollingState(WebCore::FrameView&, WebCore::FloatPoint const&, WTF::Variant<WTF::Optional<WebCore::FloatPoint>, WTF::Optional<WebCore::FloatRect> > const&, WebCore::ScrollType, WebCore::ViewportRectStability, WebCore::ScrollingLayerPositionAction) + 625 (AsyncScrollingCoordinator.cpp:417) 20 com.apple.WebCore 0x0000000545ca33fd WebCore::AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll(unsigned long long, WebCore::FloatPoint const&, WTF::Optional<WebCore::FloatPoint>, WebCore::ScrollType, WebCore::ScrollingLayerPositionAction) + 509 (AsyncScrollingCoordinator.cpp:377) 21 com.apple.WebCore 0x0000000545ca2c3c WebCore::AsyncScrollingCoordinator::applyScrollUpdate(unsigned long long, WebCore::FloatPoint const&, WTF::Optional<WebCore::FloatPoint>, WebCore::ScrollType, WebCore::ScrollingLayerPositionAction) + 236 (AsyncScrollingCoordinator.cpp:353) 22 com.apple.WebCore 0x0000000545cfffb3 WebCore::ThreadedScrollingTree::scrollingTreeNodeDidScroll(WebCore::ScrollingTreeScrollingNode&, WebCore::ScrollingLayerPositionAction) + 675 (ThreadedScrollingTree.cpp:209) 23 com.apple.WebCore 0x0000000545cfe180 WebCore::ScrollingTreeScrollingNode::currentScrollPositionChanged(WebCore::ScrollType, WebCore::ScrollingLayerPositionAction) + 112 (ScrollingTreeScrollingNode.cpp:270) 24 com.apple.WebCore 0x0000000542ba8aa0 WebCore::ScrollingTreeFrameScrollingNodeMac::currentScrollPositionChanged(WebCore::ScrollType, WebCore::ScrollingLayerPositionAction) + 80 (ScrollingTreeFrameScrollingNodeMac.mm:156) 25 com.apple.WebCore 0x0000000545cfe00c WebCore::ScrollingTreeScrollingNode::scrollTo(WebCore::FloatPoint const&, WebCore::ScrollType, WebCore::ScrollClamping) + 556 (ScrollingTreeScrollingNode.cpp:262) 26 com.apple.WebCore 0x0000000542ba7ec3 WebCore::ScrollingTreeFrameScrollingNodeMac::commitStateAfterChildren(WebCore::ScrollingStateNode const&) + 131 (ScrollingTreeFrameScrollingNodeMac.mm:110) 27 com.apple.WebCore 0x0000000545ce32e5 WebCore::ScrollingTree::updateTreeFromStateNodeRecursive(WebCore::ScrollingStateNode const*, WebCore::CommitTreeState&) + 1941 (ScrollingTree.cpp:384) 28 com.apple.WebCore 0x0000000545ce2461 WebCore::ScrollingTree::commitTreeState(std::__1::unique_ptr<WebCore::ScrollingStateTree, std::__1::default_delete<WebCore::ScrollingStateTree> >&&) + 1649 (ScrollingTree.cpp:306) 29 com.apple.WebCore 0x000000054293e781 WebCore::ScrollingCoordinatorMac::commitTreeStateIfNeeded() + 369 (ScrollingCoordinatorMac.mm:120) 30 com.apple.WebCore 0x0000000545ca2a0c WebCore::AsyncScrollingCoordinator::requestScrollPositionUpdate(WebCore::ScrollableArea&, WebCore::IntPoint const&, WebCore::ScrollType, WebCore::ScrollClamping) + 860 (AsyncScrollingCoordinator.cpp:279) 31 com.apple.WebCore 0x0000000545ab9e0c WebCore::FrameView::requestScrollPositionUpdate(WebCore::IntPoint const&, WebCore::ScrollType, WebCore::ScrollClamping) + 668 (FrameView.cpp:2690) 32 com.apple.WebCore 0x0000000545d87002 WebCore::ScrollableArea::setScrollPositionFromAnimation(WebCore::IntPoint const&) + 82 (ScrollableArea.cpp:246) 33 com.apple.WebCore 0x0000000545d656d1 WebCore::ScrollAnimator::notifyPositionChanged(WebCore::FloatSize const&) + 257 (ScrollAnimator.cpp:283) 34 com.apple.WebCore 0x0000000541374bad WebCore::ScrollAnimatorMac::notifyPositionChanged(WebCore::FloatSize const&) + 77 (ScrollAnimatorMac.mm:893) 35 com.apple.WebCore 0x0000000545d642b9 WebCore::ScrollAnimator::scrollToPositionWithoutAnimation(WebCore::FloatPoint const&, WebCore::ScrollClamping) + 1209 (ScrollAnimator.cpp:139) 36 com.apple.WebCore 0x000000054137389b WebCore::ScrollAnimatorMac::scrollToPositionWithoutAnimation(WebCore::FloatPoint const&, WebCore::ScrollClamping) + 91 (ScrollAnimatorMac.mm:801) 37 com.apple.WebCore 0x0000000545d8614f WebCore::ScrollableArea::scrollToPositionWithoutAnimation(WebCore::FloatPoint const&, WebCore::ScrollClamping) + 79 (ScrollableArea.cpp:149) 38 com.apple.WebCore 0x0000000545d888e7 WebCore::ScrollableArea::updateScrollSnapState() + 551 (ScrollableArea.cpp:557) 39 com.apple.WebCore 0x000000054678f834 WebCore::RenderLayerModelObject::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) + 1556 (RenderLayerModelObject.cpp:202) 40 com.apple.WebCore 0x0000000546662e7e WebCore::RenderBox::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) + 254 (RenderBox.cpp:300) 41 com.apple.WebCore 0x00000005465d8fea WebCore::RenderBlock::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) + 42 (RenderBlock.cpp:438) 42 com.apple.WebCore 0x0000000546620982 WebCore::RenderBlockFlow::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) + 258 (RenderBlockFlow.cpp:2103) 43 com.apple.WebCore 0x00000005466df04c WebCore::RenderElement::setStyle(WebCore::RenderStyle&&, WebCore::StyleDifference) + 892 (RenderElement.cpp:527) 44 com.apple.WebCore 0x0000000546b9fe3d WebCore::RenderTreeUpdater::updateRendererStyle(WebCore::RenderElement&, WebCore::RenderStyle&&, WebCore::StyleDifference) + 269 (RenderTreeUpdater.cpp:299) 45 com.apple.WebCore 0x0000000546b9ea17 WebCore::RenderTreeUpdater::updateElementRenderer(WebCore::Element&, WebCore::Style::ElementUpdates const&) + 1239 46 com.apple.WebCore 0x0000000546b9deb2 WebCore::RenderTreeUpdater::updateRenderTree(WebCore::ContainerNode&) + 1170 (RenderTreeUpdater.cpp:194) 47 com.apple.WebCore 0x0000000546b9d520 WebCore::RenderTreeUpdater::commit(std::__1::unique_ptr<WebCore::Style::Update const, std::__1::default_delete<WebCore::Style::Update const> >) + 624 (RenderTreeUpdater.cpp:126) 48 com.apple.WebCore 0x0000000544899527 WebCore::Document::updateRenderTree(std::__1::unique_ptr<WebCore::Style::Update const, std::__1::default_delete<WebCore::Style::Update const> >) + 439 (Document.cpp:1996) 49 com.apple.WebCore 0x0000000544899d30 WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType) + 1792 (Document.cpp:2086) 50 com.apple.WebCore 0x000000054489b1d1 WebCore::Document::updateStyleIfNeeded() + 577 (Document.cpp:2178) 51 com.apple.WebCore 0x00000005448a45fd WebCore::Document::implicitClose() + 973 (Document.cpp:3183) <rdar://74275830>

Attachments
Test (729 bytes, text/html) 2021-05-18 18:13 PDT, Ryosuke Niwa	no flags	Details
Patch (10.66 KB, patch) 2021-05-26 06:01 PDT, Martin Robinson	no flags	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Ryosuke Niwa

Comment 1 2021-05-18 18:13:57 PDT

The issue here is that ScrollableArea::updateScrollSnapState which gets called by RenderLayerModelObject::styleDidChange is immediately trying to schedule a scroll. That's just wrong. This appears to be a regression from https://trac.webkit.org/changeset/185762.

Ryosuke Niwa

Comment 2 2021-05-18 18:14:18 PDT

I can reproduce this issue with ASAN release build of WebKitTestRunner at r277641.

Carlos Garcia Campos

Comment 3 2021-05-25 02:31:28 PDT

(In reply to Ryosuke Niwa from comment #1) > The issue here is that ScrollableArea::updateScrollSnapState which gets > called by RenderLayerModelObject::styleDidChange is immediately trying to > schedule a scroll. That's just wrong. > This appears to be a regression from > https://trac.webkit.org/changeset/185762. I'm not sure it's a regression of r185762, the problem is the call to scrollToPositionWithoutAnimation() that was there before r185762. Could it be that the problem is updating snap state from styleDidChange? Should we ensure it's done after style change/layout? That was introduced in r190330.

Martin Robinson

Comment 4 2021-05-25 06:25:57 PDT

I have been investigating this a bit. I think there are two straight-forward options here: 1. Changes to scroll-snap style could just trigger a relayout. This would be very easy to implement and I think it fixes a WPT test. 2. We could add a new StyleDifference for when it is necessary to recalculate the scroll position. Ryosuke and Simon, do you have a preference here?

Simon Fraser (smfr)

Comment 5 2021-05-25 10:08:06 PDT

Let's not introduce a new StyleDifference just to fix this. A relayout seems fine.

Martin Robinson

Comment 6 2021-05-26 06:01:21 PDT

Created attachment 429752 [details] Patch

EWS

Comment 7 2021-05-28 01:59:26 PDT

Committed r278193 (238236@main): <https://commits.webkit.org/238236@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 429752 [details].

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component Layout and Rendering

Assignee

Martin Robinson

Reported

2021-05-18 18:13 PDT

Modified

2021-05-29 14:29 PDT History

CC List

13 users Show

URL

Keywords InRadar

Depends on

Blocks