RESOLVED FIXED 206917
Crash in RenderTreeBuilder::Table::findOrCreateParentForChild with multicol spanner 
https://bugs.webkit.org/show_bug.cgi?id=206917
Summary Crash in RenderTreeBuilder::Table::findOrCreateParentForChild with multicol s...
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
DetailsFormatted DiffDiff
Patch (5.83 KB, patch)
2020-01-30 18:15 PST, Doug Kelly 		zalan: review+
DetailsFormatted DiffDiff
Patch (6.63 KB, patch)
2020-02-01 10:04 PST, zalan 		no flags
DetailsFormatted DiffDiff
Patch (6.56 KB, patch)
2020-02-07 16:55 PST, Doug Kelly 		no flags
DetailsFormatted DiffDiff
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.