Bug 218575 - Debug assert failure in RenderTable::layout()
Summary: Debug assert failure in RenderTable::layout()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Printing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-11-04 11:18 PST by Ryosuke Niwa
Modified: 2021-03-23 01:56 PDT (History)
13 users (show)

See Also:


Attachments
Reduced test case (239 bytes, text/html)
2020-11-04 11:18 PST, Ryosuke Niwa
no flags Details
Reduce test case with table cells satisfying !isBaselineAligned() (285 bytes, text/html)
2021-02-04 05:45 PST, Frédéric Wang (:fredw)
no flags Details
Patch (3.48 KB, patch)
2021-02-25 03:08 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch to add test (1.66 KB, patch)
2021-03-22 05:31 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2020-11-04 11:18:14 PST
e.g.

./rendering/RenderTable.cpp(586) : virtual void WebCore::RenderTable::layout()
1   0x3ccc632a9 WTFCrash
2   0x3ae461b8b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x3b2b51640 WebCore::RenderTable::layout()
4   0x3b287396c WebCore::RenderElement::layoutIfNeeded()
5   0x3b2b515e7 WebCore::RenderTable::layout()
6   0x3b293ede2 WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
7   0x3b293d3a4 WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&)
8   0x3b293c186 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)
9   0x3b291ee99 WebCore::RenderBlock::layout()
10  0x3b293ede2 WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
11  0x3b293d3a4 WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&)
12  0x3b293c186 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)
13  0x3b291ee99 WebCore::RenderBlock::layout()
14  0x3b293ede2 WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
15  0x3b293d3a4 WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&)
16  0x3b293c186 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)
17  0x3b291ee99 WebCore::RenderBlock::layout()
18  0x3b2bbdb93 WebCore::RenderView::layout()
19  0x3b2141181 WebCore::FrameViewLayoutContext::layout()
20  0x3b213d683 WebCore::FrameView::forceLayout(bool)
21  0x3b213d0d3 WebCore::FrameView::forceLayoutForPagination(WebCore::FloatSize const&, WebCore::FloatSize const&, float, WebCore::AdjustViewSizeOrNot)
22  0x3b213cc61 WebCore::Frame::setPrinting(bool, WebCore::FloatSize const&, WebCore::FloatSize const&, float, WebCore::AdjustViewSizeOrNot)
23  0x3b224862c WebCore::PrintContext::begin(float, float)
24  0x3a1bada4d WebKit::WebPage::beginPrinting(WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, WebKit::PrintInfo const&)
25  0x3a1c5f31c void IPC::callMemberFunctionImpl<WebKit::WebPage, void (WebKit::WebPage::*)(WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, WebKit::PrintInfo const&), std::__1::tuple<WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, WebKit::PrintInfo>, 0ul, 1ul>(WebKit::WebPage*, void (WebKit::WebPage::*)(WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, WebKit::PrintInfo const&), std::__1::tuple<WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, WebKit::PrintInfo>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul>)
26  0x3a1c5e2b0 void IPC::callMemberFunction<WebKit::WebPage, void (WebKit::WebPage::*)(WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, WebKit::PrintInfo const&), std::__1::tuple<WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, WebKit::PrintInfo>, std::__1::integer_sequence<unsigned long, 0ul, 1ul> >(std::__1::tuple<WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, WebKit::PrintInfo>&&, WebKit::WebPage*, void (WebKit::WebPage::*)(WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, WebKit::PrintInfo const&))
27  0x3a1c0f4b9 void IPC::handleMessage<Messages::WebPage::BeginPrinting, WebKit::WebPage, void (WebKit::WebPage::*)(WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, WebKit::PrintInfo const&)>(IPC::Decoder&, WebKit::WebPage*, void (WebKit::WebPage::*)(WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, WebKit::PrintInfo const&))
28  0x3a1c05756 WebKit::WebPage::didReceiveWebPageMessage(IPC::Connection&, IPC::Decoder&)
29  0x3a1bacd62 WebKit::WebPage::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
30  0x3a0463591 IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&)
31  0x3a16e4727 WebKit::WebProcess::didReceiveMessage(IPC::Connection&, IPC::Decoder&)

<rdar://problem/69563902>
Comment 1 Ryosuke Niwa 2020-11-04 11:18:33 PST
Created attachment 413187 [details]
Reduced test case
Comment 2 Sergio Villar Senin 2020-11-06 01:33:16 PST
(In reply to Ryosuke Niwa from comment #1)
> Created attachment 413187 [details]
> Reduced test case

Are we sure the reduced test case is correct?, it isn't hitting any assertion for me.
Comment 3 Ryosuke Niwa 2020-11-07 20:31:06 PST
(In reply to Sergio Villar Senin from comment #2)
> (In reply to Ryosuke Niwa from comment #1)
> > Created attachment 413187 [details]
> > Reduced test case
> 
> Are we sure the reduced test case is correct?, it isn't hitting any
> assertion for me.

Yes. Are you testing it on macOS or GTK+?
Comment 4 Sergio Villar Senin 2020-11-09 00:46:37 PST
(In reply to Ryosuke Niwa from comment #3)
> (In reply to Sergio Villar Senin from comment #2)
> > (In reply to Ryosuke Niwa from comment #1)
> > > Created attachment 413187 [details]
> > > Reduced test case
> > 
> > Are we sure the reduced test case is correct?, it isn't hitting any
> > assertion for me.
> 
> Yes. Are you testing it on macOS or GTK+?

I've tried with both GTK and WPE. I'll try with macOS.
Comment 5 Sergio Villar Senin 2020-11-11 01:40:15 PST
(In reply to Sergio Villar Senin from comment #4)
> (In reply to Ryosuke Niwa from comment #3)
> > (In reply to Sergio Villar Senin from comment #2)
> > > (In reply to Ryosuke Niwa from comment #1)
> > > > Created attachment 413187 [details]
> > > > Reduced test case
> > > 
> > > Are we sure the reduced test case is correct?, it isn't hitting any
> > > assertion for me.
> > 
> > Yes. Are you testing it on macOS or GTK+?
> 
> I've tried with both GTK and WPE. I'll try with macOS.

Nevermind. I was testing with MiniBrowser and it was working fine, but it looks like it reliably crashes with RWT.
Comment 6 Ryosuke Niwa 2020-11-11 19:02:50 PST
(In reply to Sergio Villar Senin from comment #5)
> (In reply to Sergio Villar Senin from comment #4)
> > (In reply to Ryosuke Niwa from comment #3)
> > > (In reply to Sergio Villar Senin from comment #2)
> > > > (In reply to Ryosuke Niwa from comment #1)
> > > > > Created attachment 413187 [details]
> > > > > Reduced test case
> > > > 
> > > > Are we sure the reduced test case is correct?, it isn't hitting any
> > > > assertion for me.
> > > 
> > > Yes. Are you testing it on macOS or GTK+?
> > 
> > I've tried with both GTK and WPE. I'll try with macOS.
> 
> Nevermind. I was testing with MiniBrowser and it was working fine, but it
> looks like it reliably crashes with RWT.

Ah, makes sense. Yeah, most of these fuzzer bugs require WKTR to reproduce.
Comment 7 Rob Buis 2020-11-12 12:49:21 PST
I do see the crash on OS X:
SHOULD NEVER BE REACHED
./rendering/RenderTable.cpp(586) : virtual void WebCore::RenderTable::layout()
1   0x558117309 WTFCrash
2   0x51d275c40 canLoad_libAccessibility__AXSIsolatedTreeMode
3   0x528fa2613 WebCore::RenderTable::layout()
4   0x528624ed0 WebCore::RenderElement::layoutIfNeeded()
etc...
Comment 8 Frédéric Wang (:fredw) 2021-02-02 07:09:05 PST
So apparently this is an instance of bug 174412, we are basically hitting the ASSERT added in bug 174413 to workaround infinite recursion. The patch uploaded to bug 174412 was landed in bug 176219. Would you be able to provide access to bug 176219, so that we have the full context on this issue?
Comment 9 Ryosuke Niwa 2021-02-02 10:18:24 PST
(In reply to Frédéric Wang (:fredw) from comment #8)
> So apparently this is an instance of bug 174412, we are basically hitting
> the ASSERT added in bug 174413 to workaround infinite recursion. The patch
> uploaded to bug 174412 was landed in bug 176219. Would you be able to
> provide access to bug 176219, so that we have the full context on this issue?

Done.
Comment 10 Frédéric Wang (:fredw) 2021-02-04 05:45:17 PST
Created attachment 419275 [details]
Reduce test case with table cells satisfying !isBaselineAligned()

This is the same as attachment 413187 [details], but with table cells satisfying !isBaselineAligned() so that layoutBlock happens only once in RenderTableCell::layout(). It still allows to reproduce the issue.
Comment 11 Frédéric Wang (:fredw) 2021-02-04 06:19:08 PST
(In reply to Ryosuke Niwa from comment #9)
> (In reply to Frédéric Wang (:fredw) from comment #8)
> > So apparently this is an instance of bug 174412, we are basically hitting
> > the ASSERT added in bug 174413 to workaround infinite recursion. The patch
> > uploaded to bug 174412 was landed in bug 176219. Would you be able to
> > provide access to bug 176219, so that we have the full context on this issue?
> 
> Done.

Thanks. Bug 176219 does not seem to have much info either. Anyway, for the record this is what I've found so far for attachment 419275 [details]. Any advice would be welcome:

When print() is called, WebCore::RenderTable::layout() is executed in paginated mode. It performs layout of each section at

https://webkit-search.igalia.com/webkit/rev/5edc52e6a4c801295ad3cbf56569dd4a5b738f78/Source/WebCore/rendering/RenderTable.cpp#476

which ends up laying out the cell's section and setting its height at

https://webkit-search.igalia.com/webkit/rev/5edc52e6a4c801295ad3cbf56569dd4a5b738f78/Source/WebCore/rendering/ComplexLineLayout.cpp#1955

The sections are then positioned at

https://webkit-search.igalia.com/webkit/rev/5edc52e6a4c801295ad3cbf56569dd4a5b738f78/Source/WebCore/rendering/RenderTable.cpp#476

Since the table was previously laid out in non-paginated mode, this detects a section move and we arrive at the code I mentioned in comment 8:

https://webkit-search.igalia.com/webkit/rev/5edc52e6a4c801295ad3cbf56569dd4a5b738f78/Source/WebCore/rendering/RenderTable.cpp#586

Then WebCore::RenderTable::layout() is executed a second time. A difference happens in ComplexLineLayout::determineStartPosition() : for the previous layout paginationDelta was 0 and the line's paginationStrut had been set accordingly ; for this new layout, the line's paginationStrut remains nonzero and so the calculated paginationDelta is no longer 0 :

https://webkit-search.igalia.com/webkit/rev/5edc52e6a4c801295ad3cbf56569dd4a5b738f78/Source/WebCore/rendering/ComplexLineLayout.cpp#1855

The adjustBlockDirectionPosition call below then modifies the line's lineBoxBottom which at the end changes the calculated cell's height.

Finally the sections are positioned again, a section move detected (due to at least one cell's height change) and the assertion failure triggered.

I'm not familiar enough with the pagination code to really understand what this code is really doing. However, I see there are pagination-related FIXMEs all over the code, for example these ones (which don't seem to be involved in the current problem though):

https://webkit-search.igalia.com/webkit/rev/5edc52e6a4c801295ad3cbf56569dd4a5b738f78/Source/WebCore/rendering/RenderTableSection.cpp#593

Additionally, I modified the tests to use 100 sections and noticed that the RenderTable layout does stabilize after ~50 recursions. For completeness, I can also mention that after commenting out the adjustBlockDirectionPosition call, the layout is similar to non-paginated mode and does not recurse/assert.

NB: Similar behavior happens for attachment 413187 [details], but it is a slightly more difficult to debug because RenderTableCell::layout() can lay out its content twice.
Comment 12 Carlos Garcia Campos 2021-02-25 02:36:16 PST
The problem here is that moving one section affects the table layout, so in the next layout iteration, another section moves. I think this is valid case, it's really possible to recurse several times. The recursion check was introduced in r219394, but there's no much information nor a test case, so I don't know how this can end up in an infinite recursion. Instead of limiting to one recursion, we could allow to recurse for the amount of sections in the table?
Comment 13 Carlos Garcia Campos 2021-02-25 03:08:15 PST
Created attachment 421513 [details]
Patch
Comment 14 Carlos Garcia Campos 2021-03-15 03:10:38 PDT
Ping reviewers.
Comment 15 EWS 2021-03-18 02:30:01 PDT
Committed r274627: <https://commits.webkit.org/r274627>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421513 [details].
Comment 16 Ryosuke Niwa 2021-03-18 10:55:41 PDT
Is this a security bug?
Comment 17 Ryosuke Niwa 2021-03-18 12:11:24 PDT
If it's not a security bug, can we also add a test?
Comment 18 Carlos Garcia Campos 2021-03-22 02:32:30 PDT
It doesn't look like a security issue to me, I'll submit a test.
Comment 19 Carlos Garcia Campos 2021-03-22 05:26:17 PDT
Reopening to attach a new patch.
Comment 20 Carlos Garcia Campos 2021-03-22 05:31:04 PDT
Created attachment 423873 [details]
Patch to add test
Comment 21 EWS 2021-03-23 01:56:38 PDT
Committed r274854: <https://commits.webkit.org/r274854>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423873 [details].