WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Patch
(2.16 KB, patch)
2019-11-22 11:40 PST
,
Jack
no flags
Details
Formatted Diff
Diff
Patch
(5.92 KB, patch)
2019-11-22 17:18 PST
,
Jack
no flags
Details
Formatted Diff
Diff
html that causes the crash
(550 bytes, text/plain)
2019-11-22 19:35 PST
,
Jack
no flags
Details
Minimum reduction
(223 bytes, text/html)
2019-11-22 19:40 PST
,
Ryosuke Niwa
no flags
Details
Test case2
(105 bytes, text/html)
2019-11-22 19:42 PST
,
zalan
no flags
Details
Patch
(2.28 KB, patch)
2019-12-04 16:44 PST
,
Jack
no flags
Details
Formatted Diff
Diff
Patch
(4.42 KB, patch)
2019-12-04 16:51 PST
,
Jack
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Jack
Comment 1
2019-11-22 11:40:00 PST
Created
attachment 384181
[details]
Patch
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
[InRada]
rdar://56624519
Ryosuke Niwa
Comment 6
2019-11-22 15:53:04 PST
<
rdar://problem/56624519
>
Jack
Comment 7
2019-11-22 17:18:59 PST
Created
attachment 384214
[details]
Patch
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
Created
attachment 384862
[details]
Patch
Jack
Comment 23
2019-12-04 16:51:33 PST
Created
attachment 384863
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug