Bug 33012 - Marker code is buggy: referencePoint translation is off
Summary: Marker code is buggy: referencePoint translation is off
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
: 15594 26231 (view as bug list)
Depends on:
Blocks: 30055
  Show dependency treegraph
 
Reported: 2009-12-29 03:05 PST by Nikolas Zimmermann
Modified: 2010-01-06 15:06 PST (History)
5 users (show)

See Also:


Attachments
Initial patch (164.42 KB, patch)
2009-12-29 03:23 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch (164.36 KB, patch)
2009-12-29 03:27 PST, Nikolas Zimmermann
krit: review-
Details | Formatted Diff | Diff
Updated patch v2 (156.05 KB, patch)
2010-01-02 13:21 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch v3 (212.06 KB, patch)
2010-01-03 17:55 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch v4 (213.18 KB, patch)
2010-01-03 19:58 PST, Nikolas Zimmermann
eric: review-
Details | Formatted Diff | Diff
Updated patch v5 (237.82 KB, patch)
2010-01-06 13:12 PST, Nikolas Zimmermann
krit: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2009-12-29 03:05:44 PST
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.
Comment 1 Nikolas Zimmermann 2009-12-29 03:23:20 PST
Created attachment 45589 [details]
Initial patch

No regressions, only fixes :-)
Comment 2 Nikolas Zimmermann 2009-12-29 03:27:59 PST
Created attachment 45590 [details]
Updated patch

Forgot to preserve original copyright from RenderPath.cpp. Fixed.
Comment 3 Dirk Schulze 2009-12-29 10:56:37 PST
Comment on attachment 45590 [details]
Updated patch

Great work!
Comment 4 Eric Seidel (no email) 2009-12-29 13:25:26 PST
This broke bots!
http://build.webkit.org/builders/Leopard%20Intel%20Release%20%28Tests%29/builds/8810

Crashes on Leopard. :(
Comment 5 Eric Seidel (no email) 2009-12-29 13:25:58 PST
This looks to have been committed as http://trac.webkit.org/changeset/52630.
Comment 6 Eric Seidel (no email) 2009-12-29 13:26:14 PST
I suggest we roll this out unless there is a quick fix.
Comment 7 Eric Seidel (no email) 2009-12-29 14:08:39 PST
Committed r52633: <http://trac.webkit.org/changeset/52633>
Comment 8 Dirk Schulze 2009-12-29 14:30:45 PST
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.
Comment 9 Dirk Schulze 2009-12-29 15:20:40 PST
(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.
Comment 10 Dirk Schulze 2009-12-30 01:06:05 PST
Niko, can you add a test with opacity < 1 so that we can close bug 26231 as well?
Comment 11 Dirk Schulze 2009-12-30 12:03:55 PST
*** Bug 15594 has been marked as a duplicate of this bug. ***
Comment 12 Nikolas Zimmermann 2010-01-02 13:12:21 PST
*** Bug 26231 has been marked as a duplicate of this bug. ***
Comment 13 Nikolas Zimmermann 2010-01-02 13:21:02 PST
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.
Comment 14 WebKit Review Bot 2010-01-02 13:22:06 PST
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
Comment 15 Nikolas Zimmermann 2010-01-02 13:32:28 PST
(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.
Comment 16 Adam Barth 2010-01-02 15:28:29 PST
> 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.
Comment 17 Dirk Schulze 2010-01-03 01:30:59 PST
Comment on attachment 45749 [details]
Updated patch v2

Doesn't fix the complete issue. Clearing the review flag for the moment.
Comment 18 Nikolas Zimmermann 2010-01-03 17:55:23 PST
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 :-)
Comment 19 WebKit Review Bot 2010-01-03 18:00:59 PST
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
Comment 20 Nikolas Zimmermann 2010-01-03 19:58:56 PST
Created attachment 45763 [details]
Updated patch v4

Updated the wrong layout test results, fixed.
Comment 21 WebKit Review Bot 2010-01-03 20:03:13 PST
style-queue ran check-webkit-style on attachment 45763 [details] without any errors.
Comment 22 Dirk Schulze 2010-01-04 00:11:37 PST
Comment on attachment 45763 [details]
Updated patch v4

Great work Niko!
Comment 23 Nikolas Zimmermann 2010-01-04 09:32:03 PST
Thanks for the quick review. Landed in r52745. Waiting for buildbots to finish, until I close this bug.
Comment 24 Eric Seidel (no email) 2010-01-04 13:17:14 PST
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.
Comment 25 Dirk Schulze 2010-01-04 14:14:26 PST
(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*.
Comment 26 Eric Seidel (no email) 2010-01-04 14:16:17 PST
The crashes occurred on Leopard, not SL:
http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r52745%20(8919)/results.html
Comment 27 Eric Seidel (no email) 2010-01-04 14:17:05 PST
I agree the lack of crash traces sucks.  That's bug 14861.
Comment 28 Dirk Schulze 2010-01-04 14:34:13 PST
(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.
Comment 29 Eric Seidel (no email) 2010-01-04 14:40:24 PST
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.
Comment 30 Nikolas Zimmermann 2010-01-04 17:29:08 PST
(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.
Comment 31 Nikolas Zimmermann 2010-01-04 19:02:55 PST
(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.
Comment 32 Nikolas Zimmermann 2010-01-04 19:24:26 PST
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 :-)
Comment 33 Eric Seidel (no email) 2010-01-06 08:27:01 PST
Comment on attachment 45763 [details]
Updated patch v4

Marking this patch r- since it was rolled out.
Comment 34 Nikolas Zimmermann 2010-01-06 13:12:43 PST
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 :-)
Comment 35 Eric Seidel (no email) 2010-01-06 13:15:23 PST
Can we fix the <use> bug first?  i.e. split some chunk out of this huge patch?
Comment 36 Dirk Schulze 2010-01-06 13:16:12 PST
Comment on attachment 45984 [details]
Updated patch v5

It will hopefully fixed after the ... 5th review? :-) r=me
Comment 37 Dirk Schulze 2010-01-06 13:19:44 PST
(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.
Comment 38 Nikolas Zimmermann 2010-01-06 14:07:08 PST
Landed in r52866. Watching build bots again, until I close the bug.
Comment 39 Nikolas Zimmermann 2010-01-06 15:06:49 PST
Seems to be fixed. Unfortunately SL bots are not available atm, but I think it's fine.