The default bug view has changed. See this FAQ.

Crash in SAXReader with doctype missing public, system ID's

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
XML
--
critical
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: WeirdAl, Assigned: WeirdAl)

Tracking

({crash, fixed1.8.1.2, testcase})

Trunk
mozilla1.9alpha1
x86
Mac OS X
crash, fixed1.8.1.2, testcase
Points:
---
Bug Flags:
wanted1.8.1.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Assignee)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
Created attachment 247230 [details]
gdb stack trace
(Assignee)

Comment 2

11 years ago
Created attachment 247231 [details]
testcase, part 1 of 2 (XML document)

Comment 3

11 years ago
Shoot, I thought we got all of these.
(Assignee)

Comment 4

11 years ago
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

11 years ago
This crash is not exposed in Firefox 2, because the feed parser does not attach an nsILexicalHandler.
Target Milestone: --- → mozilla1.9alpha

Comment 6

11 years ago
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

11 years ago
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).
Flags: blocking1.8.1.1?
Too late for this, especially if vanilla FF2 is unaffected. Moving nom to the branch flag rather than the specific release.
Flags: blocking1.8.1.1? → wanted1.8.1.x+
(Assignee)

Comment 9

11 years ago
Created attachment 247238 [details] [diff] [review]
patch, v1
Assignee: xml → ajvincent
Status: NEW → ASSIGNED
Attachment #247238 - Flags: review?(sayrer)

Updated

10 years ago
Attachment #247238 - Flags: review?(sayrer) → review+
(Assignee)

Updated

10 years ago
Attachment #247238 - Flags: superreview?(bzbarsky)
Comment on attachment 247238 [details] [diff] [review]
patch, v1

sr=bzbarsky; try to add some tests for this?
Attachment #247238 - Flags: superreview?(bzbarsky) → superreview+

Comment 11

10 years ago
Checking in nsSAXXMLReader.cpp;
/cvsroot/mozilla/parser/xml/src/nsSAXXMLReader.cpp,v  <--  nsSAXXMLReader.cpp
new revision: 1.13; previous revision: 1.12
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Attachment #247238 - Flags: approval1.8.1.2?
(Assignee)

Comment 12

10 years ago
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?
Flags: in-testsuite?

Comment 13

10 years ago
no, we should have a test/ directory in parser/xml containing SAX tests.
(Assignee)

Comment 14

10 years ago
Created attachment 247752 [details] [diff] [review]
xpcshell unit test
Attachment #247752 - Flags: review?(sayrer)
(Assignee)

Comment 15

10 years ago
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

10 years ago
Comment on attachment 247752 [details] [diff] [review]
xpcshell unit test

This is good to start.
Attachment #247752 - Flags: review?(sayrer) → review+

Comment 17

10 years ago
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
Flags: in-testsuite? → in-testsuite+

Comment 18

10 years ago
Comment on attachment 247238 [details] [diff] [review]
patch, v1

Approved for 1.8 branch, a=jay for drivers.
Attachment #247238 - Flags: approval1.8.1.2? → approval1.8.1.2+
We're coming up on the code freeze, need a check-in on this if it's going to make 2.0.0.2
Whiteboard: [at risk] need check-in
Whiteboard: [at risk] need check-in → [at risk][checkin needed]
Checked in on branch.
Whiteboard: [at risk][checkin needed]
Keywords: fixed1.8.1.2
You need to log in before you can comment on or make changes to this bug.