Bug 210278 – [ macOS debug wk2 ] REGRESSION(r259761): ASSERTION FAILED: !needsLayout() on fast/events/scroll-subframe-in-rendering-update.html

• Component: Scrolling
• Status: RESOLVED FIXED

[Jacob Uphoff]

fast/events/scroll-subframe-in-rendering-update.html

This test started to flaky crash with an assertion failure and first failure was seen on r259732 for macOS debug wk2.

History:

https://results.webkit.org/?suite=layout-tests&test=fast%2Fevents%2Fscroll-subframe-in-rendering-update.html&platform=mac&style=debug&flavor=wk2

Log:

No crash log found for com.apple.WebKit.WebContent.Development:16610.

stdout:

stderr:
ASSERTION FAILED: !needsLayout()
./page/FrameView.cpp(4276) : virtual void WebCore::FrameView::paintContents(WebCore::GraphicsContext &, const WebCore::IntRect &, WebCore::Widget::SecurityOriginPaintPolicy, WebCore::EventRegionContext *)
1   0x27e312cf9 WTFCrash
2   0x260fe0c6b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x2646da016 WebCore::FrameView::paintContents(WebCore::GraphicsContext&, WebCore::IntRect const&, WebCore::Widget::SecurityOriginPaintPolicy, WebCore::EventRegionContext*)
4   0x2649522b4 WebCore::ScrollView::paint(WebCore::GraphicsContext&, WebCore::IntRect const&, WebCore::Widget::SecurityOriginPaintPolicy, WebCore::EventRegionContext*)
5   0x2651dc56b WebCore::RenderWidget::paintContents(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
6   0x2651dce27 WebCore::RenderWidget::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
7   0x265061cd4 WebCore::RenderLayer::collectEventRegionForFragments(WTF::Vector<WebCore::LayerFragment, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&, WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&)
8   0x26505dedf WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>)
9   0x2650a9119 WebCore::RenderLayerBacking::paintIntoLayer(WebCore::GraphicsLayer const*, WebCore::GraphicsContext&, WebCore::IntRect const&, WTF::OptionSet<WebCore::PaintBehavior>, WebCore::EventRegionContext*)::$_5::operator()(WebCore::RenderLayer&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>) const
10  0x2650a89d2 WebCore::RenderLayerBacking::paintIntoLayer(WebCore::GraphicsLayer const*, WebCore::GraphicsContext&, WebCore::IntRect const&, WTF::OptionSet<WebCore::PaintBehavior>, WebCore::EventRegionContext*)
11  0x2650a5da0 WebCore::RenderLayerBacking::updateEventRegion()::$_2::operator()(WebCore::GraphicsLayer&) const
12  0x2650a59c4 WebCore::RenderLayerBacking::updateEventRegion()
13  0x2650b3147 WebCore::RenderLayerCompositor::updateBackingAndHierarchy(WebCore::RenderLayer&, WTF::Vector<WTF::Ref<WebCore::GraphicsLayer, WTF::DumbPtrTraits<WebCore::GraphicsLayer> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::RenderLayerCompositor::UpdateBackingTraversalState&, WebCore::ScrollingTreeState&, WTF::OptionSet<WebCore::RenderLayerCompositor::UpdateLevel>)
14  0x2650b354e WebCore::RenderLayerCompositor::updateBackingAndHierarchy(WebCore::RenderLayer&, WTF::Vector<WTF::Ref<WebCore::GraphicsLayer, WTF::DumbPtrTraits<WebCore::GraphicsLayer> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::RenderLayerCompositor::UpdateBackingTraversalState&, WebCore::ScrollingTreeState&, WTF::OptionSet<WebCore::RenderLayerCompositor::UpdateLevel>)
15  0x2650b3602 WebCore::RenderLayerCompositor::updateBackingAndHierarchy(WebCore::RenderLayer&, WTF::Vector<WTF::Ref<WebCore::GraphicsLayer, WTF::DumbPtrTraits<WebCore::GraphicsLayer> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::RenderLayerCompositor::UpdateBackingTraversalState&, WebCore::ScrollingTreeState&, WTF::OptionSet<WebCore::RenderLayerCompositor::UpdateLevel>)
16  0x2650b0950 WebCore::RenderLayerCompositor::updateCompositingLayers(WebCore::CompositingUpdateType, WebCore::RenderLayer*)
17  0x2646c90c3 WebCore::FrameView::updateCompositingLayersAfterLayout()
18  0x2646cb997 WebCore::FrameView::didLayout(WTF::WeakPtr<WebCore::RenderElement>)
19  0x2647111a7 WebCore::FrameViewLayoutContext::layout()
20  0x2646a5f25 WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive()
21  0x26473a3ae WebCore::Page::layoutIfNeeded()
22  0x26473a50a WebCore::Page::updateRendering()
23  0x105cc5686 WebKit::WebPage::updateRendering()
24  0x105706854 WebKit::TiledCoreAnimationDrawingArea::updateRendering(WebKit::TiledCoreAnimationDrawingArea::UpdateRenderingType)
25  0x10570b02d WebKit::TiledCoreAnimationDrawingArea::updateRenderingRunLoopCallback()
26  0x10571b318 WebKit::TiledCoreAnimationDrawingArea::TiledCoreAnimationDrawingArea(WebKit::WebPage&, WebKit::WebPageCreationParameters const&)::$_0::operator()() const
27  0x10571b2ce WTF::Detail::CallableWrapper<WebKit::TiledCoreAnimationDrawingArea::TiledCoreAnimationDrawingArea(WebKit::WebPage&, WebKit::WebPageCreationParameters const&)::$_0, void>::call()
28  0x260ff3612 WTF::Function<void ()>::operator()() const
29  0x2649e35c0 WebCore::RunLoopObserver::runLoopObserverFired()
30  0x2649e3520 WebCore::RunLoopObserver::runLoopObserverFired(__CFRunLoopObserver*, unsigned long, void*)
31  0x7fff349984f5 __CFRUNLOOP_IS_CALLING_OUT_TO_AN_OBSERVER_CALLBACK_FUNCTION__
LEAK: 2 WebPageProxy

[Radar WebKit Bug Importer]

<rdar://problem/61517389>

[Jacob Uphoff]

Reproduced with command: 'run-webkit-tests --iterations 2000 --exit-after-n-failures 1 --exit-after-n-crashes-or-timeouts 1 --debug-rwt-logging --no-retry --force --no-build -f --debug --root /Volumes/Data/tmp/MacDebug fast/events/scroll-subframe-in-rendering-update.html'
Found Failures using Iterations on Debug WK2

[Daniel Bates]

Oops. I checked the wrong view in RenderWidget::paint(). Need to check that m_widget does not need layout. Not the view() of the RenderWidget.

[Daniel Bates]

Created attachment 395962 [details]
Patch

[Ryan Haddad]

*** Bug 210244 has been marked as a duplicate of this bug. ***

[Darin Adler]

Comment on attachment 395962 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395962&action=review

> Source/WebCore/ChangeLog:10
> +        Only EventRegion paint the contents of the widget if the widget is a frame view that
> +        does not need layout.

This says what the patch does. But where’s the explanation of why?

Might even want the reason why as a comment in the source code.

Not arguing for something super-wordy, but the relevant question for comments is always "why".

[Daniel Bates]

Thanks for the review. I will add a why: you don't paint a widget before it layed out. Otherwise, you hit the assertion.

[Daniel Bates]

Created attachment 396001 [details]
To land

[Daniel Bates]

Committed r259821: <https://trac.webkit.org/changeset/259821>

[Simon Fraser (smfr)]

Comment on attachment 396001 [details]
To land

This isn't right. We should never be testing needsLayout() except to assert; the code simply shouldn't ever get here, and if it does, something is wrong.

[Darin Adler]

That makes sense to me. Sorry I didn’t think of that when reviewing.

Do you think we should roll out immediately, or remove this when adding a real fix?

[alan]

(In reply to Simon Fraser (smfr) from comment #10)
> Comment on attachment 396001 [details]
> To land
> 
> This isn't right. We should never be testing needsLayout() except to assert;
> the code simply shouldn't ever get here, and if it does, something is wrong.
on WK1 we even early return if the tree needs layout. This patch should be reverted.

[Truitt Savell]

*** Bug 210308 has been marked as a duplicate of this bug. ***

[Daniel Bates]

(In reply to zalan from comment #12)
> (In reply to Simon Fraser (smfr) from comment #10)
> > Comment on attachment 396001 [details]
> > To land
> > 
> > This isn't right. We should never be testing needsLayout() except to assert;
> > the code simply shouldn't ever get here, and if it does, something is wrong.
> on WK1 we even early return if the tree needs layout. This patch should be
> reverted.

Ok, I don't need the needsLayout bit. I can write in terms needsEventRegionUpdateForNonCompositedFrame()

[Truitt Savell]

*** Bug 210307 has been marked as a duplicate of this bug. ***

[Daniel Bates]

Re-opening to write in terms of needsEventRegionUpdateForNonCompositedFrame()

[alan]

this stacktrace indicates that
1. we either do not manage to clean the tree properly (we come back from layout with some dirty renderers)
2. someone marks some renderers dirty after layout but still within this stacktrace (essentially between layout and paint)

ASSERTION FAILED: !needsLayout()
./page/FrameView.cpp(4276) : virtual void WebCore::FrameView::paintContents(WebCore::GraphicsContext &, const WebCore::IntRect &, WebCore::Widget::SecurityOriginPaintPolicy, WebCore::EventRegionContext *)
1   0x27e312cf9 WTFCrash
2   0x260fe0c6b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x2646da016 WebCore::FrameView::paintContents(WebCore::GraphicsContext&, WebCore::IntRect const&, WebCore::Widget::SecurityOriginPaintPolicy, WebCore::EventRegionContext*)
4   0x2649522b4 WebCore::ScrollView::paint(WebCore::GraphicsContext&, WebCore::IntRect const&, WebCore::Widget::SecurityOriginPaintPolicy, WebCore::EventRegionContext*)
5   0x2651dc56b WebCore::RenderWidget::paintContents(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
6   0x2651dce27 WebCore::RenderWidget::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
7   0x265061cd4 WebCore::RenderLayer::collectEventRegionForFragments(WTF::Vector<WebCore::LayerFragment, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&, WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&)
8   0x26505dedf WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>)
9   0x2650a9119 WebCore::RenderLayerBacking::paintIntoLayer(WebCore::GraphicsLayer const*, WebCore::GraphicsContext&, WebCore::IntRect const&, WTF::OptionSet<WebCore::PaintBehavior>, WebCore::EventRegionContext*)::$_5::operator()(WebCore::RenderLayer&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>) const
10  0x2650a89d2 WebCore::RenderLayerBacking::paintIntoLayer(WebCore::GraphicsLayer const*, WebCore::GraphicsContext&, WebCore::IntRect const&, WTF::OptionSet<WebCore::PaintBehavior>, WebCore::EventRegionContext*)
11  0x2650a5da0 WebCore::RenderLayerBacking::updateEventRegion()::$_2::operator()(WebCore::GraphicsLayer&) const
12  0x2650a59c4 WebCore::RenderLayerBacking::updateEventRegion()
13  0x2650b3147 WebCore::RenderLayerCompositor::updateBackingAndHierarchy(WebCore::RenderLayer&, WTF::Vector<WTF::Ref<WebCore::GraphicsLayer, WTF::DumbPtrTraits<WebCore::GraphicsLayer> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::RenderLayerCompositor::UpdateBackingTraversalState&, WebCore::ScrollingTreeState&, WTF::OptionSet<WebCore::RenderLayerCompositor::UpdateLevel>)
14  0x2650b354e WebCore::RenderLayerCompositor::updateBackingAndHierarchy(WebCore::RenderLayer&, WTF::Vector<WTF::Ref<WebCore::GraphicsLayer, WTF::DumbPtrTraits<WebCore::GraphicsLayer> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::RenderLayerCompositor::UpdateBackingTraversalState&, WebCore::ScrollingTreeState&, WTF::OptionSet<WebCore::RenderLayerCompositor::UpdateLevel>)
15  0x2650b3602 WebCore::RenderLayerCompositor::updateBackingAndHierarchy(WebCore::RenderLayer&, WTF::Vector<WTF::Ref<WebCore::GraphicsLayer, WTF::DumbPtrTraits<WebCore::GraphicsLayer> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::RenderLayerCompositor::UpdateBackingTraversalState&, WebCore::ScrollingTreeState&, WTF::OptionSet<WebCore::RenderLayerCompositor::UpdateLevel>)
16  0x2650b0950 WebCore::RenderLayerCompositor::updateCompositingLayers(WebCore::CompositingUpdateType, WebCore::RenderLayer*)
17  0x2646c90c3 WebCore::FrameView::updateCompositingLayersAfterLayout()
18  0x2646cb997 WebCore::FrameView::didLayout(WTF::WeakPtr<WebCore::RenderElement>)

[alan]

oh crossing frame boundaries :(

[Simon Fraser (smfr)]

The issue here is that RenderLayerBacking::updateEventRegion() now crosses frame boundaries with the fake paint, but we're inside of updateCompositingLayers() on some document, with no guarantee that subframes have been laid out yet.

We need to move event region "painting" to some later time when all frames have been laid out, or we need event region painting to not cross frame boundaries, and have some solution for non-composited subframes.

[alan]

(In reply to Simon Fraser (smfr) from comment #19)
> The issue here is that RenderLayerBacking::updateEventRegion() now crosses
> frame boundaries with the fake paint, but we're inside of
> updateCompositingLayers() on some document, with no guarantee that subframes
> have been laid out yet.
> 
> We need to move event region "painting" to some later time when all frames
> have been laid out, or we need event region painting to not cross frame
> boundaries, and have some solution for non-composited subframes.
why does event region painting cross frame boundaries? Normal paint does not do it. What's so special about event regions in this context?

[Daniel Bates]

Committed r259839: <https://trac.webkit.org/changeset/259839>

[Daniel Bates]

(In reply to zalan from comment #20)
> (In reply to Simon Fraser (smfr) from comment #19)
> > The issue here is that RenderLayerBacking::updateEventRegion() now crosses
> > frame boundaries with the fake paint, but we're inside of
> > updateCompositingLayers() on some document, with no guarantee that subframes
> > have been laid out yet.
> > 
> > We need to move event region "painting" to some later time when all frames
> > have been laid out, or we need event region painting to not cross frame
> > boundaries, and have some solution for non-composited subframes.
> why does event region painting cross frame boundaries? Normal paint does not
> do it. What's so special about event regions in this context?

I explained this to you in a pm. To summarize here, event regions are stored on compositing layers only. So, when they are triggered there is no guarantee that all frames (including non-composited frames) have layed out. See bug #210311 for more details.