Bug 33012

Created attachment 45589 [details] Initial patch No regressions, only fixes :-)

Created attachment 45590 [details] Updated patch Forgot to preserve original copyright from RenderPath.cpp. Fixed.

Comment on attachment 45590 [details] Updated patch Great work!

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

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

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

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

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.

(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.

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

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

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

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.

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

(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.

> 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 on attachment 45749 [details] Updated patch v2 Doesn't fix the complete issue. Clearing the review flag for the moment.

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 :-)

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

Created attachment 45763 [details] Updated patch v4 Updated the wrong layout test results, fixed.

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

Comment on attachment 45763 [details] Updated patch v4 Great work Niko!

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

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.

(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*.

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

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

(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.

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.

(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.

(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.

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 on attachment 45763 [details] Updated patch v4 Marking this patch r- since it was rolled out.

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 :-)

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

Comment on attachment 45984 [details] Updated patch v5 It will hopefully fixed after the ... 5th review? :-) r=me

(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.

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

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