Closed
Bug 339599
Opened 18 years ago
Closed 18 years ago
nsISAXXMLReader expects a nsIChannel passed to onStartRequest, but interface takes an nsIRequest
Categories
(Core :: XML, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.8.1beta2
People
(Reporter: toddf, Assigned: sayrer)
References
Details
(Keywords: fixed1.8.1, Whiteboard: [swag: 1d])
Attachments
(1 file)
1.20 KB,
patch
|
mrbkap
:
review+
peterv
:
superreview+
darin.moz
:
approval1.8.1+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3 Build Identifier: XULRunner 1.8 Branch The application will crash unless you do something like: function StreamListener( reader, channel ) { this.reader = reader; this.channel = channel; } StreamListener.prototype = { reader:null, onStartRequest: function( request,context ){ dump( "starting\n" ); // XXX: passing channel here to work around bug in nsParser.cpp this.reader.onStartRequest( this.channel, context ); }, onDataAvailable: function( request, context, input, offset, count ){ dump( "offset: " + offset + ", count: " + count + "\n" ); // XXX: passing channel here to work around bug in nsParser.cpp this.reader.onDataAvailable( this.channel, context, input, offset, count ); }, onStopRequest: function( request, context, status ){ // XXX: passing channel here to work around bug in nsParser.cpp this.reader.onStopRequest( this.channel, context, status ); } }; crash happens in: nsresult nsParser::OnStartRequest(nsIRequest *request, nsISupports* aContext) ... nsCOMPtr<nsIChannel> channel = do_QueryInterface(request); NS_ASSERTION(channel, "parser needs a channel to find a dtd"); // the channel is NULL since it wasn't an nsIChannel, it was an nsIRequest rv = channel->GetContentType(contentType); if (NS_SUCCEEDED(rv)) { mParserContext->SetMimeType(contentType); } Reproducible: Always Steps to Reproduce: call nsISAXXMLReader without using an nsIChannel, instead using something like an nsIInputStreamPump. Like below: var reader = CC["@mozilla.org/saxparser/xmlreader;1"].createInstance(CI.nsISAXXMLReader); var filestream = CC["@mozilla.org/network/file-input-stream;1"].createInstance(CI.nsIFileInputStream); var pump = CC["@mozilla.org/network/input-stream-pump;1"].createInstance(CI.nsIInputStreamPump); var ioService = CC["@mozilla.org/network/io-service;1"].getService(CI.nsIIOService); var channel = ioService.newChannel( urlSpec,null,null ); var uri = ioService.newURI( urlSpec, null, null ); filestream.init(file, 0x01, 0444, 0); reader.contentHandler = new RestoreParser( broker, progress ); reader.parseAsync( null ); pump.init( filestream, -1, -1, StreamChunkSize, 1, false ); reader.baseURI = uri; pump.asyncRead( reader, null );
Reporter | ||
Updated•18 years ago
|
Version: Trunk → 1.8 Branch
Assignee | ||
Comment 1•18 years ago
|
||
I have fix for that depends on 337451, after that lands, we'll be able to test for a channel in nsSAXXMLReader::InitParser.
Assignee | ||
Updated•18 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [swag: 1d]
Assignee | ||
Updated•18 years ago
|
Target Milestone: --- → mozilla1.8.1beta1
Assignee | ||
Updated•18 years ago
|
Flags: blocking1.8.1?
Assignee | ||
Comment 2•18 years ago
|
||
The ASSERT in the current code seems bogus to me, since it will keep going if channel->GetContentType fails. I tested XML files with and without declarations, and the crashing code in Comment #1 worked fine for me on trunk.
Attachment #225225 -
Flags: superreview?(peterv)
Attachment #225225 -
Flags: review?(mrbkap)
Comment 3•18 years ago
|
||
Comment on attachment 225225 [details] [diff] [review] don't assume nsIChannel It looks like with this patch, we'll end up in DetermineParseMode's catch-all else case, which, while scary, seems like it should work.
Attachment #225225 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 4•18 years ago
|
||
(In reply to comment #3) > (From update of attachment 225225 [details] [diff] [review] [edit]) > It looks like with this patch, we'll end up in DetermineParseMode's catch-all > else case, which, while scary, seems like it should work. For trunk, we could add nsIParser::SetMimeType, and then assert that mParserContext->mMimeType is set in OnStartRequest.
Comment 5•18 years ago
|
||
Comment on attachment 225225 [details] [diff] [review] don't assume nsIChannel Yeah, this seems like a fragile solution.
Attachment #225225 -
Flags: superreview?(peterv) → superreview+
Assignee | ||
Comment 6•18 years ago
|
||
Comment on attachment 225225 [details] [diff] [review] don't assume nsIChannel For the branch, I think this is the best we can do :/
Attachment #225225 -
Flags: approval-branch-1.8.1?(peterv)
Assignee | ||
Comment 7•18 years ago
|
||
/cvsroot/mozilla/parser/htmlparser/src/nsParser.cpp,v <-- nsParser.cpp new revision: 3.393; previous revision: 3.392 done
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Updated•18 years ago
|
Attachment #225225 -
Flags: approval-branch-1.8.1?(peterv) → approval1.8.1?
Updated•18 years ago
|
Flags: blocking1.8.1? → blocking1.8.1-
Updated•18 years ago
|
Attachment #225225 -
Flags: approval1.8.1? → approval1.8.1+
Assignee | ||
Comment 8•18 years ago
|
||
retargetted to b2 per beltzner
Target Milestone: mozilla1.8.1beta1 → mozilla1.8.1beta2
Assignee | ||
Comment 9•18 years ago
|
||
/cvsroot/mozilla/parser/htmlparser/src/nsParser.cpp,v <-- nsParser.cpp new revision: 3.370.4.4; previous revision: 3.370.4.3 done
Keywords: fixed1.8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•