84430 - <![CDATA[ ]]/> (invalid markup) is not handled correctly, missing content in view source

Status: RESOLVED FIXED

(Core Graveyard :: View Source, defect, P3)

Description

[timeless]

um. one flavor of this is a crasher, but let's start w/ a simpler example.

Comment 1

[timeless]

Attachment: tiny xul file

Comment 2

[timeless]

Attachment: view-source output by mozilla

Comment 3

[timeless]

it should be noted that although mozilla does attempt to butcher the file, it 
doesn't manage to output valid xml.

Comment 4

[Hixie (not reading bugmail)]

We shouldn't even be butchering the file. Invalid XML should not ever attempt
to be rendered.

Sending to the right component.

One can probably simplify the testcase by not using XUL, and just having an 
unclosed CDATA section. But I haven't checked.

Comment 5

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

Attachment: minimal testcase (we deal correctly)

Comment 6

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

Attachment: testcase that CRASHES Mozilla

Comment 7

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

Stack:

nsLoadGroup::RemoveRequest(nsLoadGroup * const 0x03cf3030, nsIRequest *
0x00000000, nsISupports * 0x00000000, unsigned int 0) line 478 + 28 bytes
nsXULDocument::ResumeWalk() line 5000 + 54 bytes
nsXULDocument::EndLoad(nsXULDocument * const 0x057d0a70) line 1562 + 8 bytes
XULContentSinkImpl::DidBuildModel(XULContentSinkImpl * const 0x057d1cd0, int 1)
line 512
CWellFormedDTD::DidBuildModel(CWellFormedDTD * const 0x0577a790, unsigned int 0,
int 1, nsIParser * 0x057d1830, nsIContentSink * 0x057d1cd0) line 296 + 20 bytes
nsParser::DidBuildModel(unsigned int 0) line 1438 + 60 bytes
nsParser::ResumeParse(int 1, int 1) line 1921
nsParser::OnStopRequest(nsParser * const 0x057d1838, nsIRequest * 0x057b05a0,
nsISupports * 0x00000000, unsigned int 0) line 2362 + 19 bytes
nsDocumentOpenInfo::OnStopRequest(nsDocumentOpenInfo * const 0x057a72a0,
nsIRequest * 0x057b05a0, nsISupports * 0x00000000, unsigned int 0) line 255
nsFileChannel::OnStopRequest(nsFileChannel * const 0x057b05a8, nsIRequest *
0x057c7414, nsISupports * 0x00000000, unsigned int 0) line 469 + 41 bytes
nsOnStopRequestEvent::HandleEvent() line 159
nsARequestObserverEvent::HandlePLEvent(PLEvent * 0x057ce0d4) line 64
PL_HandleEvent(PLEvent * 0x057ce0d4) line 590 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00a1de70) line 520 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x21720892, unsigned int 49455, unsigned int 0,
long 10608240) line 1071 + 9 bytes
USER32! 77e71820()
00a1de70()

request parameter is 0, we use it without checkin.

Comment 8

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

Attachment: previous testcase with text/xml mime type, should work

Comment 9

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

This should perhaps be 4 different bugs, but I think we could keep them all in
here, at least for a little while.

a) XUL: fail to report error, attachment 1 [details] [diff] [review]
b) XUL: something fishy so that we send null to nsLoadGroup::RemoveRequest,
   attachment 4 [details] [diff] [review]
c) necko: crash because no null check for parameter, attachment 4 [details] [diff] [review]
d) XML: error does not show content where error, even though location correct,
   attachment 5 [details]

Adding hyatt & darin to Cc.

b) and c) might appear because of bug that causes a) and/or d). But we might
really have 4 different bugs as welll.

Comment 10

[timeless]

actually i already filed seperate bugs for crashes and asserts, waterson owns 
them.

Bug 84439 [e-mail] dereferencing null request in PR_LOGGING [@ 
nsLoadGroup::Rem 
Bug 84440 [e-mail] <![CDATA[ ]]/> can trigger: 
NS_PRECONDITION(mState != eInEpi

I'm sorry i didn't mention that earlier.

since this bug is genericly about us not handling correctly and since it was 
filed first i'll let it track the other two bugs (dependency tree)

but this bug should focus less on asserts or crashes and more on correct 
behaviors.

fwiw, in _exactly one_ instance while i was playing w/ this stuff in winembed 
(w32debug) the console actually warned about bad xml. -- if i come across the 
output again i'll post it here.

Comment 11

[praveen]

data between <CDATA/> is ignored.

Comment 12

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

We no longer crash, lowering severity. However, we now seem to miss some
content, adding dataloss keyword. If you open any of the testcases and view
source, we seem to be missing the contents of the script elements/cdata
sections. Also the test cases with XUL mime type do not show where the error
occurred.

Comment 13

[Rakesh Mishra]

Changing QA contact

Comment 14

[Boris Zbarsky [:bzbarsky]]

The view-source problem is likely due to the fact that view-source uses the HTML
tokenizer, even for XML documents...

Comment 15

[Brant Gurganus]

By the definitions on <http://bugzilla.mozilla.org/bug_status.html#severity> and
<http://bugzilla.mozilla.org/enter_bug.cgi?format=guided>, crashing and dataloss
bugs are of critical or possibly higher severity.  Only changing open bugs to
minimize unnecessary spam.  Keywords to trigger this would be crash, topcrash,
topcrash+, zt4newcrash, dataloss.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Downgrading again, since it looks like we now properly report the CDATA sections
as unteminated.  Is this bug, wfm?

Comment 17

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

This is still a dataloss bug, at least when doing view source:

tiny xul file, missing:
";
    
    function initFunction()
    {
      parent.initPanel("about:blank");
    }

    AddColorPicker(initFunction);

  ]]/"

the others are missing "/" from "]]/>"

Reassigning to view source.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Isn't that just bug 91240?  I don't believe that anything view-source does can
be considered dataloss by the normal definition (no data is ever lost), so
removing that keyword.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Make CDATA not disappear if it is unclosed

This patch is very hacky, because of the view-source specific stuff and it also
fixes bug 236517. Basically, if when we hit EOF and hadn't found the end of the
CDATA section, we were returning an error. Because this was the last chunk of
data, however, the parser would not attempt to tokenize again, and cdata chunk
would fall off the map (as the parser actually built the model and so on). Now,
if the scanner is not incremental, we have no more data coming, so we make do
with what we have. I've scattered 'XXX' comments throughout for places that
will need to be changed/places that are very 'hacky' because of view-source.

The view-source part of the patch simply has us remember the last character in
the CDATA section. Then, nsViewSourceHTML.cpp doesn't need to guess (and
automatically output a '>'). As a note, when I get around to finishing the new
tokenizer, *all* tokens will know their last characters, but until then, this
is a special case.

Just to be clear, this will not change any existing behavior we have now (in
non-view-source mode). CNavDTD.cpp converts CDATA tokens into comment tokens,
so we don't change our rendering of anything.

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 157857 [details] [diff] [review]
Make CDATA not disappear if it is unclosed

bz, tentatively asking you for r+sr=. If you don't have time, just point me to
another reviewer.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 157857 [details] [diff] [review]
Make CDATA not disappear if it is unclosed

r+sr=bzbarsky

Comment 22

[Kenneth Herron]

Checking in nsHTMLTokens.cpp;
/cvsroot/mozilla/parser/htmlparser/src/nsHTMLTokens.cpp,v  <--  nsHTMLTokens.cpp
new revision: 3.250; previous revision: 3.249
done
Checking in nsViewSourceHTML.cpp;
/cvsroot/mozilla/parser/htmlparser/src/nsViewSourceHTML.cpp,v  <-- 
nsViewSourceHTML.cpp
new revision: 1.152; previous revision: 1.151
done

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Marking fixed, as the patch was checked in.