Bug 33012 – Marker code is buggy: referencePoint translation is off

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

Marker code is buggy since a while. Dynamic updates are completly failing (svg/custom/marker-changes*), because of a bug in SVGResource, introduced end 2007 (!).
Bounds are not calculated until painting time, so all repaint rects are off. Needs a rewrite.

[Nikolas Zimmermann]

Created attachment 45589 [details]
Initial patch

No regressions, only fixes :-)

[Nikolas Zimmermann]

Created attachment 45590 [details]
Updated patch

Forgot to preserve original copyright from RenderPath.cpp. Fixed.

[Dirk Schulze]

Comment on attachment 45590 [details]
Updated patch

Great work!

[Eric Seidel (no email)]

This broke bots!
http://build.webkit.org/builders/Leopard%20Intel%20Release%20%28Tests%29/builds/8810

Crashes on Leopard. :(

[Eric Seidel (no email)]

This looks to have been committed as http://trac.webkit.org/changeset/52630.

[Eric Seidel (no email)]

I suggest we roll this out unless there is a quick fix.

[Eric Seidel (no email)]

Committed r52633: <http://trac.webkit.org/changeset/52633>

[Dirk Schulze]

Comment on attachment 45590 [details]
Updated patch

The code hits assertions svgElement && svgElement->document() && svgElement->isStyled() in RenderPath::calculateMarkerBoundsIfNeeded for the following tests:
* svg/W3C-SVG-1.1/animate-elem-30-t.svg
* svg/W3C-SVG-1.1/animate-elem-40-t.svg

This LayoutTest needs an upda
* svg/W3C-SVG-1.1/painting-marker-01-f.svg

The feConv test is realy strange. The failure on feConv is realy strange. We saw it many times before. But my Mac always give me 115 for the text position. We should figure out why we have the problems with it.

[Dirk Schulze]

(In reply to comment #8)
> (From update of attachment 45590 [details])
> The code hits assertions svgElement && svgElement->document() &&
> svgElement->isStyled() in RenderPath::calculateMarkerBoundsIfNeeded for the
> following tests:
> * svg/W3C-SVG-1.1/animate-elem-30-t.svg
> * svg/W3C-SVG-1.1/animate-elem-40-t.svg
> 
> This LayoutTest needs an upda
> * svg/W3C-SVG-1.1/painting-marker-01-f.svg
> 
> The feConv test is realy strange. The failure on feConv is realy strange. We
> saw it many times before. But my Mac always give me 115 for the text position.
> We should figure out why we have the problems with it.

It looks like isStyled() is not alway true. Both tests have in common, that they use the use-element and a path in combination with animation.

[Dirk Schulze]

Niko, can you add a test with opacity < 1 so that we can close bug 26231 as well?

[Dirk Schulze]

*** Bug 15594 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

*** Bug 26231 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

Created attachment 45749 [details]
Updated patch v2

Updated patch, fixing the assertions seen on the bots, and also including a new layoutest to demonstrate that markers + opacity<1 work fine now.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/svg/graphics/SVGResourceMarker.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/rendering/SVGMarkerLayoutInfo.h:46:  _type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/rendering/SVGMarkerLayoutInfo.h:46:  _marker is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/rendering/SVGMarkerLayoutInfo.cpp:106:  _info is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/rendering/SVGMarkerLayoutInfo.cpp:116:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 5

[Nikolas Zimmermann]

(In reply to comment #14)
> Attachment 45749 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> WebCore/svg/graphics/SVGResourceMarker.h:31:  Alphabetical sorting problem. 
> [build/include_order] [4]
Will fix before landing.

> WebCore/rendering/SVGMarkerLayoutInfo.h:46:  _type is incorrectly named. Don't
> use underscores in your identifier names.  [readability/naming] [4]
> WebCore/rendering/SVGMarkerLayoutInfo.h:46:  _marker is incorrectly named.
> Don't use underscores in your identifier names.  [readability/naming] [4]
> WebCore/rendering/SVGMarkerLayoutInfo.cpp:106:  _info is incorrectly named.
> Don't use underscores in your identifier names.  [readability/naming] [4]
Who says that? Style guide does _not_ enforce this. I'm ignoring those warnings.

> WebCore/rendering/SVGMarkerLayoutInfo.cpp:116:  Tests for true/false,
> null/non-null, and zero/non-zero should all be done without equality
> comparisons.  [readability/comparison_to_zero] [5]
Hrm, this is true, though in this case, I think it's more readable this way, but I'm going to change this as well to make the bot happy.

[Adam Barth]

> Who says that? Style guide does _not_ enforce this. I'm ignoring those
> warnings.

I think this comes from rule #1 under Names: http://webkit.org/coding/coding-style.html

The root of the problem is that you're not using the "m_" prefix for member variables, as required by the style guide.

[Dirk Schulze]

Comment on attachment 45749 [details]
Updated patch v2

Doesn't fix the complete issue. Clearing the review flag for the moment.

[Nikolas Zimmermann]

Created attachment 45761 [details]
Updated patch v3

Finally, a patch fixing all known issues with markers, at least those I am aware off. Most important change is that we're calculating marker bounds on layout time now, instead of paint time. The old style fails on any dynamic updates (repainting). Clippers & Maskers already calculate the bounds on layout time, filters as well (though there's another issue that Dirk is already working on) - 2010 will become a good year for SVG + render tree :-)

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/rendering/SVGMarkerLayoutInfo.cpp:44:  _info is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1

[Nikolas Zimmermann]

Created attachment 45763 [details]
Updated patch v4

Updated the wrong layout test results, fixed.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45763 [details] without any errors.

[Dirk Schulze]

Comment on attachment 45763 [details]
Updated patch v4

Great work Niko!

[Nikolas Zimmermann]

Thanks for the quick review. Landed in r52745. Waiting for buildbots to finish, until I close this bug.

[Eric Seidel (no email)]

This was reverted *AGAIN*.

http://trac.webkit.org/changeset/52750

Please do not land this without testing on Leopard before landing.  The commit-queue can test on leopard for you if you do not have your own machine to test with.

[Dirk Schulze]

(In reply to comment #24)
> This was reverted *AGAIN*.
> 
> http://trac.webkit.org/changeset/52750
> 
> Please do not land this without testing on Leopard before landing.  The
> commit-queue can test on leopard for you if you do not have your own machine to
> test with.

Eric. This is a SL only bug. It's hard to find the reason for a crash on SL without testing it on this platform. I run the patch on Leopard (release and debug) and don't see crashes or asserts.
Please don't blame one of us but SL or the missing backtraces on the build bots that the patch needed to revert *again*.

[Eric Seidel (no email)]

The crashes occurred on Leopard, not SL:
http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r52745%20(8919)/results.html

[Eric Seidel (no email)]

I agree the lack of crash traces sucks.  That's bug 14861.

[Dirk Schulze]

(In reply to comment #26)
> The crashes occurred on Leopard, not SL:
> http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r52745%20(8919)/results.html

Ok. I'm sorry. I didn't realised the Leopard Release bot. But I tested this revision on my own Leopard (release build) and it realy doesn't crash on this two tests.
And they didn't crash on the Leopard Debug bot either. I don't have a clue what's wrong here.

[Eric Seidel (no email)]

My tone was needlessly harsh, for which I apologize.  This build break happened to be the most recent trigger to bug 30098, causing a false-rejection and thus my attention.  I also do not know why this latest test crashed w/o running it locally or solving bug 14861.

[Nikolas Zimmermann]

(In reply to comment #29)
> My tone was needlessly harsh, for which I apologize.  This build break happened
> to be the most recent trigger to bug 30098, causing a false-rejection and thus
> my attention.  I also do not know why this latest test crashed w/o running it
> locally or solving bug 14861.

To clarify, I asked Dimitry to revert the patch, as I was seeing these errors. As Dirk already mentioned, I am trying hard to reproduce these crashes on Leopard, but I am unable to so far. Will try another clean release build, as I was only using debug builds before, which just worked fine.

[Nikolas Zimmermann]

(In reply to comment #28)
> (In reply to comment #26)
> > The crashes occurred on Leopard, not SL:
> > http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r52745%20(8919)/results.html
> 
> Ok. I'm sorry. I didn't realised the Leopard Release bot. But I tested this
> revision on my own Leopard (release build) and it realy doesn't crash on this
> two tests.
> And they didn't crash on the Leopard Debug bot either. I don't have a clue
> what's wrong here.

Hah, I can reproduce the crashes on my leopard box now, using trunk + my patch and a release build. Will investigate tomorrow, need sleep.

[Nikolas Zimmermann]

Okay, got an evil backtrace:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000000
0x00000000 in ?? ()
(gdb) bt
#0  0x00000000 in ?? ()
#1  0x022311b9 in WebCore::RenderPath::calculateMarkerBoundsIfNeeded (this=0x9e8cac) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/rendering/RenderPath.cpp:282
#2  0x02231608 in WebCore::RenderPath::markerBoundingBox (this=0x9e8cac) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/rendering/RenderPath.cpp:140
#3  0x02230a20 in WebCore::RenderPath::repaintRectInLocalCoordinates (this=0xbfffd520) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/rendering/RenderPath.cpp:161
#4  0x0239e618 in WebCore::SVGRenderBase::clippedOverflowRectForRepaint (object=0x9e8cac, repaintContainer=0x0) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/rendering/SVGRenderSupport.cpp:57
#5  0x02240b70 in WebCore::RenderSVGModelObject::clippedOverflowRectForRepaint (this=0x9e8cac, repaintContainer=0x0) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/rendering/RenderSVGModelObject.cpp:54
#6  0x02224225 in WebCore::RenderObject::repaint (this=0x9e8cac, immediate=32) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/rendering/RenderObject.cpp:1143
#7  0x0222ca7a in WebCore::RenderObjectChildList::removeChildNode (this=0x9e8c20, owner=0x9e8c00, oldChild=0xbfffd520, fullRemove=true) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/rendering/RenderObjectChildList.cpp:73
#8  0x02223e76 in WebCore::RenderObject::removeChild (this=0x9e8c00, oldChild=0x9e8cac) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/rendering/RenderObject.cpp:333
#9  0x02226987 in WebCore::RenderObject::destroy (this=0x9e8c20) at RenderObject.h:744
#10 0x02173dfa in WebCore::Node::detach (this=0x36dd580) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/dom/Node.cpp:1263
#11 0x021769bf in WebCore::Node::~Node (this=0x36dd580) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/dom/Node.cpp:470
#12 0x023699a1 in WebCore::SVGLineElement::~SVGLineElement (this=0x36dd580) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/svg/SVGLineElement.cpp:49
#13 0x01a8813c in WebCore::removeAllChildrenInContainer<WebCore::Node, WebCore::ContainerNode> (container=0x36ffa50) at ContainerNodeAlgorithms.h:64
#14 0x01a857e6 in WebCore::ContainerNode::~ContainerNode (this=0x36ffa50) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/dom/ContainerNode.cpp:61
#15 0x02355cd9 in WebCore::SVGGElement::~SVGGElement (this=0x36ffa50) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/svg/SVGGElement.cpp:41
#16 0x023d311c in WebCore::SVGUseElement::buildPendingResource (this=0x36a5c00) at TreeShared.h:67
#17 0x023d067b in WTF::RefPtr<WebCore::SVGElement>::operator WebCore::SVGElement* WTF::RefPtr<WebCore::SVGElement>::* () at RefPtr.h:140
#18 0x023d067b in WebCore::SVGUseElement::svgAttributeChanged (this=0x36a5c00, attrName=@0x3745740) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/svg/SVGUseElement.cpp:142
#19 0x0216fa51 in WTF::RefPtr<WebCore::Attribute>::operator-> () at RefPtr.h:278
#20 0x0216fa51 in WebCore::NamedNodeMap::addAttribute (this=0x36a5c00, prpAttribute=@0xbfffd914) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/dom/NamedAttrMap.cpp:280
#21 0x01c6a81e in ~PassRefPtr [inlined] () at /Users/nikolaszimmermann/Coding/WebKit/WebCore/dom/Element.cpp:541
#22 ~PassRefPtr [inlined] () at PassRefPtr.h:56
#23 WebCore::Element::setAttribute (this=0x36a5c00, name=@0xbfffd9e0, value=@0xbfffd9dc, ec=@0xbfffd9e8) at PassRefPtr.h:541
#24 0x022fcc28 in ~RefPtr [inlined] () at /Users/nikolaszimmermann/Coding/WebKit/WebCore/svg/SVGAnimationElement.cpp:305
#25 ~RefPtr [inlined] () at RefPtr.h:53
#26 ~AtomicString [inlined] () at RefPtr.h:66
#27 ~AtomicString [inlined] () at RefPtr.h:39
#28 WebCore::SVGAnimationElement::setTargetAttributeAnimatedValue (this=0x3736c00, value=@0xbfffda5c) at text/AtomicString.h:305
#29 0x022f5c5e in ~RefPtr [inlined] () at /Users/nikolaszimmermann/Coding/WebKit/WebCore/svg/SVGAnimateElement.cpp:256
#30 ~String [inlined] () at RefPtr.h:53
#31 ~String [inlined] () at RefPtr.h:66
#32 WebCore::SVGAnimateElement::applyResultsToTarget (this=0x3736c00) at text/PlatformString.h:256
#33 0x022cd73f in WebCore::SMILTimeContainer::updateAnimations (this=0x9ba800, elapsed=@0xbfffdba8) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/svg/animation/SMILTimeContainer.cpp:307
#34 0x022cdad4 in WebCore::SMILTimeContainer::begin (this=0x9ba800) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/svg/animation/SMILTimeContainer.cpp:103
#35 0x02310638 in WebCore::SVGDocumentExtensions::startAnimations (this=0x3697630) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/svg/SVGDocumentExtensions.cpp:72
#36 0x01b42e95 in WebCore::Document::implicitClose (this=0x9fc000) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/dom/Document.cpp:1790

This will be fun to debug, but at least it's clear now, that this patch is causing trouble.
Note that RenderPath code ends up being called from a SVGLineElment destructor (probably a <use> shadow tree element) - this cries for trouble. At least I'm happy it's not a problem with <use> itself :-)

[Eric Seidel (no email)]

Comment on attachment 45763 [details]
Updated patch v4

Marking this patch r- since it was rolled out.

[Nikolas Zimmermann]

Created attachment 45984 [details]
Updated patch v5

Okay, found the real cause of the marker problems. A bug in SVGUseElement: need to detach() the shadow tree root element, before destructing it. Otherwhise we'll end up calling into Render* code, that tries to access SVG*Element objects, while those objects are being destructed. The bug only appears for the shadow tree root element, as it does not live in a Document, thus no one holds a RefPtr to it, all other regular nodes living in a Document are not affected. As we attach() the tree manually, we allso have to detach() manually. This also removes several hacks, about calling detach() from the SVGSVGElement/SVGMarkerElement destructor.

Tested with from-scratch release & debug builds. I can not reproduce any crashes/hangs/whatever anymore, should be safe. Builds on gtk/qt/win/mac & chromium, as the previous landing have shown. This patch also includes a new layout test, covering more marker stuff. Have fun reviewing :-)

[Eric Seidel (no email)]

Can we fix the <use> bug first?  i.e. split some chunk out of this huge patch?

[Dirk Schulze]

Comment on attachment 45984 [details]
Updated patch v5

It will hopefully fixed after the ... 5th review? :-) r=me

[Dirk Schulze]

(In reply to comment #35)
> Can we fix the <use> bug first?  i.e. split some chunk out of this huge patch?

The fix is smaler than nikos explanation. I think it's ok to commit it at once here.

[Nikolas Zimmermann]

Landed in r52866. Watching build bots again, until I close the bug.

[Nikolas Zimmermann]

Seems to be fixed. Unfortunately SL bots are not available atm, but I think it's fine.