326741 - Crash with parsing malformed XUL file with -chrome

Status: RESOLVED FIXED

(Core :: XML, defect, P3)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

With the attached testcase, I see a crash when firefox.exe is started with -chrome with the file. Seems to be related to a <?xml-stylesheet without a closing ?>.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: test case bug, run with -chrome

Stack trace here; the top stack line is

"mRootContent->GetAttr(kNameSpaceID_None, nsHTMLAtoms::title, title);"

I assume mRootContent is null, but I can't verify that with VS2k5.


>	gklayout.dll!nsXULDocument::ResumeWalk()  Line 2964 + 0x3 bytes	C++
 	gklayout.dll!nsXULDocument::EndLoad()  Line 659	C++
 	gklayout.dll!XULContentSinkImpl::DidBuildModel()  Line 406	C++
 	gkparser.dll!nsExpatDriver::DidBuildModel(unsigned int anErrorCode=0, int aNotifySink=1, nsIParser * aParser=0x01fd0480, nsIContentSink * aSink=0x00c02460)  Line 1126	C++
 	gkparser.dll!nsParser::DidBuildModel(unsigned int anErrorCode=0)  Line 975 + 0x13 bytes	C++
 	gkparser.dll!nsParser::ResumeParse(int allowIteration=1, int aIsFinalChunk=1, int aCanInterrupt=1)  Line 1684	C++
 	gkparser.dll!nsParser::ContinueInterruptedParsing()  Line 1139 + 0xc bytes	C++
 	gkparser.dll!nsParser::ContinueParsing()  Line 1118	C++
 	gklayout.dll!CSSLoaderImpl::SheetComplete(SheetLoadData * aLoadData=0x028cb0f0, unsigned int aStatus=0)  Line 1503	C++
 	gklayout.dll!CSSLoaderImpl::SheetComplete(SheetLoadData * aLoadData=0x028db6c0, unsigned int aStatus=0)  Line 1521	C++
 	gklayout.dll!CSSLoaderImpl::ParseSheet(nsIUnicharInputStream * aStream=0x028b8790, SheetLoadData * aLoadData=0x00000000, int & aCompleted=1)  Line 1435	C++
 	gklayout.dll!SheetLoadData::OnStreamComplete(nsIUnicharStreamLoader * aLoader=0x028db7e8, nsISupports * aContext=0x00000000, unsigned int aStatus=0, nsIUnicharInputStream * aDataStream=0x028b8790)  Line 825 + 0x16 bytes	C++
 	necko.dll!nsUnicharStreamLoader::OnStopRequest(nsIRequest * request=0x028b8790, nsISupports * ctxt=0x00000000, unsigned int aStatus=0)  Line 174	C++
 	jar50.dll!nsJARChannel::OnStopRequest(nsIRequest * req=0x028dbdf0, nsISupports * ctx=0x00000000, unsigned int status=0)  Line 719	C++
 	necko.dll!nsInputStreamPump::OnStateStop()  Line 512	C++
 	necko.dll!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream * stream=0x028dbef0)  Line 337	C++
 	xpcom_core.dll!nsInputStreamReadyEvent::EventHandler(PLEvent * plevent=0x028dbe7c)  Line 121	C++
 	xpcom_core.dll!PL_HandleEvent(PLEvent * self=0x028dbe7c)  Line 689	C
 	xpcom_core.dll!PL_ProcessPendingEvents(PLEventQueue * self=0x00bfad88)  Line 624	C
 	xpcom_core.dll!_md_EventReceiverProc(HWND__ * hwnd=0x00220482, unsigned int uMsg=49662, unsigned int wParam=0, long lParam=12561800)  Line 1409	C

Comment 2

[Martijn Wargers (dead)]

Doesn't crash Mozilla1.7 (there, I get an xml parsing error, which I would expect), so seems like a regression.
A regression range might be useful here.

Comment 3

[Ria Klaassen (not reading all bugmail)]

Regression between 1.9a1_2006012705 and 1.9a1_2006012714.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Thanks, Ria! bug 323299 seems like a potential candidate, asking over there.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So... I see us going through the loop in nsExpatDriver::ConsumeToken just once when we're restarted from the stylesheet load.  We pass a null buffer to expat, which parses only two bytes...  Then we set mEOF to true (since start == end, chunk is final, etc).  And then we bail.

I never see nsExpatDriver::HandleError called.  If it had been, we'd have a <parseerror> in the XUL document and the stupid XULDocument code would be ok...

So yeah, this is likely a regression from bug 323299.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: NSPR log of this bit

Comment 7

[Boris Zbarsky [:bzbarsky]]

So the thing is that XML_ResumeParser doesn't take an aIsFinal argument.  So it reads the '<?', then doesn't know that's it and bails out waiting for more data.  So we need to call into expat one more time here, either because we update mEOF _inside_ the while loop (right now we do it outside), or just generally go through the while loop a second time if the first time through we resumed...

Peterv, thoughts?

Comment 8

[Peter Van der Beken [:peterv]]

I have a patch in attachment 213745 [details] [diff] [review] for bug 325733 that also fixes this.

Comment 9

[Peter Van der Beken [:peterv]]

Fixed by the fix for bug 325733.