Bug 225950

Summary: Release assert in Document::updateStyleIfNeeded() via ScrollableArea::updateScrollSnapState
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: Layout and RenderingAssignee: Martin Robinson <mrobinson>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cgarcia, ews-feeder, fred.wang, gpoo, mrobinson, product-security, rbuis, simon.fraser, svillar, webkit-bug-importer, wenson_hsieh, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Test
none
Patch none

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
DetailsFormatted DiffDiff
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.