306353 - XHTML parsed as XML with doctype has trilicense/other comments from dtd in DOM

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Bryan Mills]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050815 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050815 Firefox/1.0+

My web server happens to serve files ending in .xml as text/xml, and xhtml may
be served as xml (although that is not recommended).  Deer Park Alpha 2 decides
to parse text/xml documents with an XHTML doctype as XHTML but display it as
XML, and in the process unrolls some internal XML into the document.  The end
result is that my document, which is not in any way covered by the MPL/GPL/LGPL,
is shown as an XML parse tree /with an MPL/GPL block that isn't supposed to be
there/.

It is technically allowable to treat my xml-served XHTML as plain old style-less
XML.  However, if that is the case it should not be combined with internal GPL'd
documents.  Alternatively, it could be displayed and rendered properly as XHTML,
which would be preferable but is not at all required.

See http://www.w3.org/TR/xhtml-media-types/#text-xml .

Representing a non-GPL'd document as a GPL'd document is a potential legal
liability, so this really ought to be fixed, altough it doesn't come up much in
practice.

Reproducible: Always

Steps to Reproduce:
View http://www.andrew.cmu.edu/user/bmills/test.xml .
Actual Results:  
An XML parse tree is shown, including several spurious comments with the MPL/GPL
and other comments not in the actual document.

Expected Results:  
EITHER correctly render the XHTML as XHTML, OR display the correct XML parse
tree WITHOUT spurious comments.

Comment 1

[Phil Ringnalda (:philor)]

Not that we should really be spewing our comments like that, but, if you add the
utterly required xmlns="http://www.w3.org/1999/xhtml" attribute on your html
element, it will work like you expect.

Comment 2

[Phil Ringnalda (:philor)]

Attachment: reporter's testcase

Comment 3

[Bryan Mills]

Just noticed that myself, but since the xmlns is technically required for it to
be XHTML it shouldn't even be parsed as XHTML and the garbage shouldn't show up.
 An error message would even be fine -- "this document has an XHTML doctype but
is not valid XHTML and may contain errors".  Regardless, the comments should
absolutely not be leaking out into a purportedly faithful parse tree, valid
XHTML or not.

Comment 4

[Bryan Mills]

Also, Safari appears to tolerate the missing xmlns, so this may be a
compatibility issue down the road.  Possibly needing a quirks-mode kind of
tolerance?

Comment 5

[Josh Birnbaum]

Marking as a regression, I don't see the extra nodes in Moz 1.7.11/Windows

Comment 6

[Peter Van der Beken [:peterv]]

The comments are coming from xhtml11.dtd. Maybe we should strip them with the
preprocessor (same for mathml.dtd).

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

what do you mean with "parsed as XHTML"? the file is parsed as XML like any
other XML mime type...

Comment 8

[Bryan Mills]

Well, during parsing the system treats it as XHTML and not just XML, because the
XHTML DTD is what's causing the spurious comments.

So I suppose the real problem is that comments from the local DTD are being
shown in the parse tree of non-local markup that references a corresponding DTD,
in this case the Firefox version of the xhtml DTD.

Comment 9

[dolphinling]

Nearly filed my own bug on this...

The missing xmlns doesn't matter, if you look in the DOM Inspector they're there
even in ones with an xmlns. Also, it also affects application/xhtml+xml documents.

It doesn't matter if the file is local or not, I've seen it either way. 

The only thing that does seem to matter is if the doctype declaration is there.

I'll work on finding a regression range. I suspect it's in the past 2-3 days
max, though.

Comment 10

[dolphinling]

Seems I was wrong on the regression: it was between 2005-05-13 and 2005-05-14.
Bonsai points to bug 288133. ccing bzbarsky as he made those changes.

Changing summary to more accurately reflect the problem.

Comment 11

[Peter Van der Beken [:peterv]]

The regression range isn't that important, this is probably fallout from
updating Expat. We're now doing the right thing (including the comments from a
DTD), we just need to strip the license from the dtd's that we ship.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Hmm... are we really supposed to include comments and text from an external DTD
in the main DOM?  That seems odd...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: I think we want this behavior

It really makes no sense to put comments and newlines from an external DTD into
the main page DOM...  I think we should just ignore those callbacks altogether
while we're parsing an external DTD.

Comment 14

[Peter Van der Beken [:peterv]]

Comment on attachment 194367 [details] [diff] [review]
I think we want this behavior

As discussed, let's add this to PIs too.

Comment 15

[Peter Van der Beken [:peterv]]

XML Information Set does specify this for comments, but not for PIs: "There is a
comment information item for each XML comment in the original document, except
for those appearing in the DTD (which are not represented)." This either means
they forgot or we need to handle PIs in internal/external subsets :-(.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: More comprehensive

I realized that the comments part wouldn't reserialize correctly as it was
written too...

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 194413 [details] [diff] [review]
More comprehensive

I think we want this for 1.8b.	Without this patch, random parts of external
DTDs can end up in a page DOM.

Comment 18

[Mike Schroepfer]

Bz,

Can we get a risk-assement on this bug this late in the game?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

schrep, the risk is fairly low.  The only codepaths affected are:

1)  Loading of an external DTD that contains comments, whitespace or PIs
2)  Loading of an internal subset that contains comments or PIs

The former case only happens with our in-tree DTDs, since we don't generally
load external DTDs.  In all cases, the only real change is to not create nodes
in the DOM for the stuff from the DTD, which is effectively a regression fix.  I
don't see this fix breaking anything.

Comment 20

[Mike Schroepfer]

Comment on attachment 194413 [details] [diff] [review]
More comprehensive

Thnx for the analysis bz.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Fixed on branch.

Comment 22

[Dave Townsend [:mossop]]

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