[webkit-reviews] review granted: [Bug 205925] [Web Animations] Stop creating CSS Animations for <noscript> elements : [Attachment 387103] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Antoine Quint
<graouts at apple.com>'s request for review:
Bug 205925: [Web Animations] Stop creating CSS Animations for <noscript>
elements
https://bugs.webkit.org/show_bug.cgi?id=205925

Attachment 387103: Patch

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




--- Comment #3 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 387103
  --> https://bugs.webkit.org/attachment.cgi?id=387103
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.