Bug 205925 – [Web Animations] Stop creating CSS Animations for <noscript> elements

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Antoine Quint]

[Web Animations] Stop creating CSS Animations for <noscript> elements

[Antoine Quint]

Created attachment 387103 [details]
Patch

[Antoine Quint]

<rdar://problem/58158479>

[Antti Koivisto]

Comment on attachment 387103 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        It makes no sense to create CSS Animations for a <noscript> element and it has the side effect of potential crashes.
> +        Indeed, AnimationTimeline::updateCSSAnimationsForElement() may be called without a currentStyle and so we never have
> +        a list of previously-applied animations to compare to the list of animations in afterChangeStyle. So on each call we
> +        end up creating a new CSSAnimation and the previous animation for the same name is never explicitly removed from the
> +        effect stack and is eventually destroyed and the WeakPtr for it in the stack ends up being null, which would cause a
> +        crash under KeyframeEffectStack::ensureEffectsAreSorted().

You'll should add some other defense against this in animation code too as this patch likely doesn't cover all the paths where this can happen.

> Source/WebCore/html/HTMLElement.cpp:736
> -bool HTMLElement::rendererIsNeeded(const RenderStyle& style)
> +bool HTMLElement::rendererIsEverNeeded()

You can also switch HTMLInputElement::rendererIsNeeded to rendererIsEverNeeded, maybe others. Please check, they can probably all be used to get this crash.

[Antti Koivisto]

In fact most rendererIsNeeded functions can be turned into rendererIsEverNeeded. 

Doesn't need to be done in this patch though.

[Antoine Quint]

(In reply to Antti Koivisto from comment #3)
> Comment on attachment 387103 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387103&action=review
> 
> > Source/WebCore/ChangeLog:16
> > +        It makes no sense to create CSS Animations for a <noscript> element and it has the side effect of potential crashes.
> > +        Indeed, AnimationTimeline::updateCSSAnimationsForElement() may be called without a currentStyle and so we never have
> > +        a list of previously-applied animations to compare to the list of animations in afterChangeStyle. So on each call we
> > +        end up creating a new CSSAnimation and the previous animation for the same name is never explicitly removed from the
> > +        effect stack and is eventually destroyed and the WeakPtr for it in the stack ends up being null, which would cause a
> > +        crash under KeyframeEffectStack::ensureEffectsAreSorted().
> 
> You'll should add some other defense against this in animation code too as
> this patch likely doesn't cover all the paths where this can happen.

It's more than a defense that is needed, the problem is that `AnimationTimeline` doesn't know to expect several CSS Animations with the same animation-name. I will address this as part of a different bug as soon as possible. I raised https://bugs.webkit.org/show_bug.cgi?id=205928.

> > Source/WebCore/html/HTMLElement.cpp:736
> > -bool HTMLElement::rendererIsNeeded(const RenderStyle& style)
> > +bool HTMLElement::rendererIsEverNeeded()
> 
> You can also switch HTMLInputElement::rendererIsNeeded to
> rendererIsEverNeeded, maybe others. Please check, they can probably all be
> used to get this crash.

I will add more rendererIsNeeded overrides in a followup, I raised

[Antoine Quint]

(In reply to Antoine Quint from comment #5)
> I will add more rendererIsNeeded overrides in a followup, I raised

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

[Antoine Quint]

Committed r254201: <https://trac.webkit.org/changeset/254201>