Bug 232185 – ASSERT(parent->element()) triggered in Styleable::fromRenderer

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Gabriel Nava Marino]

We used to check that renderer.element() was not nullptr here before, but it looks like that was removed for some cases in a recent refactor (bug #230008). Adding it back seems to resolve the issue.

[Gabriel Nava Marino]

Created attachment 442214 [details]
Patch

[Tim Nguyen (:ntim)]

Comment on attachment 442214 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=442214&action=review

> Source/WebCore/style/Styleable.cpp:57
> +    if (!renderer.element())
> +        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.

> LayoutTests/fast/animation/css-animation-marker-crash.html:4
> +      -webkit-mask-image: url();

maybe use a more common property like `background: green;`

> LayoutTests/fast/animation/css-animation-marker-crash.html:15
> +<li>PASS</li>

nit: PASS if this doesn't crash

[Tim Nguyen (:ntim)]

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);
```

Please file a followup bug to add a test for the correctness of `Styleable::fromRenderer` for `::backdrop` and `::marker`.

>> 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.

[Gabriel Nava Marino]

(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.

[Gabriel Nava Marino]

Created attachment 442385 [details]
Patch

[Tim Nguyen (:ntim)]

Comment on attachment 442385 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=442385&action=review

This looks fine to me, though I'm not a reviewer.

> Source/WebCore/ChangeLog:10
> +        loop through parent renderers as ::backdrop and ::marker renderers do not

the problem here is that RenderMultiColumnFlowThread don't have associated renderers, so we have to loop through parents until we find the RenderListItem which has an associated element.

[Tim Nguyen (:ntim)]

Comment on attachment 442385 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=442385&action=review

>> Source/WebCore/ChangeLog:10
>> +        loop through parent renderers as ::backdrop and ::marker renderers do not
> 
> the problem here is that RenderMultiColumnFlowThread don't have associated renderers, so we have to loop through parents until we find the RenderListItem which has an associated element.

don't have associated elements*

[Antti Koivisto]

Comment on attachment 442385 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=442385&action=review

>>> Source/WebCore/ChangeLog:10
>>> +        loop through parent renderers as ::backdrop and ::marker renderers do not
>> 
>> the problem here is that RenderMultiColumnFlowThread don't have associated renderers, so we have to loop through parents until we find the RenderListItem which has an associated element.
> 
> don't have associated elements*

CSS term for a renderer without an associated element is "anonymous box"

[Gabriel Nava Marino]

Created attachment 442490 [details]
Patch

[Gabriel Nava Marino]

(In reply to Tim Nguyen (:ntim) from comment #6)
> Comment on attachment 442385 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=442385&action=review
> 
> This looks fine to me, though I'm not a reviewer.
> 
> > Source/WebCore/ChangeLog:10
> > +        loop through parent renderers as ::backdrop and ::marker renderers do not
> 
> the problem here is that RenderMultiColumnFlowThread don't have associated
> renderers, so we have to loop through parents until we find the
> RenderListItem which has an associated element.

Thank you, I have updated this to reflect in the ChangeLog.

[EWS]

Committed r284871 (243548@main): <https://commits.webkit.org/243548@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 442490 [details].

[Radar WebKit Bug Importer]

<rdar://problem/84665480>