[Webkit-unassigned] [Bug 232185] ASSERT(parent->element()) triggered in Styleable::fromRenderer

[bugzilla-daemon at webkit.org]

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

--- Comment #4 from Gabriel Nava Marino <gnavamarino at apple.com> ---
(In reply to Tim Nguyen (:ntim) from comment #3)
> Comment on attachment 442214 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=442214&action=review
> 
> Here are some suggestions on how to address my previous comments.
> 
> >> Source/WebCore/style/Styleable.cpp:57
> >> +        return std::nullopt;
> > 
> > This looks wrong, ::backdrop and ::marker renderers do not have any associated element, so the code below for ::backdrop & ::marker will just stop working altogether.
> > 
> > I'm surprised no test has caught this so far.
> > 
> > From a quick look at the render tree dump, overflow-y: -webkit-paged-y on <li> seems to put the marker renderer as a child of RenderMultiColumnFlowThread instead of RenderListItem.
> > 
> > I'm glad the assert caught this, because it shows a real bug in the code. We should either:
> > 
> > * In the `case PseudoId::Marker:` branch of this function, loop through renderer ancestors until we find a `RenderListItem`, and return that.
> > 
> > * Simply disallow multi column flow for list items? See: https://webkit-search.igalia.com/webkit/rev/522cdac023da69b36fa895cbedea14e96f44d678/Source/WebCore/rendering/RenderBlockFlow.cpp#425-426
> > 
> > I wasn't able to make multicol work properly inside a list item, so maybe this is a good solution? Though not sure if there's any other feature which may wrap renderers and trigger this assert.
> 
> Let's loop through parent renderers for now, since column flow does work if
> you use `columns`.
> 
> ```
> case PseudoId::Marker:
>   auto* ancestor = renderer.parent();
>   while (ancestor && !ancestor->element()) {
>     ancestor = ancestor.parent();
>   }
>   ASSERT(is<RenderListItem>(ancestor));
>   ASSERT(downcast<RenderListItem>(ancestor)->markerRenderer() == &renderer);
>   return Styleable(*ancestor->element(), PseudoId::Marker);
> ```

Thank you, I have made the recommended changes and can confirm this adresses the crash.
> 
> Please file a followup bug to add a test for the correctness of
> `Styleable::fromRenderer` for `::backdrop` and `::marker`.

Thank you, I have filed bug #232248.
> 
> >> LayoutTests/fast/animation/css-animation-marker-crash.html:4
> >> +      -webkit-mask-image: url();
> > 
> > maybe use a more common property like `background: green;`
> 
> oh, only composited properties trigger this crash, maybe `opacity: 0`?
> 
> > LayoutTests/fast/animation/css-animation-marker-crash.html:12
> > +    overflow-y: -webkit-paged-y;
> 
> Can you use `columns: 3;` instead? It also triggers this crash, as it
> triggers column flow. It's more standard/common than overflow:
> -webkit-paged-y.
Thank you, I have updated the test case to use both `columns: 3;` and `opacity: 0` and confirmed the issue is still caught.

-- 
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/20211025/9b3012aa/attachment-0001.htm>