Bug 15290 – REGRESSION (r14184-r14216): Duplicate DOCTYPE definitions when saving files as webarchives

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Eric Seidel (no email)]

Duplicate DOCTYPE definitions when saving SVGs as WebArchives

Save an SVG as a WebArchive and you end up with a duplicate DOCTYPE definition.  This then breaks parsing when trying to open the archive.

[Eric Seidel (no email)]

Created attachment 16405 [details]
an example busted archive

[Eric Seidel (no email)]

Created attachment 16406 [details]
minimal SVG file to reproduce the problem

[Eric Seidel (no email)]

Created attachment 16407 [details]
minimal example busted SVG archive

[Eric Seidel (no email)]

I believe this is caused by this line in markup.cpp:

        case Node::DOCUMENT_NODE: {
            // Documents do not normally contain a docType as a child node, force it to print here instead.
            const DocumentType* docType = static_cast<const Document*>(node)->doctype();
            if (docType)
                return docType->toString().deprecatedString();
            return "";
        }

[Eric Seidel (no email)]

This actually applies to HTML as well, it just doesn't break the document, since the HTML parser is more forgiving.  This may be a regression from 2.0 (likely, since we have made major improvements to XML serialization since then).

[Eric Seidel (no email)]

Created attachment 16408 [details]
minimal example html file

[Eric Seidel (no email)]

Created attachment 16409 [details]
minimal example html archive (still loads, just is clearly serialized incorrectly)

[Eric Seidel (no email)]

So I tested with Safari 2.0 and this is actually a regression.  However it only affects XHTML (since SVG didn't work in Safari 2) so I'm not sure anyone cares.  I've added the keyword (since it is technically a regression), but someone else can certainly remove it given the only-affecting-xhtml circumstances.

[Eric Seidel (no email)]

Actually, the bug seems to be from the use of:
_stringWithDocumentTypeStringAndMarkupString

which also pre-pends the DOCTYPE to the beginning of the file as well as createMarkup (when printing the DocumentNode).

[David Kilzer (:ddkilzer)]

Unfortunately, it appears that the method that DumpRenderTree uses to create webarchives is not the same method that Safari uses.

(In reply to comment #8)
> So I tested with Safari 2.0 and this is actually a regression.  However it only
> affects XHTML (since SVG didn't work in Safari 2) so I'm not sure anyone cares.

Actually, it affects any HTML or XML document with a pre-existing DOCTYPE.

[David Kilzer (:ddkilzer)]

An autospade run says:

Works: r14184  Fails: r14216

[David Kilzer (:ddkilzer)]

* STEPS TO REPRODUCE
1. Launch Safari/WebKit.
2. Open Attachment 16408 [details].
3. Save as webarchive.
4. Open saved webarchive.
5. View source of page.

* EXPECTED RESULTS
There should be only one "<!DOCTYPE>" tag.

* ACTUAL RESULTS
There are two "<!DOCTYPE>" tags at the top of the file.

[David Kilzer (:ddkilzer)]

<rdar://problem/5509173>

[David Kilzer (:ddkilzer)]

(In reply to comment #11)
> An autospade run says:
> Works: r14184  Fails: r14216

Revision r14204 looks most suspicious.

http://trac.webkit.org/projects/webkit/changeset/14204

[Eric Seidel (no email)]

I don't think it was 14204.  I had a patch locally on my system to remove "realDocType()" as it's the same as the default implementation of docType() (which is no longer overridden by anyone and thus realDocType() doesn't need to exist)

[David Kilzer (:ddkilzer)]

Created attachment 22066 [details]
Patch v1

Proposed fix.  Tested both the SVG and HTML examples.  Layout tests pass as well.

Comment on attachment 22066 [details]
Patch v1

r=me

[David Kilzer (:ddkilzer)]

Comment on attachment 22066 [details]
Patch v1

Clearing review flag for now.  I think I may need to check to see that the "first" node is a DOCTYPE node since this method may be called for any arbitrary node in the DOM.

[David Kilzer (:ddkilzer)]

Created attachment 22087 [details]
Patch v2

Added checks for the node being a DOCUMENT_NODE or DOCUMENT_TYPE_NODE and only prepending the document type if the node type is NOT one of those.

Comment on attachment 22087 [details]
Patch v2

r=me

[David Kilzer (:ddkilzer)]

Committed r35006

[David Kilzer (:ddkilzer)]

Fixed ChangeLog entry in r35014.