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>
Created attachment 389097 [details] Patch
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>
(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...
> > 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();
Yep, the second relayout fixes the issue. :)
Created attachment 389323 [details] Patch
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+.
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.
Created attachment 389463 [details] Patch
(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).
(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.)
Created attachment 390150 [details] Patch
Comment on attachment 390150 [details] Patch Clearing flags on attachment: 390150 Committed r256089: <https://trac.webkit.org/changeset/256089>
All reviewed patches have been landed. Closing bug.