RESOLVED FIXED 204527
Nullptr crash in RenderLayoutState::pageLogicalHeight const via RenderGrid::computeIntrinsicLogicalWidths inside RenderMarquee::updateMarqueePosition
https://bugs.webkit.org/show_bug.cgi?id=204527
Summary Nullptr crash in RenderLayoutState::pageLogicalHeight const via RenderGrid::c...
Jack
Reported 2019-11-22 11:10:53 PST
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
Attachments
html for reproducing this bug (539.20 KB, text/html)
2019-11-22 11:10 PST, Jack
no flags
Patch (2.16 KB, patch)
2019-11-22 11:40 PST, Jack
no flags
Patch (5.92 KB, patch)
2019-11-22 17:18 PST, Jack
no flags
html that causes the crash (550 bytes, text/plain)
2019-11-22 19:35 PST, Jack
no flags
Minimum reduction (223 bytes, text/html)
2019-11-22 19:40 PST, Ryosuke Niwa
no flags
Test case2 (105 bytes, text/html)
2019-11-22 19:42 PST, zalan
no flags
Patch (2.28 KB, patch)
2019-12-04 16:44 PST, Jack
no flags
Patch (4.42 KB, patch)
2019-12-04 16:51 PST, Jack
no flags
Jack
Comment 1 2019-11-22 11:40:00 PST
zalan
Comment 2 2019-11-22 11:42:30 PST
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.
Alexey Proskuryakov
Comment 3 2019-11-22 15:14:05 PST
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
Alexey Proskuryakov
Comment 4 2019-11-22 15:15:01 PST
("it" being the radar URL, not other details)
Jack
Comment 5 2019-11-22 15:48:47 PST
Ryosuke Niwa
Comment 6 2019-11-22 15:53:04 PST
Jack
Comment 7 2019-11-22 17:18:59 PST
Simon Fraser (smfr)
Comment 8 2019-11-22 17:19:59 PST
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?
Ryosuke Niwa
Comment 9 2019-11-22 17:33:35 PST
Wait a minute, why are we calling into RenderTable::layout inside RenderLayer::updateLayerPositionsAfterLayout? That doesn't seem right.
zalan
Comment 10 2019-11-22 17:38:56 PST
(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.
Simon Fraser (smfr)
Comment 11 2019-11-22 17:40:45 PST
The real issue is that RenderMarquee::updateMarqueePosition() can trigger layout.
Ryosuke Niwa
Comment 12 2019-11-22 17:45:07 PST
(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.
Jack
Comment 13 2019-11-22 19:35:12 PST
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.
Ryosuke Niwa
Comment 14 2019-11-22 19:39:30 PST
(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!
Ryosuke Niwa
Comment 15 2019-11-22 19:40:24 PST
Created attachment 384227 [details] Minimum reduction I've reduced a bit more to make the render tree analysis easy.
zalan
Comment 16 2019-11-22 19:42:21 PST
Created attachment 384228 [details] Test case2
zalan
Comment 17 2019-11-22 19:43:03 PST
(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 :)
zalan
Comment 18 2019-11-22 19:44:34 PST
Should be relatively simple to figure out what's going on.
zalan
Comment 19 2019-11-23 06:43:50 PST
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.
zalan
Comment 20 2019-11-23 11:48:18 PST
^^ 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.
Ryosuke Niwa
Comment 21 2019-12-04 00:08:48 PST
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.
Jack
Comment 22 2019-12-04 16:44:13 PST
Jack
Comment 23 2019-12-04 16:51:33 PST
Ryosuke Niwa
Comment 24 2019-12-04 20:08:48 PST
Comment on attachment 384863 [details] Patch Nice!
WebKit Commit Bot
Comment 25 2019-12-04 20:29:10 PST
Comment on attachment 384863 [details] Patch Clearing flags on attachment: 384863 Committed r253139: <https://trac.webkit.org/changeset/253139>
WebKit Commit Bot
Comment 26 2019-12-04 20:29:12 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.