Closed Bug 88354 Opened 24 years ago Closed 24 years ago

Foreign processing instructions in XML halt rendering

Categories

(Core :: XML, defect, P1)

x86
Windows NT
defect

Tracking

()

VERIFIED FIXED
mozilla0.9.3

People

(Reporter: decoy, Assigned: hjtoi-bugzilla)

References

()

Details

(Keywords: regression, Whiteboard: [PDT-][nsBranch+][fixed and verified on the trunk])

Attachments

(2 files)

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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Target Milestone: --- → mozilla0.9.3
*** Bug 88272 has been marked as a duplicate of this bug. ***
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).
Severity: normal → major
Status: NEW → ASSIGNED
Keywords: regression
Whiteboard: [fixinhand]
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.
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).
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?
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...
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.
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.
nsBranch ok, really important for our XML support.
Keywords: nsBranch
r=peterv.
r=harishd.
Priority: P2 → P1
sr=jst
Fix checked in. Please verify this by Thursday afternoon because I'd like to get this in for RTM.
really fixed
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
The above link renders fine. Verified on July 3rd trunk build.
Status: RESOLVED → VERIFIED
Meant to say: Verified with July 5th trunk build. Btw, the previous comment was from me not from vidur :)
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+.
Whiteboard: [fixinhand] → [nsBranch+][fixed and verified on the trunk]
Re-opening bug until the fix goes on the branch...
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
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-
Whiteboard: [nsBranch+][fixed and verified on the trunk] → [PDT-][nsBranch+][fixed and verified on the trunk]
Status: REOPENED → RESOLVED
Closed: 24 years ago24 years ago
Resolution: --- → FIXED
Marking fixed because this has been fixed on the trunk.
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?
Marking verified fixed in the Sept 06th build (2001-09-06-05).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: