Bug 232185

Created attachment 442214 [details] Patch

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

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.

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

Created attachment 442385 [details] Patch

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.

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*

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"

Created attachment 442490 [details] Patch

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

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

<rdar://problem/84665480>