Last Comment Bug 362547 - Crash in SAXReader with doctype missing public, system ID's
: Crash in SAXReader with doctype missing public, system ID's
Status: RESOLVED FIXED
: crash, fixed1.8.1.2, testcase
Product: Core
Classification: Components
Component: XML (show other bugs)
: Trunk
: x86 Mac OS X
: -- critical (vote)
: mozilla1.9alpha1
Assigned To: Alex Vincent [:WeirdAl]
: Ashish Bhatt
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-12-01 15:01 PST by Alex Vincent [:WeirdAl]
Modified: 2007-01-16 19:39 PST (History)
3 users (show)
dveditz: wanted1.8.1.x+
sayrer: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
gdb stack trace (12.88 KB, text/plain)
2006-12-01 15:06 PST, Alex Vincent [:WeirdAl]
no flags Details
testcase, part 1 of 2 (XML document) (22 bytes, application/xml)
2006-12-01 15:09 PST, Alex Vincent [:WeirdAl]
no flags Details
testcase, part 2 of 2 (xpcshell script) (5.75 KB, application/javascript)
2006-12-01 15:29 PST, Alex Vincent [:WeirdAl]
no flags Details
patch, v1 (4.55 KB, patch)
2006-12-01 17:06 PST, Alex Vincent [:WeirdAl]
sayrer: review+
bzbarsky: superreview+
jaymoz: approval1.8.1.2+
Details | Diff | Splinter Review
xpcshell unit test (8.99 KB, patch)
2006-12-06 16:59 PST, Alex Vincent [:WeirdAl]
sayrer: review+
Details | Diff | Splinter Review

Description Alex Vincent [:WeirdAl] 2006-12-01 15:01:49 PST
In trying to parse a XML document with:

<!DOCTYPE bindings>

I crashed in nsCharTraits<unsigned short>::length because (s=0x0).
nsSAXXMLReader::HandleStartDTD(), with a lexical handler set, joyfully calls for nsDependentString(nsnull), and kaboom.

Stack, testcase coming up.
Comment 1 Alex Vincent [:WeirdAl] 2006-12-01 15:06:40 PST
Created attachment 247230 [details]
gdb stack trace
Comment 2 Alex Vincent [:WeirdAl] 2006-12-01 15:09:58 PST
Created attachment 247231 [details]
testcase, part 1 of 2 (XML document)
Comment 3 Robert Sayre 2006-12-01 15:20:57 PST
Shoot, I thought we got all of these.
Comment 4 Alex Vincent [:WeirdAl] 2006-12-01 15:29:28 PST
Created attachment 247234 [details]
testcase, part 2 of 2 (xpcshell script)

Steps to reproduce:
(1) Save the xpcshell script locally.
(2) gdb xpcshell (you need the debugger to get the stack trace) and run
(3) load the script into xpcshell.
(4) runTest();
(5) Wait about ten seconds; the script does do a fetch of the XML document from Bugzilla.
Comment 5 Robert Sayre 2006-12-01 15:34:18 PST
This crash is not exposed in Firefox 2, because the feed parser does not attach an nsILexicalHandler.
Comment 6 Robert Sayre 2006-12-01 15:35:47 PST
We should get this on the branch for 2.0.0.2 in case extensions use it... but I can't set a relevant flag.
Comment 7 Robert Sayre 2006-12-01 15:45:55 PST
requesting blocking to get opinion from drivers. I don't think it should actually block, but it would be a crasher if an extension hits it (I don't know of any that use LexicalHandler, other than Alex's).
Comment 8 Daniel Veditz [:dveditz] 2006-12-01 16:23:19 PST
Too late for this, especially if vanilla FF2 is unaffected. Moving nom to the branch flag rather than the specific release.
Comment 9 Alex Vincent [:WeirdAl] 2006-12-01 17:06:35 PST
Created attachment 247238 [details] [diff] [review]
patch, v1
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2006-12-05 11:24:39 PST
Comment on attachment 247238 [details] [diff] [review]
patch, v1

sr=bzbarsky; try to add some tests for this?
Comment 11 Robert Sayre 2006-12-05 11:38:21 PST
Checking in nsSAXXMLReader.cpp;
/cvsroot/mozilla/parser/xml/src/nsSAXXMLReader.cpp,v  <--  nsSAXXMLReader.cpp
new revision: 1.13; previous revision: 1.12
done
Comment 12 Alex Vincent [:WeirdAl] 2006-12-05 12:26:41 PST
per bz's request, setting in-testsuite? flag.  If we create a DOMParser instead of the XMLHttpRequest in the xpcshell script, it should be possible to make this a single-file testcase (JavaScript, xpcshell).  Isn't that what our "make check" system likes?
Comment 13 Robert Sayre 2006-12-05 12:27:49 PST
no, we should have a test/ directory in parser/xml containing SAX tests.
Comment 14 Alex Vincent [:WeirdAl] 2006-12-06 16:59:12 PST
Created attachment 247752 [details] [diff] [review]
xpcshell unit test
Comment 15 Alex Vincent [:WeirdAl] 2006-12-06 17:02:15 PST
Comment on attachment 247752 [details] [diff] [review]
xpcshell unit test

>+  if (parseErrorLog.length > 0) {
>+    dump(parseErrorLog.join("\n"));
>+  }
>+  do_check_true_with_dump(parseErrorLog.length == 0, parseErrorLog);

I don't know how I let the first three lines of that slip through. :(
Comment 16 Robert Sayre 2006-12-07 13:37:28 PST
Comment on attachment 247752 [details] [diff] [review]
xpcshell unit test

This is good to start.
Comment 17 Robert Sayre 2006-12-09 19:17:18 PST
Checking in Makefile.in;
/cvsroot/mozilla/parser/xml/Makefile.in,v  <--  Makefile.in
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/parser/xml/test/Makefile.in,v
done
Checking in test/Makefile.in;
/cvsroot/mozilla/parser/xml/test/Makefile.in,v  <--  Makefile.in
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/parser/xml/test/unit/test_parser.js,v
done
Checking in test/unit/test_parser.js;
/cvsroot/mozilla/parser/xml/test/unit/test_parser.js,v  <--  test_parser.js
initial revision: 1.1
done
Comment 18 Jay Patel [:jay] 2006-12-27 15:02:39 PST
Comment on attachment 247238 [details] [diff] [review]
patch, v1

Approved for 1.8 branch, a=jay for drivers.
Comment 19 Daniel Veditz [:dveditz] 2007-01-16 15:59:50 PST
We're coming up on the code freeze, need a check-in on this if it's going to make 2.0.0.2
Comment 20 Jeff Walden [:Waldo] (remove +bmo to email) 2007-01-16 19:34:27 PST
Checked in on branch.

Note You need to log in before you can comment on or make changes to this bug.