| Summary: | [ macOS debug wk2 ] REGRESSION(r259761): ASSERTION FAILED: !needsLayout() on fast/events/scroll-subframe-in-rendering-update.html | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jacob Uphoff <jacob_uphoff> | ||||||
| Component: | Scrolling | Assignee: | Daniel Bates <dbates> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | darin, dbates, esprehn+autocc, ews-watchlist, glenn, kondapallykalyan, pdr, ryanhaddad, simon.fraser, tsavell, webkit-bot-watchers-bugzilla, webkit-bug-importer, zalan | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=210244 https://bugs.webkit.org/show_bug.cgi?id=210311 |
||||||||
| Bug Depends on: | 210041 | ||||||||
| Bug Blocks: | |||||||||
| Attachments: |
|
||||||||
|
Description
Jacob Uphoff
2020-04-09 07:44:58 PDT
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 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. Created attachment 395962 [details]
Patch
*** Bug 210244 has been marked as a duplicate of this bug. *** 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". Thanks for the review. I will add a why: you don't paint a widget before it layed out. Otherwise, you hit the assertion. Created attachment 396001 [details]
To land
Committed r259821: <https://trac.webkit.org/changeset/259821> 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.
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? (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. *** Bug 210308 has been marked as a duplicate of this bug. *** (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() *** Bug 210307 has been marked as a duplicate of this bug. *** Re-opening to write in terms of needsEventRegionUpdateForNonCompositedFrame() 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>) oh crossing frame boundaries :( 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. (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? Committed r259839: <https://trac.webkit.org/changeset/259839> (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. |