Bug 205925

Created attachment 387103 [details]
Patch

<rdar://problem/58158479>

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.

In fact most rendererIsNeeded functions can be turned into rendererIsEverNeeded. 

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

(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

(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

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