Bug 25206 – XMLSerializer().serializeToString() doesn't generate the XML declaration markup like Opera and Firefox

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: XML

[Michael A. Puls II]

WebKit r42344

When you use serializeToString() on an xml document that was generated by parsing xml markup that contained an xml declaration, the xml declaration markup is missing from the returned string.

For example:

var xmlpi = '<?xml version="1.0" encoding="UTF-8"?>';
var markup = xmlpi + '<root><test/></root>';
var xmldoc = new DOMParser().parseFromString(markup, "text/xml");
var smarkup = new XMLSerializer().serializeToString(xmldoc);

In Opera and Firefox, smarkup will begin with '<?xml version="1.0" encoding="UTF-8"?>'. In Safari, it's missing.

Difference between Opera and Firefox though:

1. If the @encoding value of the xml declaration in the parsed markup is lowercase, Firefox will upppercase it for serializeToString(). For example, encoding="utf-8" -> encoding="UTF-8". Opera does not do that and leaves it as-is.

2. If the parsed markup doesn't contain an xml declaration, Firefox won't generate xml declaration markup for serializeToString. Opera will generate '<?xml version="1.0"?>' it seems.

It seems it would be good to at least follow FF here.

[Michael A. Puls II]

Created attachment 29496 [details]
TC. Passes in Firefox and Opera. Fails in Safari.

[Alexey Proskuryakov]

I think that this was discussed in bugzilla before, but I couldn't find any bug.

[Alexey Proskuryakov]

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

[Charles Pritchard]

This bug has been sitting for several years. What's the verdict?

[szyk]

(In reply to comment #4)
> This bug has been sitting for several years. What's the verdict?

It's obvious!!!! This is not bug! This is feature!!!!

[Rob Buis]

Created attachment 133669 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 133669 [details]
Patch

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

Good catch! I have some issues with appendXMLDecl that I'd like to see resolved first:

> Source/WebCore/ChangeLog:8
> +        Serialize the xml declaraion if present in the document.

typo: declaration.

> Source/WebCore/editing/MarkupAccumulator.cpp:299
> +void MarkupAccumulator::appendXMLDecl(StringBuilder& result, const Document* doc)

s/doc/document/.  Also the function should be renamed to something like appendXMLDeclaration, to avoid abbreviations.

> Source/WebCore/editing/MarkupAccumulator.cpp:315
> +    static const char xmlDeclString[] = "<?xml ";
> +    result.append(xmlDeclString, sizeof(xmlDeclString) - 1);
> +    result.append("version=");
> +    result.append('"');
> +    result.append(doc->xmlVersion());
> +    result.append('"');
> +    result.append(" encoding=");
> +    result.append('"');
> +    result.append(encoding);
> +    result.append('"');
> +    result.append("?>");

Using StringBuilder this way is inefficient, if you know off-hand how long the final string is.
I'd advice to switch to use String::operator+, which is most-efficient for this kind of string concatenation.
result.append(String("<?xml " + xmlDeclString + "version="......)
While this may look inefficient on first sight, it only causes one allocation of the final string size, because of the magic behind the scenes when using String::operator+.

If you repeatedly call result.append(), you're growing a Vector of Strings inside the StringBuilder, which is needless.

> Source/WebCore/editing/MarkupAccumulator.cpp:446
> +        appendXMLDecl(result, static_cast<const Document*>(node));

I'd break; here for consistency after calling appendXMLDecl.

> LayoutTests/fast/dom/dom-parse-serialize-xmldecl.html:15
> +        document.body.innerHTML = smarkup.indexOf(xmlpi) === 0 ? "PASS" : "FAIL";

This is fine, but what if there's a typo in the serialization code, we wouldn't' notice this with such a test.
I'd suggest to add an "expectedString" result, and compare that. If it differs, print FAIL, and include the differences.
Easiest is to include fast/js/resources/js-test-pre.js and use the shouldBeEqualToString() functionality for that.

> LayoutTests/fast/dom/dom-parse-serialize-xmldecl.html:19
> +<body onload="runTest();"/>

s/;//

[Rob Buis]

Hi Niko,

(In reply to comment #7)
> (From update of attachment 133669 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133669&action=review
> 
> Good catch! I have some issues with appendXMLDecl that I'd like to see resolved first:
> 
> > Source/WebCore/ChangeLog:8
> > +        Serialize the xml declaraion if present in the document.
> 
> typo: declaration.
> 
> > Source/WebCore/editing/MarkupAccumulator.cpp:299
> > +void MarkupAccumulator::appendXMLDecl(StringBuilder& result, const Document* doc)
> 
> s/doc/document/.  Also the function should be renamed to something like appendXMLDeclaration, to avoid abbreviations.
> 
> > Source/WebCore/editing/MarkupAccumulator.cpp:315
> > +    static const char xmlDeclString[] = "<?xml ";
> > +    result.append(xmlDeclString, sizeof(xmlDeclString) - 1);
> > +    result.append("version=");
> > +    result.append('"');
> > +    result.append(doc->xmlVersion());
> > +    result.append('"');
> > +    result.append(" encoding=");
> > +    result.append('"');
> > +    result.append(encoding);
> > +    result.append('"');
> > +    result.append("?>");
> 
> Using StringBuilder this way is inefficient, if you know off-hand how long the final string is.
> I'd advice to switch to use String::operator+, which is most-efficient for this kind of string concatenation.
> result.append(String("<?xml " + xmlDeclString + "version="......)
> While this may look inefficient on first sight, it only causes one allocation of the final string size, because of the magic behind the scenes when using String::operator+.
> 
> If you repeatedly call result.append(), you're growing a Vector of Strings inside the StringBuilder, which is needless.
> 
> > Source/WebCore/editing/MarkupAccumulator.cpp:446
> > +        appendXMLDecl(result, static_cast<const Document*>(node));
> 
> I'd break; here for consistency after calling appendXMLDecl.
> 
> > LayoutTests/fast/dom/dom-parse-serialize-xmldecl.html:15
> > +        document.body.innerHTML = smarkup.indexOf(xmlpi) === 0 ? "PASS" : "FAIL";
> 
> This is fine, but what if there's a typo in the serialization code, we wouldn't' notice this with such a test.
> I'd suggest to add an "expectedString" result, and compare that. If it differs, print FAIL, and include the differences.
> Easiest is to include fast/js/resources/js-test-pre.js and use the shouldBeEqualToString() functionality for that.
> 
> > LayoutTests/fast/dom/dom-parse-serialize-xmldecl.html:19
> > +<body onload="runTest();"/>
> 
> s/;//

Thanks for the detailed review, I agree on all fronts. Will put up a new patch soon.
Cheers,

Rob.

[Rob Buis]

Created attachment 133680 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 133680 [details]
Patch

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

Still an issue with the test:

> Source/WebCore/editing/MarkupAccumulator.cpp:303
> +    String encoding = document->xmlEncoding();
> +    if (encoding.isEmpty())
> +        return;

Ah, I forgot to ask: why do you only care for encoding here? Can't xmlVersion() return an empty string?
This needs to be commented, otherwise anyone reading this function will have those questions :-)

> LayoutTests/fast/dom/dom-parse-serialize-xmldecl-expected.txt:6
> +PASS smarkup is markup

This is not useful...

> LayoutTests/fast/dom/dom-parse-serialize-xmldecl.html:10
> +        var xmlpi = '<?xml version="1.0" encoding="UTF-8"?>';
> +        var markup = xmlpi + '<root><test/></root>';
> +        var xmldoc = new DOMParser().parseFromString(markup, "text/xml");
> +        var smarkup = new XMLSerializer().serializeToString(xmldoc);
> +        shouldBe("smarkup", "markup");

shouldBeEqualToString("new XMLSerializer().serializeToString(xmldoc)", "<?xml....")
is more useful, as it includes the actual expected output in the test.

[Rob Buis]

Hi Niko,

(In reply to comment #10)
> (From update of attachment 133680 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133680&action=review
> 
> Still an issue with the test:
> 
> > Source/WebCore/editing/MarkupAccumulator.cpp:303
> > +    String encoding = document->xmlEncoding();
> > +    if (encoding.isEmpty())
> > +        return;
> 
> Ah, I forgot to ask: why do you only care for encoding here? Can't xmlVersion() return an empty string?
> This needs to be commented, otherwise anyone reading this function will have those questions :-)

I use it to check whether it is an xml doc basically. Version is required, encoding is optional. I guess my check is not quite right, maybe I should stick to !HTMLDocument? and then later on test for encoding.

> > LayoutTests/fast/dom/dom-parse-serialize-xmldecl-expected.txt:6
> > +PASS smarkup is markup
> 
> This is not useful...
> 
> > LayoutTests/fast/dom/dom-parse-serialize-xmldecl.html:10
> > +        var xmlpi = '<?xml version="1.0" encoding="UTF-8"?>';
> > +        var markup = xmlpi + '<root><test/></root>';
> > +        var xmldoc = new DOMParser().parseFromString(markup, "text/xml");
> > +        var smarkup = new XMLSerializer().serializeToString(xmldoc);
> > +        shouldBe("smarkup", "markup");
> 
> shouldBeEqualToString("new XMLSerializer().serializeToString(xmldoc)", "<?xml....")
> is more useful, as it includes the actual expected output in the test.

Strangely when I try that, no matter how I escape, the comparison does not work :(
Cheers,

Rob.

[WebKit Review Bot]

Comment on attachment 133680 [details]
Patch

Attachment 133680 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12129498

New failing tests:
fast/xsl/xslt-processor.html
http/tests/xmlhttprequest/xml-encoding.html

[WebKit Review Bot]

Created attachment 133692 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Rob Buis]

Created attachment 133711 [details]
Patch

[WebKit Review Bot]

Comment on attachment 133711 [details]
Patch

Attachment 133711 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12128612

New failing tests:
fast/dom/allowed-children.html
fast/xmlhttprequest/xmlhttprequest-get.xhtml
fast/dom/Node/normalize-with-cdata.html
http/tests/xmlhttprequest/xml-encoding.html
fast/xsl/xslt-processor.html
fast/html/xhtml-serialize.html
http/tests/xmlhttprequest/serialize-document.html
fast/dom/dom-parse-serialize.html
fast/dom/serialize-nodes.xhtml
fast/dom/dom-parse-serialize-display.html
fast/dom/Document/replace-child.html
svg/custom/dynamic-svg-document-creation.svg
fast/dom/XMLSerializer.html

[WebKit Review Bot]

Created attachment 133722 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Rob Buis]

Created attachment 133787 [details]
Patch

[WebKit Review Bot]

Comment on attachment 133787 [details]
Patch

Attachment 133787 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12130877

New failing tests:
fast/xsl/xslt-processor.html
http/tests/xmlhttprequest/xml-encoding.html

[WebKit Review Bot]

Created attachment 133793 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Rob Buis]

Hi Niko,

(In reply to comment #10)
> (From update of attachment 133680 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133680&action=review
> 
> > LayoutTests/fast/dom/dom-parse-serialize-xmldecl.html:10
> > +        var xmlpi = '<?xml version="1.0" encoding="UTF-8"?>';
> > +        var markup = xmlpi + '<root><test/></root>';
> > +        var xmldoc = new DOMParser().parseFromString(markup, "text/xml");
> > +        var smarkup = new XMLSerializer().serializeToString(xmldoc);
> > +        shouldBe("smarkup", "markup");
> 
> shouldBeEqualToString("new XMLSerializer().serializeToString(xmldoc)", "<?xml....")
> is more useful, as it includes the actual expected output in the test.

Ok, I have this covered in the new test now, it needed some replace() stuff.
Cheers,

Rob.

[Nikolas Zimmermann]

Comment on attachment 133787 [details]
Patch

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

I'd r+ it if cr-linux was green, but it isn't and clearly indicates a problem. The cr-linux bot attached the layout test results archive, can you inspect the two failing tests?

> Source/WebCore/editing/MarkupAccumulator.cpp:302
> +    // USe encoding as a way to detect that the xml declaration has been explicitly set.
> +    String encoding = document->xmlEncoding();

To assure we never have to touch this code again in MarkupAccumulator, I'd really add bool Document::hasXMLDeclaration() const, and move the xmlEncoding() check there. What do you think?

> LayoutTests/fast/dom/dom-parse-serialize-xmldecl-expected.txt:6
> +PASS new XMLSerializer().serializeToString(xmldoc).replace(/"/g,"'") is "<?xml version='1.0' encoding='UTF-8'?><root><test/></root>"

Looks great and descriptive.

[Rob Buis]

Hi Niko,

(In reply to comment #21)
> (From update of attachment 133787 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133787&action=review
> 
> I'd r+ it if cr-linux was green, but it isn't and clearly indicates a problem. The cr-linux bot attached the layout test results archive, can you inspect the two failing tests?

Yeah these results should simply be updated, I think it is a slight improvement anyway.

> > Source/WebCore/editing/MarkupAccumulator.cpp:302
> > +    // USe encoding as a way to detect that the xml declaration has been explicitly set.
> > +    String encoding = document->xmlEncoding();
> 
> To assure we never have to touch this code again in MarkupAccumulator, I'd really add bool Document::hasXMLDeclaration() const, and move the xmlEncoding() check there. What do you think?

Have to think about it... To be honest the xmlVersion code etc. looks a bit wierd. For instance HTMLDocument has to call clearXMLVersion in the ctor, that does not look optimal to me.

> > LayoutTests/fast/dom/dom-parse-serialize-xmldecl-expected.txt:6
> > +PASS new XMLSerializer().serializeToString(xmldoc).replace(/"/g,"'") is "<?xml version='1.0' encoding='UTF-8'?><root><test/></root>"
> 
> Looks great and descriptive.

Good :) May do a new patch soon depending on workload...
Cheers,

Rob.

[Rob Buis]

Created attachment 134041 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 134041 [details]
Patch

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

This seems like a lot of fixes at once, no?  I worry about your test coverage, but otherwise I like the change.

> Source/WebCore/dom/Document.cpp:1172
> +    if (!implementation()->hasFeature("XML", String()))
> +        return;

Is this tested?

> Source/WebCore/dom/Document.h:1139
> +    Document(Frame*, const KURL&, bool isXHTML, bool isHTML, const String& xmlVersion = String("1.0"));

When does this get initialized?  I worry this is a static initializer?

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:993
> +    if (standalone >= 0)

Is this tested?

[Rob Buis]

Hi Eric,

(In reply to comment #24)
> (From update of attachment 134041 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134041&action=review
> 
> This seems like a lot of fixes at once, no?  I worry about your test coverage, but otherwise I like the change.

Yes, some fixes. Actually the xmlVersion changes are not essential to the fix, it just seems suboptimal to me to set it to 1.0 and then to change your mind and clear it in HTMLDocument ctor :) The rest is needed to have good serialization.

> > Source/WebCore/dom/Document.cpp:1172
> > +    if (!implementation()->hasFeature("XML", String()))
> > +        return;
> 
> Is this tested?

No. But there is no point changing m_hasXmlDeclaration for non-XML. This is internal API, so not quite sure how you want to test.

> > Source/WebCore/dom/Document.h:1139
> > +    Document(Frame*, const KURL&, bool isXHTML, bool isHTML, const String& xmlVersion = String("1.0"));
> 
> When does this get initialized?  I worry this is a static initializer?

Not completely sure what you mean here. HTMLDocument uses emptyString() but otherwise we use the default param. Another way is to do it in the Document ctor based on isHTML:

m_xmlVersion(isHTML ? emptyString() : "1.0"),

Would that be better/more efficient? Or keep the clearXMLVersion? I dont like that function for some reason.

> > Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:993
> > +    if (standalone >= 0)
> 
> Is this tested?

No, I'll add more variants of the xml declaration to test, i.e. standalone and encoding being on or off, version is required.
Cheers,

Rob.

[Rob Buis]

Created attachment 134791 [details]
Patch

[WebKit Review Bot]

Comment on attachment 134791 [details]
Patch

Attachment 134791 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12291273

New failing tests:
fast/dom/Node/normalize-with-cdata.html
fast/html/xhtml-serialize.html
fast/dom/Document/replace-child.html
fast/dom/dom-parse-serialize.html
svg/custom/dynamic-svg-document-creation.svg
fast/dom/dom-parse-serialize-display.html
fast/xmlhttprequest/xmlhttprequest-get.xhtml
fast/dom/XMLSerializer.html

[WebKit Review Bot]

Created attachment 134811 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 134791 [details]
Patch

Attachment 134791 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12266342

New failing tests:
fast/dom/Node/normalize-with-cdata.html
fast/html/xhtml-serialize.html
fast/dom/Document/replace-child.html
fast/dom/dom-parse-serialize.html
svg/custom/dynamic-svg-document-creation.svg
fast/dom/dom-parse-serialize-display.html
fast/xmlhttprequest/xmlhttprequest-get.xhtml
fast/dom/XMLSerializer.html

[WebKit Review Bot]

Created attachment 134819 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Rob Buis]

Created attachment 134983 [details]
Patch

[WebKit Review Bot]

Comment on attachment 134983 [details]
Patch

Attachment 134983 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12262931

New failing tests:
fast/xmlhttprequest/xmlhttprequest-get.xhtml
fast/dom/dom-parse-serialize-display.html
fast/dom/dom-parse-serialize.html

[WebKit Review Bot]

Created attachment 134984 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 134983 [details]
Patch

Attachment 134983 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12291965

New failing tests:
fast/xmlhttprequest/xmlhttprequest-get.xhtml
fast/dom/dom-parse-serialize-display.html
fast/dom/dom-parse-serialize.html

[WebKit Review Bot]

Created attachment 134987 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Rob Buis]

Created attachment 135001 [details]
Patch

[WebKit Review Bot]

Comment on attachment 135001 [details]
Patch

Attachment 135001 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12275053

New failing tests:
fast/xmlhttprequest/xmlhttprequest-get.xhtml

[WebKit Review Bot]

Created attachment 135004 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Rob Buis]

Created attachment 135006 [details]
Patch

[Rob Buis]

(In reply to comment #39)
> Created an attachment (id=135006) [details]
> Patch

I am hoping this latest patch will fix all chromium regressions. Note that for some other platforms there still will need to be rebaselines.

[Csaba Osztrogonác]

Comment on attachment 135006 [details]
Patch

Remove r? temporarily to try yo save Qt EWS bots. :)

[Csaba Osztrogonác]

(In reply to comment #41)
> (From update of attachment 135006 [details])
> Remove r? temporarily to try yo save Qt EWS bots. :)

I'll put r? back after EWSs dropped it from their queue.

[Csaba Osztrogonác]

Comment on attachment 135006 [details]
Patch

Put r? back, because Qt EWS bots dropped this patch from their queues.

[Rob Buis]

Created attachment 136116 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 136116 [details]
Patch

Remove r?, because it broke the Qt EWS bots again. :( I'll set r? back after Qt EWS dropped this patch from their queues.

[Rob Buis]

Hi Ossy,

(In reply to comment #45)
> (From update of attachment 136116 [details])
> Remove r?, because it broke the Qt EWS bots again. :( I'll set r? back after Qt EWS dropped this patch from their queues.

Is the problem still that the bug is too big because of the attachments? I am willing to obsolete the attachments, but I did not do it before since then maybe there is no "testcase" to investigate and fix the problem.
Cheers,

Rob.

[Csaba Osztrogonác]

The problem is caused by the size of this bug ( >100Mb with attachments). Unfortunately EWS isn't to optimal and it tries to download the whole bug 3 times and then EWS dies with OOM. See https://bugs.webkit.org/show_bug.cgi?id=60223 and https://bugs.webkit.org/show_bug.cgi?id=74929 for details.

[Rob Buis]

Created attachment 136146 [details]
Patch

[Rob Buis]

*** This bug has been marked as a duplicate of bug 83432 ***