[Webkit-unassigned] [Bug 206917] Crash in RenderTreeBuilder::Table::findOrCreateParentForChild with multicol spanner

Thu Jan 30 14:27:33 PST 2020

https://bugs.webkit.org/show_bug.cgi?id=206917

--- Comment #3 from Doug Kelly <dougk at apple.com> ---
(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...

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200130/4bb10a99/attachment.htm>