[webkit-reviews] review denied: [Bug 232185] ASSERT(parent->element()) triggered in Styleable::fromRenderer : [Attachment 442214] Patch

[bugzilla-daemon at webkit.org]

Tim Nguyen (:ntim) <ntim at apple.com> has denied Gabriel Nava Marino
<gnavamarino at apple.com>'s request for review:
Bug 232185: ASSERT(parent->element()) triggered in Styleable::fromRenderer
https://bugs.webkit.org/show_bug.cgi?id=232185

Attachment 442214: Patch

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




--- Comment #2 from Tim Nguyen (:ntim) <ntim at apple.com> ---
Comment on attachment 442214
  --> https://bugs.webkit.org/attachment.cgi?id=442214
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/522cdac023da69b36fa895cbedea14e96f4
4d678/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