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.
Created attachment 90592 [details] repro
Created attachment 90615 [details] Patch
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">
Created attachment 90628 [details] Patch
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.
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.
(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.
This is crbug.com/73239, and maybe others.
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.
Created attachment 113709 [details] Patch
Created attachment 114157 [details] Patch
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.
(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.
Created attachment 114293 [details] Patch
Modified the test so that it crashes in versions prior to this patch. This patch does not crash. :-)
Comment on attachment 114293 [details] Patch LGTM. r=me.
Comment on attachment 114293 [details] Patch Clearing flags on attachment: 114293 Committed r99851: <http://trac.webkit.org/changeset/99851>
All reviewed patches have been landed. Closing bug.
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
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.
Created attachment 114596 [details] Patch
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.
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.
Created attachment 114599 [details] Patch Fixes S style checks
Comment on attachment 114599 [details] Patch r=me, let's test it :-)
Comment on attachment 114599 [details] Patch Clearing flags on attachment: 114599 Committed r100046: <http://trac.webkit.org/changeset/100046>