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

RESOLVED FIXED

Status

P3
normal
RESOLVED FIXED
18 years ago
8 years ago

People

(Reporter: timeless, Assigned: mrbkap)

Tracking

({testcase})

Trunk
x86
Windows 2000
testcase
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Hixie-P2])

Attachments

(6 attachments)

(Reporter)

Description

18 years ago
um. one flavor of this is a crasher, but let's start w/ a simpler example.
(Reporter)

Comment 1

18 years ago
Created attachment 37482 [details]
tiny xul file
(Reporter)

Comment 2

18 years ago
Created attachment 37483 [details]
view-source output by mozilla
(Reporter)

Comment 3

18 years ago
it should be noted that although mozilla does attempt to butcher the file, it 
doesn't manage to output valid xml.
Keywords: testcase
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.
Assignee: harishd → heikki
Component: Parser → XML
Keywords: qawanted
QA Contact: bsharma → petersen
Created attachment 37580 [details]
minimal testcase (we deal correctly)
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.
Severity: normal → critical
Created attachment 37588 [details]
previous testcase with text/xml mime type, should work
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.

(Reporter)

Comment 10

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

Bug 84439 waterson@netscape.com dereferencing null request in PR_LOGGING [@ 
nsLoadGroup::Rem 
Bug 84440 waterson@netscape.com <![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.
Depends on: 84439, 84440
Keywords: crash
Priority: -- → P3
Target Milestone: --- → mozilla1.0

Comment 11

17 years ago
data between <CDATA/> is ignored.
Whiteboard: [Hixie-P2]
Target Milestone: mozilla1.0 → mozilla0.9.9
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.
Severity: critical → major
Keywords: qawanted → dataloss
Keywords: nsbeta1 → nsbeta1-
Target Milestone: mozilla0.9.9 → mozilla1.0
Target Milestone: mozilla1.0 → mozilla1.0.1

Comment 13

17 years ago
Changing QA contact
QA Contact: petersen → rakeshmishra
The view-source problem is likely due to the fact that view-source uses the HTML
tokenizer, even for XML documents...
Target Milestone: mozilla1.0.1 → mozilla1.2alpha
Keywords: nsbeta1- → nsbeta1+
Target Milestone: mozilla1.2alpha → mozilla1.2beta
Target Milestone: mozilla1.2beta → mozilla1.3alpha
Target Milestone: mozilla1.3alpha → mozilla1.3beta
Keywords: nsbeta1+ → nsbeta1-
Target Milestone: mozilla1.3beta → mozilla1.4alpha

Comment 15

16 years ago
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.
Severity: major → critical
Downgrading again, since it looks like we now properly report the CDATA sections
as unteminated.  Is this bug, wfm?
Severity: critical → normal
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.
Assignee: heikki → doron
Component: XML → ViewSource
QA Contact: rakeshmishra → pmac
Summary: <![CDATA[ ]]/> is not handled correctly → <![CDATA[ ]]/> (invalid markup) is not handled correctly, missing content in view source
Target Milestone: mozilla1.4alpha → ---
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.
Depends on: 91240
Keywords: dataloss

Updated

15 years ago
Blocks: 244725
Created attachment 157857 [details] [diff] [review]
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.
Assignee: doronr → mrbkap
Status: NEW → ASSIGNED
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.
Attachment #157857 - Flags: superreview?(bzbarsky)
Attachment #157857 - Flags: review?(bzbarsky)
Comment on attachment 157857 [details] [diff] [review]
Make CDATA not disappear if it is unclosed

r+sr=bzbarsky
Attachment #157857 - Flags: superreview?(bzbarsky)
Attachment #157857 - Flags: superreview+
Attachment #157857 - Flags: review?(bzbarsky)
Attachment #157857 - Flags: review+

Comment 22

14 years ago
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
Marking fixed, as the patch was checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Product: Browser → Seamonkey
(Reporter)

Updated

8 years ago
Component: View Source → View Source
Product: SeaMonkey → Core Graveyard
You need to log in before you can comment on or make changes to this bug.