Bug 204527 – Nullptr crash in RenderLayoutState::pageLogicalHeight const via RenderGrid::computeIntrinsicLogicalWidths inside RenderMarquee::updateMarqueePosition

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Jack]

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

[Jack]

Created attachment 384181 [details]
Patch

[alan]

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]

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]

("it" being the radar URL, not other details)

[Jack]

[InRada]rdar://56624519

[Ryosuke Niwa]

<rdar://problem/56624519>

[Jack]

Created attachment 384214 [details]
Patch

[Simon Fraser (smfr)]

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]

Wait a minute, why are we calling into RenderTable::layout inside RenderLayer::updateLayerPositionsAfterLayout? That doesn't seem right.

[alan]

(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)]

The real issue is that RenderMarquee::updateMarqueePosition() can trigger layout.

[Ryosuke Niwa]

(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]

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]

(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]

Created attachment 384227 [details]
Minimum reduction

I've reduced a bit more to make the render tree analysis easy.

[alan]

Created attachment 384228 [details]
Test case2

[alan]

(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 :)

[alan]

Should be relatively simple to figure out what's going on.

[alan]

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.

[alan]

^^ 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]

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]

Created attachment 384862 [details]
Patch

[Jack]

Created attachment 384863 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 384863 [details]
Patch

Nice!

[WebKit Commit Bot]

Comment on attachment 384863 [details]
Patch

Clearing flags on attachment: 384863

Committed r253139: <https://trac.webkit.org/changeset/253139>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.