Created attachment 384174 [details] html for reproducing this bug run-webkit-tests with a randomly generated html. See attachment. 0 com.apple.WebCore 0x000000011ffae71c WebCore::RenderLayoutState::pageLogicalHeight() const + 12 1 com.apple.WebCore 0x00000001201b9302 WebCore::RenderTable::layout() + 4482 2 com.apple.WebCore 0x000000011ff06b3c WebCore::RenderElement::layoutIfNeeded() + 60 3 com.apple.WebCore 0x000000012009a24d WebCore::RenderGrid::performGridItemsPreLayout(WebCore::GridTrackSizingAlgorithm const&) const + 269 4 com.apple.WebCore 0x000000012009aa56 WebCore::RenderGrid::computeIntrinsicLogicalWidths(WebCore::LayoutUnit&, WebCore::LayoutUnit&) const + 182 5 com.apple.WebCore 0x000000011ffba2a3 WebCore::RenderBlock::computePreferredLogicalWidths() + 467 6 com.apple.WebCore 0x000000011ffe0b21 WebCore::RenderBox::maxPreferredLogicalWidth() const + 81 7 com.apple.WebCore 0x000000012016bbd9 WebCore::RenderMarquee::computePosition(WebCore::MarqueeDirection, bool) + 233 8 com.apple.WebCore 0x000000012016c226 WebCore::RenderMarquee::updateMarqueePosition() + 134 9 com.apple.WebCore 0x00000001200da528 WebCore::RenderLayer::updateLayerPositions(WebCore::RenderGeometryMap*, WTF::OptionSet<WebCore::RenderLayer::UpdateLayerPositionsFlag>) + 2184 10 com.apple.WebCore 0x00000001200da49b WebCore::RenderLayer::updateLayerPositions(WebCore::RenderGeometryMap*, WTF::OptionSet<WebCore::RenderLayer::UpdateLayerPositionsFlag>) + 2043 11 com.apple.WebCore 0x00000001200da49b WebCore::RenderLayer::updateLayerPositions(WebCore::RenderGeometryMap*, WTF::OptionSet<WebCore::RenderLayer::UpdateLayerPositionsFlag>) + 2043 12 com.apple.WebCore 0x00000001200da49b WebCore::RenderLayer::updateLayerPositions(WebCore::RenderGeometryMap*, WTF::OptionSet<WebCore::RenderLayer::UpdateLayerPositionsFlag>) + 2043 13 com.apple.WebCore 0x00000001200da7cf WebCore::RenderLayer::updateLayerPositionsAfterLayout(bool, bool) + 207 14 com.apple.WebCore 0x000000011f89d8eb WebCore::FrameView::didLayout(WTF::WeakPtr<WebCore::RenderElement>) + 139 15 com.apple.WebCore 0x000000011f8d7c00 WebCore::FrameViewLayoutContext::layout() + 2800 16 com.apple.WebCore 0x000000011f891f33 WebCore::FrameView::forceLayout(bool) + 99 17 com.apple.WebKitLegacy 0x000000010f50101e -[WebHTMLView layoutToMinimumPageWidth:height:originalPageWidth:originalPageHeight:maximumShrinkRatio:adjustingViewSize:] + 526 18 com.apple.WebKitLegacy 0x000000010f5010ba -[WebHTMLView layout] + 74 19 com.apple.WebKitLegacy 0x000000010f5ba6da -[WebDynamicScrollBarsView(WebInternal) updateScrollers] + 250 20 com.apple.WebKitLegacy 0x000000010f5bb343 -[WebDynamicScrollBarsView(WebInternal) reflectScrolledClipView:] + 227 21 com.apple.AppKit 0x00007fff325a1b1a __45-[NSClipView _reflectDocumentViewFrameChange]_block_invoke + 95 22 com.apple.AppKit 0x00007fff325a1912 -[NSClipView _reflectDocumentViewFrameChange] + 634 23 com.apple.AppKit 0x00007fff3253cccd -[NSView _postFrameChangeNotification] + 82 24 com.apple.AppKit 0x00007fff32536ac0 -[NSView setFrameSize:] + 3248 25 com.apple.WebCore 0x000000011e01516a WebCore::ScrollView::platformSetContentsSize() + 490 26 com.apple.WebCore 0x000000011faa3de0 WebCore::ScrollView::setContentsSize(WebCore::IntSize const&) + 112 27 com.apple.WebCore 0x000000011f898d0f WebCore::FrameView::setContentsSize(WebCore::IntSize const&) + 111 28 com.apple.WebCore 0x000000011f8921f9 WebCore::FrameView::adjustViewSize() + 697 29 com.apple.WebCore 0x000000011f8d7a79 WebCore::FrameViewLayoutContext::layout() + 2409 30 com.apple.WebCore 0x000000011f878d8c WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive() + 300 31 com.apple.WebCore 0x000000011f8fadfc WebCore::Page::layoutIfNeeded() + 60 32 com.apple.WebCore 0x000000011f8faeaa WebCore::Page::updateRendering() + 154 33 com.apple.WebKitLegacy 0x000000010f53deb6 -[WebView(WebPrivate) _viewWillDrawInternal] + 86 34 com.apple.WebKitLegacy 0x000000010f56553c LayerFlushController::flushLayers() + 76 35 com.apple.WebKitLegacy 0x000000010f653849 WebViewLayerFlushScheduler::layerFlushCallback() + 57 36 com.apple.WebKitLegacy 0x000000010f654c08 WebViewLayerFlushScheduler::WebViewLayerFlushScheduler(LayerFlushController*)::$_0::operator()() const + 24 37 com.apple.WebKitLegacy 0x000000010f654bc9 WTF::Detail::CallableWrapper<WebViewLayerFlushScheduler::WebViewLayerFlushScheduler(LayerFlushController*)::$_0, void>::call() + 25 38 com.apple.WebCore 0x000000011c65d87a WTF::Function<void ()>::operator()() const + 138 39 com.apple.WebCore 0x000000011fb2a66c WebCore::RunLoopObserver::runLoopObserverFired() + 140 40 com.apple.WebCore 0x000000011fb2a5d0 WebCore::RunLoopObserver::runLoopObserverFired(__CFRunLoopObserver*, unsigned long, void*) + 32 41 com.apple.CoreFoundation 0x00007fff35327d6c __CFRUNLOOP_IS_CALLING_OUT_TO_AN_OBSERVER_CALLBACK_FUNCTION__ + 23 42 com.apple.CoreFoundation 0x00007fff35327c9c __CFRunLoopDoObservers + 457 43 com.apple.CoreFoundation 0x00007fff352cb79b __CFRunLoopRun + 1179 44 com.apple.CoreFoundation 0x00007fff352cb083 CFRunLoopRunSpecific + 466 45 DumpRenderTree 0x000000010889357d runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) + 3405 (DumpRenderTree.mm:2103) 46 DumpRenderTree 0x0000000108891f24 dumpRenderTree(int, char const**) + 708 (DumpRenderTree.mm:1333) 47 DumpRenderTree 0x0000000108893f7d DumpRenderTreeMain(int, char const**) + 109 (DumpRenderTree.mm:1449) 48 DumpRenderTree 0x00000001089131b2 main + 34 (DumpRenderTreeMain.mm:34) 49 0x00007fff6d0622d5 start + 1
Created attachment 384181 [details] Patch
Comment on attachment 384181 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384181&action=review > Source/WebCore/ChangeLog:8 > + No new tests. No new functionality. We do need a test case for regression testing.
If this has an associated radar, please put it into a comment and into a ChangeLog. I found two with similar backtraces: rdar://problem/51861893&56686408
("it" being the radar URL, not other details)
[InRada]rdar://56624519
<rdar://problem/56624519>
Created attachment 384214 [details] Patch
Comment on attachment 384214 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384214&action=review > Source/WebCore/ChangeLog:8 > + Reviewed by NOBODY (OOPS!). > + > + Test: fast/table/crash-empty-layoutStateStack.html Some words here about what you changed, please. > LayoutTests/fast/table/crash-empty-layoutStateStack.html:16 > + #htmlvar00007, .class2, #htmlvar00004, #htmlvar00002 { mix-blend-mode: normal; box-reflect: right; pointer-events: bounding-box; -ms-font-feature-settings: 'onum' 1; grid-area: a; -webkit-columns: auto; -webkit-animation-direction: normal, alternate; -webkit-column-break-after: always; -webkit-backface-visibility: hidden; -webkit-padding-end: 8px; margin-bottom: 1vmax; background-attachment: local; background-repeat: round; border-image-width: 7 0 29 0; grid-auto-columns: min-content; box-decoration-break: clone; -webkit-align-content: flex-end; vector-effect: non-scaling-stroke; display: inline-grid; -webkit-column-span: all } > + #htmlvar00005 { mso-background-source: auto; align-content: flex-end safe; margin: 1px; columns: 0px; -webkit-logical-height: 0px; font-variant-caps: petite-caps; break-before: column; -webkit-margin-end: -1px; scroll-snap-points-x: inherit; -webkit-border-after-color: ; touch-action: none pan-left; writing-mode: tb-rl; animation-timing-function: steps(-1, start); justify-content: space-around center; -webkit-padding-before: 1px; text-underline: single; outline-style: ridge; user-select: none; grid-column-gap: 1 1 -1; counter-reset: c } > + </style> > + <marquee id="htmlvar00004" tabindex="1" style="-webkit-border-after: 10px dashed ; border-color: red; content: 'foo'; border-top-color: initial; border-bottom-style: groove" loop="9" truespeed="true" tabindex="0" checked="checked" colspan="-1" datetime="2000-02-01T03:04:05Z" select=".class4" formnovalidate="formnovalidate"> > + <table id="htmlvar00005" rules="rows" summary="x)+Pi?pva)%z" style="transform-origin: inherit; -webkit-min-logical-height: -1px; background-blend-mode: color-burn, normal; flex-shrink: 0.960659405674; -webkit-line-break: normal" layout="auto" border="1" framespacing="0" height="0" wrap="soft" type="text/css" reversed="reversed"> > + <body> > + <div class="tableAfter"></div> > + <div>Bug <a href="https://bugs.webkit.org/show_bug.cgi?id=204527">204527</a>: Crashes in RenderTable when layoutStateStack is empty</div> > + <div>This test passes if it does not CRASH.</div> We should be able to reduce this some more, right?
Wait a minute, why are we calling into RenderTable::layout inside RenderLayer::updateLayerPositionsAfterLayout? That doesn't seem right.
(In reply to Ryosuke Niwa from comment #9) > Wait a minute, why are we calling into RenderTable::layout inside > RenderLayer::updateLayerPositionsAfterLayout? That doesn't seem right. It looks like the grid believes we are pre-layout (meaning that we are in the preferred width computation phase). Either some hasDirtyPreferredWidth bits got lost or the grid was not laid out the first place (was not dirty) and we are laying it out now. Anyway that's why we need a test reduction so that we can debug this in a comprehensible context.
The real issue is that RenderMarquee::updateMarqueePosition() can trigger layout.
(In reply to zalan from comment #10) > (In reply to Ryosuke Niwa from comment #9) > > Wait a minute, why are we calling into RenderTable::layout inside > > RenderLayer::updateLayerPositionsAfterLayout? That doesn't seem right. > > It looks like the grid believes we are pre-layout (meaning that we are in > the preferred width computation phase). Either some hasDirtyPreferredWidth > bits got lost or the grid was not laid out the first place (was not dirty) > and we are laying it out now. Anyway that's why we need a test reduction so > that we can debug this in a comprehensible context. Yeah, it appears that something is going south here regarding grid's preferred with computation. It shouldn't be dirty while we're positioning layers in updateLayerPositionsAfterLayout.
Created attachment 384226 [details] html that causes the crash The html is trimmed down to the minimum. Removing any property would not cause crash. Not sure if it still makes sense.
(In reply to Jack from comment #13) > Created attachment 384226 [details] > html that causes the crash > > The html is trimmed down to the minimum. Removing any property would not > cause crash. Not sure if it still makes sense. Very nice reduction!
Created attachment 384227 [details] Minimum reduction I've reduced a bit more to make the render tree analysis easy.
Created attachment 384228 [details] Test case2
(In reply to Ryosuke Niwa from comment #15) > Created attachment 384227 [details] > Minimum reduction > > I've reduced a bit more to make the render tree analysis easy. me too :)
Should be relatively simple to figure out what's going on.
omg this is just so bad. What happens here is that in order for RenderMarquee (which btw is not a renderer!) to position the "scrolling" content it needs to know 1. how much space there is to scroll (this is the associated renderer's content box width) 2. how wide the content is. <body><div>foobar</div></body> -> div's content box width is the viewport width (ignore paddings margins on body/html for now) let's say: 800px while the content width [foobar] is 50px. Now RenderMarquee has 800px-50px space to scroll the content. Since at this point we are in post-layout phase, we should be able to answer to both questions, but bad_move#1: RenderMarquee calls maxPreferredLogicalWidth() to figure out the content width which is totally bogus (regressed 8 years ago at r105772). maxPreferredLogicalWidth (as the name implies) tells you how much space the content needs when no constrains applied. While this mostly works, it's incorrect nevertheless. It should instead try to figure out that _actual_ content width. bad_move#2: RenderGrid calls into layout while computing the preferred width. While preferred width computation is conceptually very similar to layout (measure and position), it should never call into layout() as the computed preferred width value is a prerequisite to layout. While I assume RenderGrid code deals with this circular dependency in most cases, it's incorrect from correctness pov. It should instead try to call the preferred width compute functions on the grid items.
^^ too dense What happens here is that in order for RenderMarquee (which btw is not a renderer!) to position the "scrolling" content it needs to know 2 things: 1. how much space there is to scroll (this is the associated renderer's content box width) 2. how wide the content is. simple example: <body><div>foobar</div></body> 1. <div>'s content box width is the viewport width (minus paddings margins on body/html etc) let's say 800px 2. the _content_ width [foobar] is, let's say 50px. -> RenderMarquee has (800-50)px space to scroll the content. Since at this point we are in post-layout phase, we should be able to answer to both questions (the content and the _content_box_ width), but bad_move#1: RenderMarquee calls maxPreferredLogicalWidth() to compute the content width (regressed 8 years ago at r105772). maxPreferredLogicalWidth (as the name implies) tells you how much space the content needs when no constrains applied. While this mostly works, it's incorrect nevertheless. It should instead try to figure out what _actual_ content width is. bad_move#2: RenderGrid calls into layout while computing the preferred width. While preferred width computation is conceptually very similar to layout (measure and position), it should never call into layout() as the computed preferred width value is a prerequisite to layout. While I assume RenderGrid code deals with this circular dependency in most cases, it's incorrect from correctness pov. It should instead try to call the preferred width compute functions on the grid items.
I talked with Geoff in person a bit but I think we can add a null check for now, and file a new bug to track the design issue.
Created attachment 384862 [details] Patch
Created attachment 384863 [details] Patch
Comment on attachment 384863 [details] Patch Nice!
Comment on attachment 384863 [details] Patch Clearing flags on attachment: 384863 Committed r253139: <https://trac.webkit.org/changeset/253139>
All reviewed patches have been landed. Closing bug.