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

Description Ryosuke Niwa 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>
Comment 1 Ryosuke Niwa 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.
Comment 2 Ryosuke Niwa 2021-05-18 18:14:18 PDT 
I can reproduce this issue with ASAN release build of WebKitTestRunner at r277641.
Comment 3 Carlos Garcia Campos 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.
Comment 4 Martin Robinson 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?
Comment 5 Simon Fraser (smfr) 2021-05-25 10:08:06 PDT 
Let's not introduce a new StyleDifference just to fix this. A relayout seems fine.
Comment 6 Martin Robinson 2021-05-26 06:01:21 PDT 
Created attachment 429752 [details]
Patch
Comment 7 EWS 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].