88354 - Foreign processing instructions in XML halt rendering

Status: VERIFIED FIXED

(Core :: XML, defect, P1)

Description

[Sampo Syreeni]

In the referenced XML document, I use a processing instruction unknown to
Mozilla (<?filedate?>). As long as it is present, Mozilla does parse the
document (browser window gets the correct title) but never renders it -- the
browser content area goes unresponsive. Foreign PI's should be silently ignored.
In versions upto 0.8 this did not happen, so it might have something to do with
the changes being done into the handling of <?xml-stylesheet?>.

I'm not quite sure if this should be XML or style, reassign as appropriate.

Comment 1

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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

Comment 2

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Proposed fix 1

Comment 3

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Peter, I think you broke this with the dynamic style landing. Could you review
the fix?

The current code does a QI to style linking element and if it succeeded it will
try to do style processing. For XML PI content, this QI will always succeed. I
added a check that it really is the xml-stylesheet PI before we do the style
processing.

The patch does a little cleanup as well. mInScript is not even used anymore, for
example.

Sampo: You might want to know that your page tries to set non-XHTML namespaces
on XHTML elements, but currently that is not supported. It is bug 41983. You
will notice this with a debug Mozilla build (assertions).

Comment 4

[Peter Van der Beken [:peterv]]

Heikki, have you tried out this fix? I haven't, but the style loading doesn't
seem to happen for the last PI in my build. The GetStyleSheetInfo should prevent
stylesheet loading from happening
(http://lxr.mozilla.org/seamonkey/source/content/xml/content/src/nsXMLProcessingInstruction.cpp#526
and
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsStyleLinkElement.cpp#209).
The only bad thing is mStyleSheetCount gets incremented
(http://lxr.mozilla.org/seamonkey/source/content/xml/document/src/nsXMLContentSink.cpp#1259),
but afai can see, that should not cause any problems.

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Yes, I tested this. It does seem a bit weird that this is needed since there is
the code you pointed out... Maybe we somehow still end up in some bad state.
I'll try to investigate a little. 

In any case, the stylesheet counter being out of whack seems bad too, since
later someone might be relying on it being correct. This patch would fix both
(and in fact this patch should make code a bit faster). 

Comment 6

[Sampo Syreeni]

Sounded interesting, went to code, suspect
http://lxr.mozilla.org/seamonkey/source/content/xml/document/src/nsXMLContentSink.cpp#1266
 -- we assume there is a style="something" attribute in there, dunno what
happens then. Changing the PI in the source to <?filedate style="text/css"?>
causes rendering to work. There might be other similar assumptions in the
following check for XSL/CSS stylesheets.

PI's do not have structure so the parser will not catch stuff which does not
look like the content model of a start tag. <?filedate=="?> is probably valid
XML, and should be expected. Code for <?xml-stylesheet?> and similar PI's which
*are* supposed to have a start tag like syntax should validate for it first, and
only after that assume that the content is something other than
plaintext-absent-PI-end-sequence.

Does the XML parser have interfaces to do this sort of fragment fragmentary
validation, matching strings against the smaller subproductions of the XML spec?

Comment 7

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Good catch. The return value of GetQuotedAttributeValue() is NS_ERROR_FALSE if
attribute was not found, NS_OK otherwise... There is a bug on that function that
it should not return nsresult error for not found case. I'll check on this as
well... 

Comment 8

[Peter Van der Beken [:peterv]]

Heikki, I guess your change is ok. I tried to move all the code for styleloading
in one place (though even more work could be done on that), this patch spreads
it out a bit again :(. As to the index, from looking through the CSS loader I
have never found a real problem with the index being incremented too much. In
fact, I think the old style loading code had the same "problem" in some similar
cases.

Comment 9

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Ok, the real problem was the return value from GetQuotedAttributeValue(), just
making sure we do not propagate that out of the function fixes this. I'll attach
a new patch.

Comment 10

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Proposed fix 2

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

nsBranch ok, really important for our XML support.

Comment 12

[Peter Van der Beken [:peterv]]

r=peterv.

Comment 13

[harishd]

r=harishd.

Comment 14

[Johnny Stenback (:jst)]

sr=jst

Comment 15

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Fix checked in. Please verify this by Thursday afternoon because I'd like to get
this in for RTM.

Comment 16

[Heikki Toivonen (remove -bugzilla when emailing directly)]

really fixed

Comment 17

[vidur (gone)]

The above link renders fine. Verified on July 3rd trunk build.

Comment 18

[harishd]

Meant to say:

Verified with July 5th trunk build.

Btw, the previous comment was from me not from vidur :)

Comment 19

[Nisheeth Ranjan]

This bug severely affects our XML support.  Any page with a foreign processing
instruction will not render.  Existing XML editors insert PIs into XML documents
to store editor specific information into the document.  With this bug in our
product, we will fail to display all such edited XML documents.

The fix is simple and safe and has been verified on the trunk.  Marking nsBranch+.

Comment 20

[Nisheeth Ranjan]

Re-opening bug until the fix goes on the branch...

Comment 21

[Nisheeth Ranjan]

The PDT was of the opinion that this does not meet the bar for inclusion into
Netscpape 6.1 at this late stage of the game.

Marking PDT-

Comment 22

[Nisheeth Ranjan]

Marking fixed because this has been fixed on the trunk.

Comment 23

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Ouch, this cripples our XML support. If the PDT is afraid of the size of the
fix, here is a one line fix for this:

-    result = nsParserUtils::GetQuotedAttributeValue(text,
NS_ConvertASCIItoUCS2("type"), type);
+    nsParserUtils::GetQuotedAttributeValue(text, NS_ConvertASCIItoUCS2("type"),
type);

i.e. just removing "result = " from the beginning of one line will fix this.
Could we still get this on the branch?

Comment 24

[Chris Petersen]

Marking verified fixed in the Sept 06th build (2001-09-06-05).