Bug 87504 – REGRESSION (r112277): use/xlink doesn't work correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Philip Rogers]

Created attachment 144074 [details]
Minimized testcase

Between Chrome 19 and Chrome 20 we had a regression in use/xlink (see the attached testcase).

Original bug: crbug.com/129556

[Florin Malita]

Created attachment 144078 [details]
A non-path test case

Not limited to <path> and not related to <g>. Works in SVG docs, appears to fail when embedded in HTML.

[Alexey Proskuryakov]

Bisected to r112329, which was a re-land of <http://trac.webkit.org/changeset/112277>

Is this known to affect any real sites?

[Philip Rogers]

This is affecting the German puzzle site in the crbug repro, and probably others. Schenney and I are on this bug.

[Florin Malita]

I think I got it - Stephen's change exposed a parser problem: 
HTMLConstructionSite::insertForeignElement() doesn't handle self-closing tags correctly (doesn't call finishParsingChildren like XMLTreeBuilder does), hence the <use> instance tree never gets built (after the change, buildPendingResources is triggered from finishParsingChildren() when m_wasInsertedByParser == true).

Making the use tag explicitly closed works around the issue: <use ...></use>

I'll work on a patch, but will probably be next week.

[Philip Rogers]

Created attachment 144122 [details]
Fix for self-closing <use> tags

This patch is really by three people: schenney, fmalita, and pdr.

[Florin Malita]

Comment on attachment 144122 [details]
Fix for self-closing <use> tags

View in context: https://bugs.webkit.org/attachment.cgi?id=144122&action=review

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:363
> +        element->finishParsingChildren();

Note that there's logic in HTMLConstructionSite::attachLater to handle this - all we need to do is set the self closing flag on the task. I'm also not convinced it's safe to call finishParsingChildren here because of the deferred attach. Here's my take:

diff --git a/Source/WebCore/html/parser/HTMLConstructionSite.cpp b/Source/WebCore/html/parser/HTMLConstructionSite.cpp
index 04e4606..4da7a7a 100644
--- a/Source/WebCore/html/parser/HTMLConstructionSite.cpp
+++ b/Source/WebCore/html/parser/HTMLConstructionSite.cpp
@@ -106,11 +106,12 @@ static inline void executeTask(HTMLConstructionSiteTask& task)
         task.child->finishParsingChildren();
 }
 
-void HTMLConstructionSite::attachLater(ContainerNode* parent, PassRefPtr<Node> prpChild)
+void HTMLConstructionSite::attachLater(ContainerNode* parent, PassRefPtr<Node> prpChild, bool selfClosing)
 {
     HTMLConstructionSiteTask task;
     task.parent = parent;
     task.child = prpChild;
+    task.selfClosing = selfClosing;
 
     if (shouldFosterParent()) {
         fosterParent(task.child);
@@ -315,11 +316,10 @@ void HTMLConstructionSite::insertHTMLElement(AtomicHTMLToken& token)
 void HTMLConstructionSite::insertSelfClosingHTMLElement(AtomicHTMLToken& token)
 {
     ASSERT(token.type() == HTMLTokenTypes::StartTag);
-    attachLater(currentNode(), createHTMLElement(token));
     // Normally HTMLElementStack is responsible for calling finishParsingChildren,
     // but self-closing elements are never in the element stack so the stack

     // doesn't get a chance to tell them that we're done parsing their children.
-    m_attachmentQueue.last().selfClosing = true;
+    attachLater(currentNode(), createHTMLElement(token), true);
     // FIXME: Do we want to acknowledge the token's self-closing flag?
     // http://www.whatwg.org/specs/web-apps/current-work/multipage/tokenization.html#acknowledge-self-closing-flag
 }
@@ -355,9 +355,7 @@ void HTMLConstructionSite::insertForeignElement(AtomicHTMLToken& token, const At
     notImplemented(); // parseError when xmlns or xmlns:xlink are wrong.
 
     RefPtr<Element> element = createElement(token, namespaceURI);
-    attachLater(currentNode(), element);
-    // FIXME: Don't we need to set the selfClosing flag on the task if we're
-    // not going to push the element on to the stack of open elements?
+    attachLater(currentNode(), element, token.selfClosing());
     if (!token.selfClosing())
         m_openElements.push(element.release());
 }
diff --git a/Source/WebCore/html/parser/HTMLConstructionSite.h b/Source/WebCore/html/parser/HTMLConstructionSite.h
index 6e28d3a..2790664 100644
--- a/Source/WebCore/html/parser/HTMLConstructionSite.h
+++ b/Source/WebCore/html/parser/HTMLConstructionSite.h
@@ -155,7 +155,7 @@ private:
     // tokens produce only one DOM mutation.
     typedef Vector<HTMLConstructionSiteTask, 1> AttachmentQueue;

 
-    void attachLater(ContainerNode* parent, PassRefPtr<Node> child);
+    void attachLater(ContainerNode* parent, PassRefPtr<Node> child, bool selfClosing = false);
 
     void findFosterSite(HTMLConstructionSiteTask&);

[Philip Rogers]

Created attachment 144127 [details]
Fix for self-closing <use> tags

Florin is right and his patch is better.

PTAL!

[Adam Barth]

Comment on attachment 144127 [details]
Fix for self-closing <use> tags

Yep.  Thanks for the patch.

[WebKit Review Bot]

Comment on attachment 144127 [details]
Fix for self-closing <use> tags

Clearing flags on attachment: 144127

Committed r118589: <http://trac.webkit.org/changeset/118589>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Philip Rogers]

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

[Bob Wyttenbach]

Fixed in today's version of Chrome (21.0.1180.57), which uses WebKit 537.1. 
Newly broken in yesterday's version of Safari (6), which uses WebKit 536.25.

[Stephen Chenney]

(In reply to comment #12)
> Fixed in today's version of Chrome (21.0.1180.57), which uses WebKit 537.1. 
> Newly broken in yesterday's version of Safari (6), which uses WebKit 536.25.

At this point you may wish to file the bug against Safari. For whatever reason it seems they did not pick up this WebKit fix.