Bug 40366 – REGRESSION (r59263): Google Docs Drawing is broken, and probably other SVG-based sites

• Component: SVG
• Status: RESOLVED FIXED

[Dimitri Glazkov (Google)]

Steps to repro:

* Create new Drawing in Google Docs
* Click on shapes
* Attempt to draw a shape

Expected result:

* should draw a shape

Actual result

* nothing draws.

Related Chromium bug: http://crbug.com/46156

[Alexey Proskuryakov]

<rdar://problem/8076507>

[James Robinson]

We're clearly underpainting, probably by eating a layout root completely instead of scheduling a relayout.  Resizing the window causes the selection UI to snap into place.

I'll see if I can reduce this a bit but it won't be easy.

[James Robinson]

Google Docs has pushed a workaround for this issue, but the bug is still unfixed.  The workaround is disgusting and I do not want to evangelize it if possible :(.  I can still reproduce this locally and will try to make a distributable reproduction ASAP.

Somehow we're getting into a state where a RenderSVG* object is marked as m_needsLayout, its parent RenderSVGRoot object is marked as m_posChildNeedsLayout, and yet the RenderSVGRoot's container does not have any layout bits set and the FrameView's m_layoutRoot and m_layoutTimer are both unset.  Either we are doing layout without actually traversing some dirty objects or we're resetting the m_layoutRoot without dirtying its containers properly.

[Dirk Schulze]

Can someone provide a simple test case, that shows the problem?

[James Robinson]

I still haven't been able to construct a repro page but have caught the page going awry in a debugger.

Here's the render tree when things go wrong:

RenderView
- RenderBlock (relayout boundary) 0x1145e5e68
-- RenderSVGRoot (relayout boundary) 0x119fc7968
--- RenderPath

and the stack:

(gdb) bt
#0  WebCore::RenderObject::markContainingBlocksForLayout (this=0x119fc7968, scheduleRelayout=false, newRoot=0x1145e5e68) at RenderObject.h:1036
#1  0x000000010176499f in WebCore::FrameView::scheduleRelayoutOfSubtree (this=0x11799f130, relayoutRoot=0x1145e5e68) at /usr/local/home/jamesr/WebKit/WebCore/page/FrameView.cpp:1330
#2  0x0000000101d1b513 in WebCore::RenderObject::scheduleRelayout (this=0x1145e5e68) at /usr/local/home/jamesr/WebKit/WebCore/rendering/RenderObject.cpp:2092
#3  0x00000001014dd795 in WebCore::RenderObject::markContainingBlocksForLayout (this=0x119fc7968, scheduleRelayout=true, newRoot=0x0) at RenderObject.h:1035
#4  0x00000001014dd83d in WebCore::RenderObject::setNeedsLayout (this=0x119fc7968, b=true, markParents=true) at RenderObject.h:941
#5  0x0000000101f0a1e8 in WebCore::SVGSVGElement::svgAttributeChanged (this=0x119f76b50, attrName=@0x119e13758) at /usr/local/home/jamesr/WebKit/WebCore/svg/SVGSVGElement.cpp:312
#6  0x0000000101e7fc30 in WebCore::SVGElement::attributeChanged (this=0x119f76b50, attr=0x119e13750, preserveDecls=false) at /usr/local/home/jamesr/WebKit/WebCore/svg/SVGElement.cpp:304
#7  0x00000001016e6f18 in WebCore::Element::setAttribute (this=0x119f76b50, name=@0x103180b60, value=@0x7fff5fbfad50) at /usr/local/home/jamesr/WebKit/WebCore/dom/Element.cpp:598
#8  0x00000001016e718f in WebCore::Element::setAttribute (this=0x119f76b50, name=@0x103180b60, value=@0x7fff5fbfad50) at /usr/local/home/jamesr/WebKit/WebCore/dom/Element.cpp:180
#9  0x0000000101e52074 in WebCore::StyledElement::updateStyleAttribute (this=0x119f76b50) at /usr/local/home/jamesr/WebKit/WebCore/dom/StyledElement.cpp:110
#10 0x000000010140c527 in WebCore::Element::attributes (this=0x119f76b50, readonly=true) at Element.h:372
#11 0x00000001016e5e78 in WebCore::Element::hasAttributeNS (this=0x119f76b50, namespaceURI=@0x108d8a008, localName=@0x108d8a000) at /usr/local/home/jamesr/WebKit/WebCore/dom/Element.cpp:1284
#12 0x00000001016e5f3d in WebCore::Element::hasAttribute (this=0x119f76b50, name=@0x1031816a8) at /usr/local/home/jamesr/WebKit/WebCore/dom/Element.cpp:212
#13 0x0000000101ec20f3 in WebCore::SVGLength::PercentageOfViewport (value=1, context=0x119ef3ac0, mode=WebCore::LengthModeHeight) at /usr/local/home/jamesr/WebKit/WebCore/svg/SVGLength.cpp:294
#14 0x0000000101ec1d6f in WebCore::SVGLength::value (this=0x7fff5fbfb000, context=0x119ef3ac0) at /usr/local/home/jamesr/WebKit/WebCore/svg/SVGLength.cpp:136
#15 0x0000000101eeb6e7 in WebCore::SVGRectElement::toPathData (this=0x119ef3ac0) at /usr/local/home/jamesr/WebKit/WebCore/svg/SVGRectElement.cpp:148
#16 0x0000000101d26aca in WebCore::RenderPath::layout (this=0x119fc6f78) at /usr/local/home/jamesr/WebKit/WebCore/rendering/RenderPath.cpp:112
#17 0x0000000101ca0aab in WebCore::RenderObject::layoutIfNeeded (this=0x119fc6f78) at RenderObject.h:544
#18 0x0000000101ef2788 in WebCore::SVGRenderBase::layoutChildren (start=0x119fc7968, selfNeedsLayout=false) at /usr/local/home/jamesr/WebKit/WebCore/rendering/SVGRenderSupport.cpp:256
#19 0x0000000101d58953 in WebCore::RenderSVGRoot::layout (this=0x119fc7968) at /usr/local/home/jamesr/WebKit/WebCore/rendering/RenderSVGRoot.cpp:121
#20 0x00000001017666a8 in WebCore::FrameView::layout (this=0x11799f130, allowSubtree=true) at /usr/local/home/jamesr/WebKit/WebCore/page/FrameView.cpp:764
#21 0x00000001015cc409 in WebCore::Document::updateLayout (this=0x1180f9400) at /usr/local/home/jamesr/WebKit/WebCore/dom/Document.cpp:1454
#22 0x00000001015ce5fd in WebCore::Document::updateLayoutIgnorePendingStylesheets (this=0x1180f9400) at /usr/local/home/jamesr/WebKit/WebCore/dom/Document.cpp:1485
#23 0x00000001014b80bc in WebCore::CSSComputedStyleDeclaration::getPropertyCSSValue (this=0x11b214870, propertyID=1002, updateLayout=WebCore::UpdateLayout) at /usr/local/home/jamesr/WebKit/WebCore/css/CSSComputedStyleDeclaration.cpp:669
#24 0x00000001014c0375 in WebCore::CSSComputedStyleDeclaration::getPropertyCSSValue (this=0x11b214870, propertyID=1002) at /usr/local/home/jamesr/WebKit/WebCore/css/CSSComputedStyleDeclaration.cpp:588
#25 0x00000001015307df in WebCore::CSSStyleDeclaration::getPropertyCSSValue (this=0x11b214870, propertyName=@0x7fff5fbfcd80) at /usr/local/home/jamesr/WebKit/WebCore/css/CSSStyleDeclaration.cpp:45
#26 0x000000010193b455 in WebCore::JSCSSStyleDeclaration::nameGetter (exec=0x112a110f0, slotBase={m_ptr = 0x11a1b29c0}, propertyName=@0x7fff5fbfcf00) at /usr/local/home/jamesr/WebKit/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:156
#27 0x000000010075bc5c in JSC::PropertySlot::getValue (this=0x7fff5fbfce30, exec=0x112a110f0, propertyName=@0x7fff5fbfcf00) at PropertySlot.h:78

A JS query for style.direction forces a subtree layout rooted at the FrameView's m_layoutRoot, which happens to be the RenderSVGRoot.  This manages to mark its containing RenderBlock as m_posChildNeedsLayout and attempts to schedule a new relayout rooted at this renderer.  This changes the m_layoutRoot from the RenderSVGRoot to the RenderBlock, but since layout is already underway this doesn't set the layout timer.  Instead layout proceeds normally (but still rooted at the RenderSVGRoot).  The end of the layout algorithm clears out the FrameView's m_layoutRoot.  This leaves the RenderBlock with m_posChildNeedsLayout set but with no layout actually scheduled to occur.  When elements beneath this RenderBlock are subsequently marked as needing layout, markContainingBlocksForLayout() notices that m_posChildNeedsLayout is already set and returns without scheduling another layout pass.

I'm not entirely sure whether the re-rooting logic is at fault here or of it's improper to mark elements outside of a subtree as needing layout during a subtree layout.  The RenderBlock is marked as m_posChildNeedsLayout through frames 5-13.  The RenderPath queries for the viewPort attribute on the SVGSVGElement which triggers an update of the style attribute that in turn calls attributeChanged which is overridden by SVGElement to mark the element's renderer (the RenderSVGRoot) to get marked as needing layout.  This pathway seems a bit odd.

Thanks for carrying on the investigation!

(In reply to comment #5)
> (gdb) bt
> #8  0x00000001016e718f in WebCore::Element::setAttribute (this=0x119f76b50, name=@0x103180b60, value=@0x7fff5fbfad50) at /usr/local/home/jamesr/WebKit/WebCore/dom/Element.cpp:180
[…]
> #12 0x00000001016e5f3d in WebCore::Element::hasAttribute (this=0x119f76b50, name=@0x1031816a8) at /usr/local/home/jamesr/WebKit/WebCore/dom/Element.cpp:212
> #13 0x0000000101ec20f3 in WebCore::SVGLength::PercentageOfViewport (value=1, context=0x119ef3ac0, mode=WebCore::LengthModeHeight) at /usr/local/home/jamesr/WebKit/WebCore/svg/SVGLength.cpp:294
> #14 0x0000000101ec1d6f in WebCore::SVGLength::value (this=0x7fff5fbfb000, context=0x119ef3ac0) at /usr/local/home/jamesr/WebKit/WebCore/svg/SVGLength.cpp:136
> #15 0x0000000101eeb6e7 in WebCore::SVGRectElement::toPathData (this=0x119ef3ac0) at /usr/local/home/jamesr/WebKit/WebCore/svg/SVGRectElement.cpp:148
> #16 0x0000000101d26aca in WebCore::RenderPath::layout (this=0x119fc6f78) at /usr/local/home/jamesr/WebKit/WebCore/rendering/RenderPath.cpp:112
[…]
> #20 0x00000001017666a8 in WebCore::FrameView::layout (this=0x11799f130, allowSubtree=true) at /usr/local/home/jamesr/WebKit/WebCore/page/FrameView.cpp:764

That Element::setAttribute() ends up being invoked beneath layout is insane. I don’t know if the problem is that hasAttibute() can call setAttribute() or, more likely, that PercentageOfViewport() calls a function (hasAttribute()) that has a known side effect of setting an attribute. But this is just asking for trouble. I think the path to resolving this bug is preventing DOM mutation during layout (other than shadow DOM, of course).

[Jonathan Kliegman]

I've been trying to get a simple test case for this and haven't been able to simplify it enough.

I have seen what you are showing though from my own testing.  Mainly m_layoutRoot being reset while in the middle of a layout.  

One thing I've found is that adding showNode calls into the rendering process for debugging actually fixes the problem.  It looks like the side effect of it you mention (where it sets the attributes) changes the order enough that the node doesn't get skipped.

I'm wondering if just setting a flag to do a full layout if the DOM is changed during a layout might work here?  Its not the most efficient/elegant solution, but it *should* at least give us correctness in this case.  If the case is rare enough that its not an issue then the performance hit won't be too bad.  If this case is common enough to hurt us I expect we'd have seen more reports.  Although we do need to be careful we don't accidentally loop (not sure if that's possible here).

[Dave Hyatt]

The bug is that SVG should not be marking things as needing layout when the style attribute changes.  The style system already handled marking things appropriately for layout or not based off style diffing, so you never have to do anything when a style attribute changes.

[James Robinson]

Querying any attribute calls Element::attributes() which will call setAttribute() for the styleAttr if it's out of date.  One fix suggested in IRC is to avoid calling setNeedsLayout() if the changed attribute is styleAttr, since changing the styles on an element should always mark the element as needing layout anyway.  I'm going to try that locally and see what happens on the rest of the tests.

[James Robinson]

As it turns out this is sufficient to fix the issue on the older, affected version of google docs:

diff --git a/WebCore/svg/SVGStyledElement.cpp b/WebCore/svg/SVGStyledElement.cpp
index 8ae463d..10cc793 100644
--- a/WebCore/svg/SVGStyledElement.cpp
+++ b/WebCore/svg/SVGStyledElement.cpp
@@ -228,7 +228,7 @@ void SVGStyledElement::parseMappedAttribute(Attribute* attr)
 
 bool SVGStyledElement::isKnownAttribute(const QualifiedName& attrName)
 {
-    return isIdAttributeName(attrName) || attrName == HTMLNames::styleAttr; 
+    return isIdAttributeName(attrName);
 }
 
 void SVGStyledElement::svgAttributeChanged(const QualifiedName& attrName)


I'll work with kliegs on making a regression test and put a patch up for review ASAP.

[James Robinson]

Created attachment 59460 [details]
Patch

[James Robinson]

Patch + reduction attached.  The test case is really complicated, but you need to go through many intermediate states in order to end up with a hosed render tree.  I'd welcome any simplifications but this is as small as I could get it.  From poking at this in GDB, I'm pretty confident that this is hitting the same codepath that Google docs was.

[James Robinson]

Thanks for the prompt review!

[Nikolas Zimmermann]

(In reply to comment #13)
> Thanks for the prompt review!

Good fix! Thanks for looking into that!

[James Robinson]

Comment on attachment 59460 [details]
Patch

Clearing flags on attachment: 59460

Committed r61693: <http://trac.webkit.org/changeset/61693>

[James Robinson]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/61693 might have broken Qt Linux Release

[James Robinson]

Committed r61696: <http://trac.webkit.org/changeset/61696>

[James Robinson]

Had to add a Qt specific result since they format SVG rectangles differently when dumping the render tree.  The test was passing on Qt.  Filed https://bugs.webkit.org/show_bug.cgi?id=41079 about the difference.