Bug 19215 – REGRESSION: transformToDocument fails when xsl includes  

• Status: RESOLVED
• Resolution: FIXED
• Component: XML

[Duncan Booth]

I have an XSL stylesheet which includes an entity escape for a non-break space. Prior to webkit nightly build r32877 this worked, but from r32901 onwards it fails (transformToDocument returns null). Simply removing the   from the XSL file makes it work again.

[Duncan Booth]

Created attachment 21311 [details]
Javascript to create xml and xsl and demonstrate the problem

[Duncan Booth]

Created attachment 21312 [details]
HTML to load the previously attached javascript

[Alexey Proskuryakov]

Sounds related to <http://trac.webkit.org/projects/webkit/changeset/32879>.

Confirming (although I haven't attempted to actually reproduce yet).

[Alexey Proskuryakov]

The problem is that we replace   with   when re-serializing the stylesheet for XSLT, which makes it invalid.

We shouldn't do this for XML. Maybe we should match Firefox and also avoid doing this for HTML in cases other than innerHTML, but so far, we've been able to get away with having innerHTML behave identically to XMLSerializer, XSLT and XMLHttpRequest serialization etc.

[Alexey Proskuryakov]

Created attachment 21358 [details]
proposed fix

[Darin Adler]

Comment on attachment 21358 [details]
proposed fix

+    Vector<UChar> s;
+    appendEscapedContent(s, make_pair(in.characters(), in.length()), escapeNBSP);
+    return String::adopt(s);

I would use a longer name than "s" for the string buffer.

Do we want to have a path where we reuse the string when there is no text to escape?

+            // FIXME: Comment content is not escaped, but some APIs calling this should raise an exception if it includes "-->".

I don't know what "some APIs calling this" refers to.

+        case Node::DOCUMENT_TYPE_NODE: {
+            const DocumentType* n = static_cast<const DocumentType*>(node);

Moving the code inside this file seems fine, but putting it all inside the case statement instead of factored into a separate function seems like a step backwards.

     if (Document *doc = document())
         if (DocumentType *doctype = doc->doctype())
-            return doctype->toString();
+            return createMarkup(doctype);
 
     return String();

Should use early returns instead of nested ifs. Or add braces if leaving this nested.

r=me

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/34197>.

(In reply to comment #6)
> Do we want to have a path where we reuse the string when there is no text to
> escape?

As far as I can tell, this function is not used in any hot code paths - although I'm not sure if I know all of its uses.

> I don't know what "some APIs calling this" refers to.

Reworded to mention one.

> Moving the code inside this file seems fine, but putting it all inside the case
> statement instead of factored into a separate function seems like a step
> backwards.

Factored out document type case into a separate function - the others seem small enough to stay inside the switch.

[Alexey Proskuryakov]

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