Bug 218575 – Debug assert failure in RenderTable::layout()

• Status: RESOLVED
• Resolution: FIXED
• Component: Printing

[Ryosuke Niwa]

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>

[Ryosuke Niwa]

Created attachment 413187 [details]
Reduced test case

[Sergio Villar Senin]

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

[Ryosuke Niwa]

(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+?

[Sergio Villar Senin]

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

[Sergio Villar Senin]

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

[Ryosuke Niwa]

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

[Rob Buis]

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...

[Frédéric Wang (:fredw)]

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?

[Ryosuke Niwa]

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

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

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

[Carlos Garcia Campos]

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?

[Carlos Garcia Campos]

Created attachment 421513 [details]
Patch

[Carlos Garcia Campos]

Ping reviewers.

[EWS]

Committed r274627: <https://commits.webkit.org/r274627>

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

[Ryosuke Niwa]

Is this a security bug?

[Ryosuke Niwa]

If it's not a security bug, can we also add a test?

[Carlos Garcia Campos]

It doesn't look like a security issue to me, I'll submit a test.

[Carlos Garcia Campos]

Reopening to attach a new patch.

[Carlos Garcia Campos]

Created attachment 423873 [details]
Patch to add test

[EWS]

Committed r274854: <https://commits.webkit.org/r274854>

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