Bug 59136 – Null deref when no use element exists for SVG element instance

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Cris Neckar]

Within SVGAnimationElement::setTargetAttributeAnimatedValue() the target element's instances are walked and the cooresponding use elements are set to need style recalc. When there is no cooresponding use element this will crash with a null deref.

I have a patch and will upload in shortly.

[Cris Neckar]

Created attachment 90592 [details]
repro

[Cris Neckar]

Created attachment 90615 [details]
Patch

[Justin Schuh]

Comment on attachment 90615 [details]
Patch

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

The approach looks good. I just have some style nits and suggested cleanup on the layout test.

> Source/WebCore/svg/SVGAnimationElement.cpp:346
> +            (*it)->correspondingUseElement()->setNeedsStyleRecalc();

You should assign the corresponding use element to a local variable in the "if" condition and use that when calling setNeedsStyleRecalc.

> LayoutTests/svg/misc/null-corresponding-element-crash.svg:12
> +    <circle transform="translate(1)" id="circleID">

You should make the circle visible here, and generally we use a green fill to denote success. How about this:
<circle transform="translate(1)" id="circleID" fill="green" cy="25" cx="25" r="20" >

> LayoutTests/svg/misc/null-corresponding-element-crash.svg:14
> +        <animateTransform attributeName="transform" fill="freeze" />

The fill attributes are unnecessary on animate and animateTransform.

> LayoutTests/svg/misc/null-corresponding-element-crash.svg:17
> +	PASS - Null corresponding element dereference does not crash.

This text is going to render off-screen when running the test manually. You should move it into view. try this:

<text y="70">

[Cris Neckar]

Created attachment 90628 [details]
Patch

[Brent Fulgham]

Comment on attachment 90628 [details]
Patch

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

> Source/WebCore/svg/SVGAnimationElement.cpp:346
> +        if (SVGUseElement *useElement = (*it)->correspondingUseElement())

There should not be a space between the SVGUseElement and the * character.

[Nikolas Zimmermann]

Comment on attachment 90628 [details]
Patch

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

Sorry Cris, I fear that involves a bit more work:

> Source/WebCore/svg/SVGAnimationElement.cpp:347
> +            useElement->setNeedsStyleRecalc();

I'm confused a shadowTreeElement w/o correspondingUseElement, what _exactly_ forces us into such a state?
What happens if you completely disable the shadowTreeElement modifications, if correspondingUseElement is 0, does the animation still work?

> LayoutTests/svg/custom/use-null-instanceroot-crash.svg:5
> +        document.getElementById("use_elem").instanceRoot.correspondingElement = 0;

Hm, that is needed? Or does it even trigger the bug?? In the code changes, you're checking whether correspondingUseElement is zero, not correspondingElement?
SVGElementInstance.idl defines: readonly attribute SVGElement correspondingElement;
so this shouldn't take any affect at all, does it?

> LayoutTests/svg/custom/use-null-instanceroot-crash.svg:17
> +        <animateTransform attributeName="transform" />

Why are two animations needed, why the transform attribute?
You definately also need a new testcase using the JS SVG Animation test framework, see the other examples in LayoutTests/svg/animation.
We need to be sure that each of the animation runs properly, so you have to setup a real animation, cy from 0 to 15, for example.

Basically we need to sample the animation at various times, and assure both animations, the one of circleID, and the circleID clone through <use> work as expected.

> LayoutTests/svg/custom/use-null-instanceroot-crash.svg:19
> +    <use id="use_elem" xlink:href="#circleID" />

You should use x="30", to translate the second circle, so when running the test in the browser, you'll actually see two circles.

[Justin Schuh]

(In reply to comment #6)
> (From update of attachment 90628 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90628&action=review
> 
> Sorry Cris, I fear that involves a bit more work:
> 
> > Source/WebCore/svg/SVGAnimationElement.cpp:347
> > +            useElement->setNeedsStyleRecalc();
> 
> I'm confused a shadowTreeElement w/o correspondingUseElement, what _exactly_ forces us into such a state?
> What happens if you completely disable the shadowTreeElement modifications, if correspondingUseElement is 0, does the animation still work?

The null correspondingUseElement is the same partially detached state that we check for explicitly in SVGElementInstance::invalidateAllInstancesOfElement. However, animation bypasses that code path and pokes properties directly (I assume to reduce the overhead of constant shadow tree destruction and rebuilding). So, it seems reasonable that animation should have the same null check.

 
> > LayoutTests/svg/custom/use-null-instanceroot-crash.svg:5
> > +        document.getElementById("use_elem").instanceRoot.correspondingElement = 0;
> 
> Hm, that is needed? Or does it even trigger the bug?? In the code changes, you're checking whether correspondingUseElement is zero, not correspondingElement?
> SVGElementInstance.idl defines: readonly attribute SVGElement correspondingElement;
> so this shouldn't take any affect at all, does it?

Referencing the instanceRoot just gets it created earlier than it would be by startAnimation. That's how we end up with a partially detached instanceRoot. You're correct that dereferencing correspondingElement and making an assignment are unnecessary. Sorry, I noticed that and meant to point it out in my earlier informal review.

[Stephen Chenney]

This is crbug.com/73239, and maybe others.

[Stephen Chenney]

OK campers. I have the underlying source of the problem.

The SVGElementInstance is a ref counted object, and any reference to it in Javascript increases the ref count. So when the instance is released, due to a rebuild of the <use> shadow DOM, it is not actually deleted and so it is not removed from the <use>'s target element's instance set.

The fix is to remove the hanging instance from the element's instance set at the time the <use> element destroys the instance tree. Patch forthcoming tomorrow.

[Stephen Chenney]

Created attachment 113709 [details]
Patch

[Stephen Chenney]

Created attachment 114157 [details]
Patch

[Stephen Chenney]

I have modified the test to only use JS and no animation. However, this will not cause a crash on failure because the crash only occurs via an animation shortcut that accesses the use instance elements directly.

We really want to test that the instanceList for the circle element only contains one instance, but we cannot get at that information from JS and I could not come up with a way to test it outside of the animate tag.

Personally I would rather rather leave in the animation that causes the crash, and hence have a direct test. But I'll respect the reviewer's opinion that we avoid animation. Note that we can remove the animateTransform without removing the animate, if that is preferable to the indirect JS null check.

[Nikolas Zimmermann]

(In reply to comment #12)
> I have modified the test to only use JS and no animation. However, this will not cause a crash on failure because the crash only occurs via an animation shortcut that accesses the use instance elements directly.
Okay, that means the animation is unavoidable.
 
> Personally I would rather rather leave in the animation that causes the crash, and hence have a direct test. But I'll respect the reviewer's opinion that we avoid animation. Note that we can remove the animateTransform without removing the animate, if that is preferable to the indirect JS null check.
Heh, you got me wrong Stephen. I suggested to try to find a JS only solution, that exposes the problem. If there is none and you've gained confidence on that, I'm going to trust you :-)

So the testcase in your last patch is not going to crash at all? Then it would be useless, and we'd need your older anim based test.

[Stephen Chenney]

Created attachment 114293 [details]
Patch

[Stephen Chenney]

Modified the test so that it crashes in versions prior to this patch. This patch does not crash. :-)

[Nikolas Zimmermann]

Comment on attachment 114293 [details]
Patch

LGTM. r=me.

[WebKit Review Bot]

Comment on attachment 114293 [details]
Patch

Clearing flags on attachment: 114293

Committed r99851: <http://trac.webkit.org/changeset/99851>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Stephen Chenney]

The patch causes the following tests to fail with a crash on Qt:
svg/custom/use-instanceRoot-with-use-removed.svg
svg/custom/empty-clip-path.svg
svg/custom/use-crash-when-href-change.svg

And changed expectations for Qt on:
svg/batik/text/textEffect.svg
svg/batik/text/textEffect3.svg
svg/custom/text-filter.svg
svg/filters/feMerge-wrong-input.svg
svg/filters/filter-on-filter-for-text.svg
svg/filters/filter-on-tspan.svg

[Stephen Chenney]

The only new crashes are:

Regressions: Unexpected DumpRenderTree crashes : (3)
  svg/custom/embedded-svg-disallowed-in-dashboard.xml = CRASH
  svg/custom/use-crash-when-href-change.svg = CRASH
  svg/custom/use-instanceRoot-with-use-removed.svg = CRASH

These occur on mac and Qt, so this is probably something to do with the JS implementation on those platforms and its handling of the detached and heavily nulled SVGElementInstance.

[Stephen Chenney]

Created attachment 114596 [details]
Patch

[Stephen Chenney]

This addresses the crash problems on mac and Qt (ports using the default webkit JS engine). Turns out the garbage collection code in JS needed to use the correspondingElement object, which the previous patch was clearing. The ultimate effect of this new patch is the same as the one that was rolled back earlier.

[WebKit Review Bot]

Attachment 114596 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/svg/..." exit_code: 1

LayoutTests/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Stephen Chenney]

Created attachment 114599 [details]
Patch

Fixes S style checks

[Nikolas Zimmermann]

Comment on attachment 114599 [details]
Patch

r=me, let's test it :-)

[WebKit Review Bot]

Comment on attachment 114599 [details]
Patch

Clearing flags on attachment: 114599

Committed r100046: <http://trac.webkit.org/changeset/100046>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.