82332 - Mozilla Trunk crashes when loading an empty file [@ nsExpatTokenizer::PushXMLErrorTokens] [@ nsAString::do_AssignFromReadable]

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect, P1)

Description

[Markus Hübner]

When going to the URL
http://zvon.org/index.php?nav_id=zvonlinks&mime=xml_xslt
Mozilla (build 22/5)
some additional checking should beware mozilla from crashing

Comment 1

[Dan Naumov]

I can confirm this. Mozilla May 22 build 2001052204 for Win32 under Linux also
crashes on this URL.

Comment 2

[Peter Van der Beken [:peterv]]

Confirming the crash in a build from (5/22/2001 pulled around 1pm GMT, but this
has nothing to do with XSLT. That URL returns an empty file (just a couple of
spaces). We crash in the parser (in nsExpatTokenizer::PushXMLErrorTokens) while
trying to report the error, I'll attach a stack trace. I tried this with 0.9, it
reports the parsing error without crashing.

Markus: please use the Bugzilla Helper to file bugs or read
http://www.mozilla.org/quality/bug-writing-guidelines.html.

Comment 3

[Peter Van der Beken [:peterv]]

Reassigning.

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: Stack trace of crash

Comment 5

[rbs]

*** Bug 83267 has been marked as a duplicate of this bug. ***

Comment 6

[rbs]

Taking... It looks like the crash is introduced by my patch for bug 47416. I am 
curious why the scanner is null in the _tokenizer_...

Comment 7

[harishd]

rbs: In nsExpatTokenizer::ConsumeToken() the scanner is set to null ( due to 
ownership reasons! ). The assumption is that any error in an xml document would 
be caught during the ConsumeToken phase. But, in the case of an empty xml 
document we enter the error checking in the DidTokenize() phase. So, two things 
should happen:
1) need a null check in PushXMLErrorTokens().
2) Reinitialize mState.scanner in DidTokenize().

I don't mind fixing this...but if you're willing to then it's all yours :)

Comment 8

[rbs]

Targetting to mozilla0.9.1 -- 1) I will feel bad if betas embed this crash,
2) adding a null check on a pointer isn't rocket science :-)

Comment 9

[harishd]

Adding a null check would prevent the crash. But then, what happens to the file 
name?

Comment 10

[rbs]

I am not sure at the moment. I haven't done any investigation yet as I am 
currently behind a restrictive firewall with no mozilla build environment.
I will be following up on the hints that you gave today (local time...).

Comment 11

[rbs]

The declaration shows that the scanner is simply a weak reference. Not sure
why/if there is an ownership problem then. Seems like it is not necessary for
it to be null-ed out... 

typedef struct _XMLParserState {
  XML_Parser parser;
  nsScanner* scanner; <---------------------------- declaration
  [...]
} XMLParserState;

Moreover, I don't see any test like 'if (mState->scanner)' anywhere throughout
the code and this is a strong indication that all users expect it not to be 
null.

Finally, when I simply remove the line in ConsumeToken() when the scanner is 
null-ed out, the problem nicely goes away. The browser runs fine and the error
is correctly reported on the testcase. Am I missing something?

nsresult nsExpatTokenizer::ConsumeToken(nsScanner& aScanner,PRBool& 
aFlushTokens) {
  [...]
  mState->tokenDeque = &mTokenDeque;
  mState->parser = mExpatParser;
  mState->scanner = &aScanner;

  [...]

- mState->scanner = nsnull;
  mState->bufferStart = mState->bufferEnd = nsnull;

  return result;
}

Comment 12

[Johnny Stenback (:jst)]

Someone needs to null out the scanner somewhere, the ownership problem is
probably a matter of making sure that the scanner pointer (being weak n' all) is
nulled out before the scanner goes away so that we don't end up poking at
deleted memory, which may or may not result in a crash.

Comment 13

[rbs]

Proposed fix... forget about the dreaded mState.scanner... use XML_GetBase()
which will return the URL which is set with XML_SetBase() when creating 
mExpatParser in its ctor nsExpatTokenizer(nsString* aURL) ...

harishd: can you r=;  jst: can you sr=; thanks.
======

Index: nsExpatTokenizer.cpp
===================================================================
RCS file: /cvsroot/mozilla/htmlparser/src/nsExpatTokenizer.cpp,v
retrieving revision 1.84
diff -u -r1.84 nsExpatTokenizer.cpp
--- nsExpatTokenizer.cpp	2001/05/19 11:27:00	1.84
+++ nsExpatTokenizer.cpp	2001/05/31 06:26:59
@@ -404,7 +404,7 @@
     // Adjust the column number so that it is one based rather than zero based.
     error->colNumber = XML_GetCurrentColumnNumber(mExpatParser) + 1;
     error->description.AssignWithConversion(XML_ErrorString(error->code));
-    error->sourceURL = mState->scanner->GetFilename();
+    error->sourceURL.Assign((PRUnichar*)XML_GetBase(mExpatParser));
     if (!aIsFinal) {
       PRInt32 byteIndexRelativeToFile = 0;
       byteIndexRelativeToFile = XML_GetCurrentByteIndex(mExpatParser);

Comment 14

[Johnny Stenback (:jst)]

sr=jst

Comment 15

[harishd]

rbs: I like your second patch a lot. r=harishd

Comment 16

[rbs]

cc:ing blizzard for a=

Comment 17

[Christopher Blizzard (:blizzard)]

a=blizzard for 0.9.1 and the trunk if you need it.

Comment 18

[rbs]

Landed in the trunk and the branch. Resolving as FIXED.

Note for future references:
===========================
The nsExpatDTD (see below) doesn't set the name. But it isn't used in the tree, 
so there is no problem. Perhaps nsExpatDTD should be removed from the tree to 
reduce the bloat if it is not necessary anymore. 

The DTD that is used for XML/XHTML is CWellFormedDTD. This sets the name and all 
the reporting goes well.

In any case if the filename isn't set, it will appear as blank -- there won't be 
a crash because aString.Assign(null) is designed not to crash, it amounts to
aString.SetLength(0).

===============
CWellFormedDTD: This is OK, it is passing the filename. It is the one in use.
===============
nsresult CWellFormedDTD::GetTokenizer(nsITokenizer*& aTokenizer) {
  nsresult result=NS_OK;
  if(!mTokenizer) {
    mTokenizer=(nsHTMLTokenizer*)new nsExpatTokenizer(&mFilename); <--- FILENAME
    NS_IF_ADDREF(mTokenizer);
  }
  aTokenizer=mTokenizer;
  return result;
}

===========
nsExpatDTD: BAD, it isn't passing the filename. Nobody uses it. Now obsolete?
===========
nsresult nsExpatDTD::GetTokenizer(nsITokenizer*& aTokenizer) {
  nsresult result=NS_OK;
  if(!mTokenizer) {
    result=NS_New_Expat_Tokenizer(&mTokenizer); <----- NO FILENAME
    mExpatParser = XML_ParserCreate(NULL);
    if (mExpatParser) {
      SetupExpatCallbacks();
    }
  }
  aTokenizer=mTokenizer;
  return result;
}

Comment 19

[Heikki Toivonen (remove -bugzilla when emailing directly)]

*** Bug 84068 has been marked as a duplicate of this bug. ***

Comment 20

[Jay Patel [:jay]]

Adding [@ nsAString::do_AssignFromReadable] to summary and topcrash keyword for
future reference, as this *was* a topcrasher according to Talkback.

Comment 21

[bsharma]

Verified on:
build: 2001-06-04-09-Mtrunk
platform: WinNT

The mozilla does not crashes but I get an xml parsing error.