Bug 87504 - REGRESSION (r112277): use/xlink doesn't work correctly
: REGRESSION (r112277): use/xlink doesn't work correctly
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: SVG
: 528+ (Nightly build)
: Unspecified Unspecified
: P1 Normal
Assigned To: Nobody
: Regression
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-25 08:17 PDT by Philip Rogers
Modified: 2012-08-01 12:51 PDT (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Philip Rogers 2012-05-25 08:17:57 PDT
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
Comment 1 Florin Malita 2012-05-25 08:35:18 PDT
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.
Comment 2 Alexey Proskuryakov 2012-05-25 10:27:34 PDT
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 Philip Rogers 2012-05-25 10:41:48 PDT
This is affecting the German puzzle site in the crbug repro, and probably others. Schenney and I are on this bug.
Comment 4 Florin Malita 2012-05-25 11:40:47 PDT
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 Philip Rogers 2012-05-25 12:27:36 PDT
Created attachment 144122 [details]
Fix for self-closing <use> tags

This patch is really by three people: schenney, fmalita, and pdr.
Comment 6 Florin Malita 2012-05-25 12:35:13 PDT
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&);
Comment 7 Philip Rogers 2012-05-25 12:52:30 PDT
Created attachment 144127 [details]
Fix for self-closing <use> tags

Florin is right and his patch is better.

PTAL!
Comment 8 Adam Barth 2012-05-25 16:08:50 PDT
Comment on attachment 144127 [details]
Fix for self-closing <use> tags

Yep.  Thanks for the patch.
Comment 9 WebKit Review Bot 2012-05-25 17:18:45 PDT
Comment on attachment 144127 [details]
Fix for self-closing <use> tags

Clearing flags on attachment: 144127

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