142847 - Trunk crashes launching mail [@ nsBufferedInputStream::Fill] [@ MSVCRT.DLL | msvcrt.dll - nsBufferedInputStream::Read]

Status: VERIFIED FIXED

(Core :: XPCOM, defect)

Description

[greer]

Saw this one on the Trunk Top 10 list on Climate. From the comments the crash 
looks like a mailnews problem, but the signature points to networking. Oddly, 
the crashes start appearing in the 2002050308 build and nsBufferedStreams.cpp 
hasn't changed since darin's checkin on March 11. That leads me to believe that 
the culprit is Ben's 5/2 checkin to nsFastLoadFile.cpp. Assigning to him, but 
cc'ing Darin to see if there is are any checks to make in the the 
nsBufferedStreams.cpp code. Ben, please reassign if you think I am in error.

nsBufferedInputStream::Fill   88 
Crash data range: 2002-05-03 to 2002-05-06
Build ID range: 2002050308 to 2002050608
Keyword List : mail(12), message(4), news(6), start(10), crash(6), 

Stack Trace: 

         nsBufferedInputStream::Fill    [nsBufferedStreams.cpp  line 347]
         nsBufferedInputStream::Read    [nsBufferedStreams.cpp  line 286]
         nsFastLoadFileReader::Read     [nsFastLoadFile.cpp  line 569]
         nsBinaryInputStream::Read32    [nsBinaryStream.cpp  line 361]
         nsXULPrototypeElement::Deserialize     [nsXULElement.cpp  line 5053]
         nsXULPrototypeDocument::Read   [nsXULPrototypeDocument.cpp  line 355]
         nsXULPrototypeCache::GetPrototype      [nsXULPrototypeCache.cpp  line 
274]
         nsChromeProtocolHandler::NewChannel    [nsChromeProtocolHandler.cpp  
line 627]
         nsIOService::NewChannelFromURI [nsIOService.cpp  line 778]
         NS_NewChannel  [../../dist/include/necko\nsNetUtil.h  line 162]
         nsDocShell::DoURILoad  [nsDocShell.cpp  line 4819]
         nsDocShell::InternalLoad       [nsDocShell.cpp  line 4719]
         nsDocShell::LoadURI    [nsDocShell.cpp  line 665]
         nsWindowWatcher::OpenWindowJS  [nsWindowWatcher.cpp  line 694]
         nsWindowWatcher::OpenWindow    [nsWindowWatcher.cpp  line 453]
         OpenWindow     [nsAppRunner.cpp  line 409]
         OpenWindow     [nsAppRunner.cpp  line 393]
         LaunchApplicationWithArgs      [nsAppRunner.cpp  line 637]
         HandleArbitraryStartup [nsAppRunner.cpp  line 775]
         DoCommandLines [nsAppRunner.cpp  line 797]
         main1  [nsAppRunner.cpp  line 1432]
         main   [nsAppRunner.cpp  line 1808]
         WinMain        [nsAppRunner.cpp  line 1826]
         WinMainCRTStartup()
         KERNEL32.DLL + 0x1b560 (0xbff8b560)
         KERNEL32.DLL + 0x1b412 (0xbff8b412)
         KERNEL32.DLL + 0x19dd5 (0xbff89dd5)
 
        Source File : nsBufferedStreams.cpp line : 347
     (6012263)  Comments: calling mozilla -mail
     (6012253)  Comments: callin mozilla -mail
     (6011337)  Comments: I was replying to a message in composer.
     (6011076)  Comments: Mozilla crashed when I tried to reply to a message.
     (6008455)  Comments: fault in necko.dll?
     (6008426)  Comments: hmmmm
     (6008418)  Comments: hmmhmm
     (6004132)  Comments: any attempt (mozilla.exe -mail; clicking on mail 
within browser) to start the mail reader causes a crash without displaying any 
part of said reader.
     (6004088)  Comments: Clicked on mozilla.exe -mail quick launch link 
     (6002815)  Comments: installed spell checker
     (5996954)  URL: http://www.inmagic.com/
     (5996954)  Comments: First fresh start after installing 2002-05-06  then 
installing Calendar
     (5988125)  Comments: started mozilla with the following command:mozilla.exe 
-P "profile name" -mail
     (5987714)  Comments: just loading mail/news :(
     (5987228)  Comments: was switching to the browser when ICQ alerts audio 
came on for receiving a message. The screen went white and I could not escape 
without doing a control-alt-delete to close down Mozilla
     (5975974)  Comments: clicked on mail/news prefs to cause crash
     (5975921)  Comments: delete all email accts in mail/news thru the mail 
prefs window exit mozilla delete mail folder from profile directory restart 
mozilla and mail try going to mail/news settings from edit menu CRASH
     (5975870)  Comments: delete all mail accts in mail/newsexit mozilladelete 
the mail folder from
the profile directoryrestart mozilla and then mail/newstry adding a new 
accountCRASH!
     (5975858)  Comments: delete all your mail accounts in Mail/News  Exit 
mozilla.delete the mail related folders from profile directoryrestart mozilla 
and then mail/news. NO  accounts (inbox)
should show up.Then try to add a new account in mail/news to crash
     (5957105)  Comments: hit edit on karnazes latest bug testcase from austria 
about cell width
     (5954614)  Comments: Starting the trunk build.
     (5954609)  Comments: Starting the trunk build.
     (5939755)  Comments: I tried to open Mozilla. The browser skin didn't even 
load.
     (5928993)  URL: www.yorinfm.nl
     (5928993)  Comments: starting the mail again
     (5928988)  URL: www.yorinfm.nl
     (5928988)  Comments: starting the mail
     (5912715)  Comments: messing with prefs file to try to sign news messages

Comment 1

[greer]

kw -> topcrash. This one is in the Trunk top 10.

Comment 2

[Oliver Klee]

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

Comment 3

[Jay Patel [:jay]]

Adding [@ MSVCRT.DLL | msvcrt.dll - nsBufferedInputStream::Read] to summary
since this crash is also being reported under those stack signatures.

Comment 4

[R.K.Aa.]

I just crashed 3 times in a row trying to start mailnews from the browser in
build 2002051104. With the same build mailnews worked perfectly - two days ago.
3 talkbacks were submitted around 40 minutes ago frmo [e-mail]
A fresh install of a new build and mailnews starts just fine again.

Comment 5

[R.K.Aa.]

doron says one of the TB ID's was 6238215

Comment 6

[greer]

Attachment: R.K.Aa's stack from incident #6238215

Comment 7

[timeless]

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

Comment 8

[Frank Tang]

from the stack trace, the crash code is around 
275     PRUint32 read = 0;
276     while (count > 0) {
277         PRUint32 amt = PR_MIN(count, mFillPoint - mCursor);
278         if (amt > 0) {
279             memcpy(buf + read, mBuffer + mCursor, amt);
280             read += amt;
281             count -= amt;
282             mCursor += amt;
283         }

notice read, count, amt are all PRUint32 . I wonder what will happen if
mCursor > mFillPoint. Will that produce a negative number or a positive number?
after the PR_MIN

Comment 9

[Darin Fisher]

here is the definition of PR_MIN (from prtypes.h):

  #define PR_MIN(x,y)     ((x)<(y)?(x):(y))

so, if mCursor > mFillPoint, then most likely amt will equal count, since
mFillPoint - mCursor will be something very large.  of course, if mCursor is
bigger than mFillPoint, then we probably will crash somewhere.

Comment 10

[John Morrison]

Okay, I have a XUL.mfl in a current opt-with-symbols trunk build that crashes
with this stack. Should I attach it for inspection? [You can't really use it
in another build since it will invalidate on mtime or path mismatches].

In the "optimized lying" debugger, It claims that |this| is null.

This is 100% reproducible with this file and my build and I've saved off a copy 
of the XUL.mfl. If someone can propose a patch to dump out some variables of 
interest, I can see if I can get some more info about what is going wrong.

Comment 11

[Ben Goodger (use ben at mozilla dot org for email)]

please do

Comment 12

[John Morrison]

mailed xul.mfl to ben (too big to attach).

Comment 13

[Darin Fisher]

i looked at the code briefly and it looks like someone is trying to read from
the nsBufferedInputStream after it has been closed.  the simple solution might
be to make Read fail if Source() is NULL.  that would at least be a safeguard
against this crash.  still... it'd be nice to know why the nsBufferedInputStream
is being used like this.

Comment 14

[Darin Fisher]

Attachment: v1 patch - add some null checks

Comment 15

[timeless]

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

Comment 16

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 84949 [details] [diff] [review]
v1 patch - add some null checks

sr=[e-mail]

Comment 17

[timeless]

Bug 102111 Comment 37 indicates that this patch is hopefully just a first 
draft, darin do you want to look into that before committing, or are we all for 
bandaids?

Comment 18

[timeless]

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

Comment 19

[Darin Fisher]

timeless: no matter what, this is a good patch to have... but that said, it
might not prevent this crash... it might just move it elsewhere :/

Comment 20

[Doug Turner (:dougt)]

Comment on attachment 84949 [details] [diff] [review]
v1 patch - add some null checks

r=dougt. lets still continue hunting for the real reason this is happening. 
The assertions will help us.

Comment 21

[Boris Zbarsky [:bzbarsky]]

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

Comment 22

[Boris Zbarsky [:bzbarsky]]

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

Comment 23

[Daniel Bratell]

Comment on attachment 84949 [details] [diff] [review]
v1 patch - add some null checks

Many of the functions in the class already has: 

    if (mStream == nsnull)
	return NS_BASE_STREAM_CLOSED;

Maybe that should be put in all functions?

Comment 24

[Ben Goodger (use ben at mozilla dot org for email)]

I've checked this patch in on the trunk. If, after tomorrow's builds, we see
other crashes related to Mail, we'll see where this has moved ;-) 

Comment 25

[Ben Goodger (use ben at mozilla dot org for email)]

I'm also going to mark this "fixed", if we see new stacks, please reopen and
post them. 

Comment 26

[Darin Fisher]

ben: is this crash really trunk only?  do we have any idea what caused this
then?  shouldn't we try to land it on the branch too just in case the code that
caused this lands there as well some day?

Comment 27

[timeless]

comment #0 blames:
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/xpcom/io/nsFastLoadFile.cpp&
rev=3.17
Which is not on the branch:
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/xpcom/io/nsFastLoadFile.cpp&
rev=MOZILLA_1_0_BRANCH
there are a lot of crash fixes resulting from fastload, i think it's almost not 
worth trying to port it and them to a branch.  I can point to other cases 
(0.9.4.1.2) where people put pieces of something on a branch and failed to 
collect or request all the crash fixes reach it.

Fastload is a neat thing, but it's probably best to just force 1.0 to live 
without it forever.

Otoh, since anything designed for 1.0 could make the same mistakes fastload 
does. -- Except that nsBuffered*Stream don't have contracts so there shouldn't 
be a way for third party things to use them.

I think the real problem is this rearranged/shortened version of init:

nsresult nsBufferedStream::Init(nsISupports* stream, PRUint32 bufferSize) { 
  NS_ASSERTION(stream, "need to supply a stream"); 
  NS_IF_ADDREF(mStream = stream);
  mBuffer = new char[bufferSize]; if (!mBuffer) return NS_ERROR_OUT_OF_MEMORY; 
  return NS_OK; 
}

nsBufferedStream::Init(0, 10)
is it just me, or does it seem that common sense demands an error condition 
from Init?

<mkaply> sure looks that way to me
<smontagu> timeless: agreed

It's all fine and dandy to assert, but we really should return an error since 
you clearly can't do *anything* useful in this case.

If someone wants me to file a new bug, i'll do so.

Comment 28

[timeless]

Attachment: a possible branch candidate

this is a clean and simple patch.
NS_NewBufferedInputStream
NS_NewBufferedOutputStream
both handle Init() failing and their callers seem to handle them failing.
mStream is only set once [Init()], so if it's null that means Init set it to
null.

I believe that the previous patches are wallpaper.  Unfortunately, I can't find
the contract for nsBuffered*Stream so I can't figure out if Init should return
the failure or if its callers should never call it with a null stream.

Comment 29

[Darin Fisher]

Comment on attachment 86126 [details] [diff] [review]
a possible branch candidate

>Index: nsBufferedStreams.cpp

>     NS_ASSERTION(stream, "need to supply a stream");
>+    if (!stream)
>+        return NS_ERROR_INVALID_ARG;

replace these three lines with:

  NS_ENSURE_ARG(stream);

and sr=darin.. nice work!  it is incorrect/meaningless to
initialize a buffered input stream with no "inner" stream.

i think we want this patch in addition to the previous one.
while it didn't address the cause of the problem it does
add an extra level of safety consistent with the implementations
of the other interface methods.

Comment 30

[timeless]

Attachment: per darin/bbaetz

Comment 31

[Ben Goodger (use ben at mozilla dot org for email)]

darin, we could land it there too, but this crash surfaced as a result of
fastload being turned on, which isn't on the branch at this stage. 

Comment 32

[Darin Fisher]

Comment on attachment 86190 [details] [diff] [review]
per darin/bbaetz

sr=darin

ben: ok, so no pressure to get this in.. though i still think it'd be good to
fixup the parameter checking on the branch as well as the trunk just to avoid
future headache.

Comment 33

[Doug Turner (:dougt)]

I think that ben's patch will reduce the chance of a crash, but there still is a
race which can result in a crash:

if Fill() is called from thread 1, and Close() is called from thread 2, mStream
will be in an uncertain state.  If Close() occurs after Fill()'s check on
mStream, a crash will occur similar to the one shown here.

Comment 34

[Darin Fisher]

dougt: i don't think the buffered input stream was designed to be called from
more than one thread...  is the buffered input stream being accessed from more
than one thread at the same time??  that would really suck if true.

ben: where is the code that is using/constructing this stream?

Comment 35

[Jay Patel [:jay]]

Has there been any checkins after the one Ben mentions in Comment #24?  Is there
still more work to be done as mentioned in the other comments? 

This crash has not been reported by Talkback on the MozillaTrunk since 6/1...and
there are currently 0 incidents for any release or milestone off the Gecko1.0
Branch.  

Should we leave this resolved fix since there seems to still be some discussion
being done on the topic or can we make this verified?

Comment 36

[Jay Patel [:jay]]

From the lack of feedback, I'm just going to mark this verified (the crash is
gone).  If more work is to be done (as hinted in comment #33 and comment #34),
please feel free to log a new bug (or reopen if totally necessary).