Bug 206917

Summary:

Crash in RenderTreeBuilder::Table::findOrCreateParentForChild with multicol spanner

Product:

WebKit

Reporter:

Doug Kelly <dougk>

Component:

Layout and Rendering

Assignee:

zalan <zalan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, commit-queue, esprehn+autocc, ews-watchlist, glenn, kondapallykalyan, pdr, simon.fraser, webkit-bug-importer, zalan

Priority:

Keywords:

InRadar

Version:

WebKit Local Build

Hardware:

Mac

OS:

macOS 10.15

Attachments:

Description	Flags
Patch	none
Patch	zalan: review+
Patch	none
Patch	none

Doug Kelly

Reported 2020-01-28 17:55:48 PST

WebKit crashes with a null pointer deref in RenderTreeBuilder::Table::findOrCreateParentForChild() when a multicol spanner is the sibling element where we're trying to insert a new element. <rdar://problem/56686036>

Attachments
Patch (6.37 KB, patch) 2020-01-28 18:01 PST, Doug Kelly	no flags	Details Formatted Diff Diff
Patch (5.83 KB, patch) 2020-01-30 18:15 PST, Doug Kelly	zalan: review+	Details Formatted Diff Diff
Patch (6.63 KB, patch) 2020-02-01 10:04 PST, zalan	no flags	Details Formatted Diff Diff
Patch (6.56 KB, patch) 2020-02-07 16:55 PST, Doug Kelly	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Doug Kelly

Comment 1 2020-01-28 18:01:25 PST

Created attachment 389097 [details] Patch

zalan

Comment 2 2020-01-30 08:05:31 PST

I think a more correct fix would adjust the beforeChild in case of a spanner so that the rest of the function would just operate on this new candidate position. diff --git a/Source/WebCore/rendering/updating/RenderTreeBuilderTable.cpp b/Source/WebCore/rendering/updating/RenderTreeBuilderTable.cpp index 18dd26c7b37..ab67975f442 100644 --- a/Source/WebCore/rendering/updating/RenderTreeBuilderTable.cpp +++ b/Source/WebCore/rendering/updating/RenderTreeBuilderTable.cpp @@ -89,6 +89,22 @@ RenderElement& RenderTreeBuilder::Table::findOrCreateParentForChild(RenderTableS if (is<RenderTableRow>(child)) return parent; + if (is<RenderBox>(beforeChild)) { + // Adjust the beforeChild if it happens to be a spanner and the its actual location is somewhere else in the render tree. + auto& beforeChildBox = downcast<RenderBox>(*beforeChild); + if (auto* enclosingFragmentedFlow = parent.enclosingFragmentedFlow()) { + auto columnSpannerPlaceholderForBeforeChild = [&]() -> RenderObject* { + if (!is<RenderMultiColumnFlow>(enclosingFragmentedFlow)) + return nullptr; + auto& multiColumnFlow = downcast<RenderMultiColumnFlow>(*enclosingFragmentedFlow); + return multiColumnFlow.findColumnSpannerPlaceholder(&beforeChildBox); + }; + + if (auto* spannerPlaceholder = columnSpannerPlaceholderForBeforeChild()) + beforeChild = spannerPlaceholder; + } + } + auto* lastChild = beforeChild ? beforeChild : parent.lastRow(); if (is<RenderTableRow>(lastChild) && lastChild->isAnonymous() && !lastChild->isBeforeOrAfterContent()) { if (beforeChild == lastChild) and here is a bit simpler test case. <style> body { display: table-header-group; overflow-y: -webkit-paged-x; } div { column-span: all; } </style> <body><span id=span></span><div></div><script> document.body.offsetHeight; span.outerText = "remove"; document.body.innerText = "This test verifies that adding an element which is a sibling to a multicol spanner finds the correct table row. Test passes if WebKit does not crash. PASS"; if (window.testRunner) testRunner.dumpAsText(); </script>

Doug Kelly

Comment 3 2020-01-30 14:27:33 PST

(In reply to zalan from comment #2) > I think a more correct fix would adjust the beforeChild in case of a spanner > so that the rest of the function would just operate on this new candidate > position. > diff --git a/Source/WebCore/rendering/updating/RenderTreeBuilderTable.cpp > b/Source/WebCore/rendering/updating/RenderTreeBuilderTable.cpp > index 18dd26c7b37..ab67975f442 100644 > --- a/Source/WebCore/rendering/updating/RenderTreeBuilderTable.cpp > +++ b/Source/WebCore/rendering/updating/RenderTreeBuilderTable.cpp > @@ -89,6 +89,22 @@ RenderElement& > RenderTreeBuilder::Table::findOrCreateParentForChild(RenderTableS > if (is<RenderTableRow>(child)) > return parent; > > + if (is<RenderBox>(beforeChild)) { > + // Adjust the beforeChild if it happens to be a spanner and the its > actual location is somewhere else in the render tree. > + auto& beforeChildBox = downcast<RenderBox>(*beforeChild); > + if (auto* enclosingFragmentedFlow = > parent.enclosingFragmentedFlow()) { > + auto columnSpannerPlaceholderForBeforeChild = [&]() -> > RenderObject* { > + if (!is<RenderMultiColumnFlow>(enclosingFragmentedFlow)) > + return nullptr; > + auto& multiColumnFlow = > downcast<RenderMultiColumnFlow>(*enclosingFragmentedFlow); > + return > multiColumnFlow.findColumnSpannerPlaceholder(&beforeChildBox); > + }; > + > + if (auto* spannerPlaceholder = > columnSpannerPlaceholderForBeforeChild()) > + beforeChild = spannerPlaceholder; > + } > + } > + > auto* lastChild = beforeChild ? beforeChild : parent.lastRow(); > if (is<RenderTableRow>(lastChild) && lastChild->isAnonymous() && > !lastChild->isBeforeOrAfterContent()) { > if (beforeChild == lastChild) > Seems fine. This does have a side effect of changing beforeChild (which is passed by reference), but I see this is already done elsewhere in this function. > and here is a bit simpler test case. > > <style> > body { > display: table-header-group; > overflow-y: -webkit-paged-x; > } > div { > column-span: all; > } > </style> > <body><span id=span></span><div></div><script> > document.body.offsetHeight; > span.outerText = "remove"; > document.body.innerText = "This test verifies that adding an element which > is a sibling to a multicol spanner finds the correct table row. Test passes > if WebKit does not crash. PASS"; > if (window.testRunner) > testRunner.dumpAsText(); > </script> In my testing, this doesn't trigger the same codepath as the original test case...

zalan

Comment 4 2020-01-30 16:00:42 PST

> > and here is a bit simpler test case. > > > > <style> > > body { > > display: table-header-group; > > overflow-y: -webkit-paged-x; > > } > > div { > > column-span: all; > > } > > </style> > > <body><span id=span></span><div></div><script> > > document.body.offsetHeight; > > span.outerText = "remove"; > > document.body.innerText = "This test verifies that adding an element which > > is a sibling to a multicol spanner finds the correct table row. Test passes > > if WebKit does not crash. PASS"; > > if (window.testRunner) > > testRunner.dumpAsText(); > > </script> > > In my testing, this doesn't trigger the same codepath as the original test > case... It looks like the unrelated "document.body.innerText = " has a side effect. We need to force a style recalc/layout first by calling document.body.offsetHeight; This should do: document.body.offsetHeight; span.outerText = "remove"; document.body.offsetHeight; document.body.innerText = "This test verifies that adding an element which is a sibling to a multicol spanner finds the correct table row. Test passes if WebKit does not crash. PASS"; if (window.testRunner) testRunner.dumpAsText();

Doug Kelly

Comment 5 2020-01-30 16:37:39 PST

Yep, the second relayout fixes the issue. :)

Doug Kelly

Comment 6 2020-01-30 18:15:23 PST

Created attachment 389323 [details] Patch

zalan

Comment 7 2020-01-31 09:54:20 PST

Comment on attachment 389323 [details] Patch I still need to do a final check on the incoming beforeChild, before landing this. Will take care of the cq+.

zalan

Comment 8 2020-02-01 09:04:46 PST

Comment on attachment 389323 [details] Patch I figured we could actually be a bit more generic here by moving this beforeChild adjustment to the caller to cover other, non table cases as well.

zalan

Comment 9 2020-02-01 10:04:36 PST

Created attachment 389463 [details] Patch

zalan

Comment 10 2020-02-01 10:06:48 PST

(In reply to zalan from comment #9) > Created attachment 389463 [details] > Patch I'd do something like this^^. If there's any fallout, we should be able to address them quickly (or just go back to the less generic fix).

Doug Kelly

Comment 11 2020-02-05 13:42:14 PST

(In reply to zalan from comment #10) > (In reply to zalan from comment #9) > > Created attachment 389463 [details] > > Patch > I'd do something like this^^. If there's any fallout, we should be able to > address them quickly (or just go back to the less generic fix). Looks good to me. Should this be marked for review? (I don't see any reason for me to re-upload the same change.)

Doug Kelly

Comment 12 2020-02-07 16:55:24 PST

Created attachment 390150 [details] Patch

WebKit Commit Bot

Comment 13 2020-02-08 06:10:30 PST

Comment on attachment 390150 [details] Patch Clearing flags on attachment: 390150 Committed r256089: <https://trac.webkit.org/changeset/256089>

WebKit Commit Bot

Comment 14 2020-02-08 06:10:32 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.