Bug 87504 - REGRESSION (r112277): use/xlink doesn't work correctly
: REGRESSION (r112277): use/xlink doesn't work correctly
Status: RESOLVED FIXED
: WebKit
SVG
: 528+ (Nightly build)
: Unspecified Unspecified
: P1 Normal
Assigned To:
:
: Regression
:
:
  Show dependency treegraph
 
Reported: 2012-05-25 08:17 PST by
Modified: 2012-08-01 12:51 PST (History)


Attachments
Minimized testcase (356 bytes, text/html)
2012-05-25 08:17 PST, Philip Rogers
no flags Details
A non-path test case (239 bytes, text/html)
2012-05-25 08:35 PST, Florin Malita
no flags Details
Fix for self-closing <use> tags (3.35 KB, patch)
2012-05-25 12:27 PST, Philip Rogers
no flags Review Patch | Details | Formatted Diff | Diff
Fix for self-closing <use> tags (5.47 KB, patch)
2012-05-25 12:52 PST, Philip Rogers
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-05-25 08:17:57 PST
Created an attachment (id=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
------- Comment #1 From 2012-05-25 08:35:18 PST -------
Created an attachment (id=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.
------- Comment #2 From 2012-05-25 10:27:34 PST -------
Bisected to r112329, which was a re-land of <http://trac.webkit.org/changeset/112277>

Is this known to affect any real sites?
------- Comment #3 From 2012-05-25 10:41:48 PST -------
This is affecting the German puzzle site in the crbug repro, and probably others. Schenney and I are on this bug.
------- Comment #4 From 2012-05-25 11:40:47 PST -------
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.
------- Comment #5 From 2012-05-25 12:27:36 PST -------
Created an attachment (id=144122) [details]
Fix for self-closing <use> tags

This patch is really by three people: schenney, fmalita, and pdr.
------- Comment #6 From 2012-05-25 12:35:13 PST -------
(From update of attachment 144122 [details])
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&);
------- Comment #7 From 2012-05-25 12:52:30 PST -------
Created an attachment (id=144127) [details]
Fix for self-closing <use> tags

Florin is right and his patch is better.

PTAL!
------- Comment #8 From 2012-05-25 16:08:50 PST -------
(From update of attachment 144127 [details])
Yep.  Thanks for the patch.
------- Comment #9 From 2012-05-25 17:18:45 PST -------
(From update of attachment 144127 [details])
Clearing flags on attachment: 144127

Committed r118589: <http://trac.webkit.org/changeset/118589>
------- Comment #10 From 2012-05-25 17:18:50 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #11 From 2012-07-17 16:46:17 PST -------
*** Bug 91498 has been marked as a duplicate of this bug. ***
------- Comment #12 From 2012-08-01 12:31:25 PST -------
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.
------- Comment #13 From 2012-08-01 12:51:48 PST -------
(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.