505221 - crash [@ CountTotalMimeAttachments(MimeContainer*) ]

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Ludovic Hirlimann [:Usul]]

0  	thunderbird.exe  	CountTotalMimeAttachments  	mailnews/mime/src/mimemoz2.cpp:216
1 	thunderbird.exe 	CountTotalMimeAttachments 	mailnews/mime/src/mimemoz2.cpp:223
2 	thunderbird.exe 	CountTotalMimeAttachments 	mailnews/mime/src/mimemoz2.cpp:223
3 	thunderbird.exe 	MimeGetAttachmentList 	mailnews/mime/src/mimemoz2.cpp:568
4 	thunderbird.exe 	mime_display_stream_complete 	mailnews/mime/src/mimemoz2.cpp:975
5 	thunderbird.exe 	nsStreamConverter::OnStopRequest 	mailnews/mime/src/nsStreamConverter.cpp:1068
6 	thunderbird.exe 	nsMsgProtocol::OnStopRequest 	mailnews/base/util/nsMsgProtocol.cpp:393
7 	thunderbird.exe 	nsMailboxProtocol::OnStopRequest 	mailnews/local/src/nsMailboxProtocol.cpp:380
8 	thunderbird.exe 	nsInputStreamPump::OnStateStop 	netwerk/base/src/nsInputStreamPump.cpp:576
9 	thunderbird.exe 	nsInputStreamPump::OnInputStreamReady 	netwerk/base/src/nsInputStreamPump.cpp:401
10 	xpcom_core.dll 	nsInputStreamReadyEvent::Run 	xpcom/io/nsStreamUtils.cpp:111
11 	xpcom_core.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:510
12 	xpcom_core.dll 	NS_ProcessNextEvent_P 	objdir-tb/mozilla/xpcom/build/nsThreadUtils.cpp:227
13 	xpcom_core.dll 	nsThread::Shutdown 	xpcom/threads/nsThread.cpp:465
14 	thunderbird.exe 	mozStorageConnection::Close 	storage/src/mozStorageConnection.cpp:244
15 	xpcom_core.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
16 	thunderbird.exe 	XPCWrappedNative::CallMethod 	js/src/xpconnect/src/xpcwrappednative.cpp:2454
17 	thunderbird.exe 	XPC_WN_CallMethod 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1590

Comment 1

[David :Bienvenu]

is this a top-crash or not? if so, we'd mark it blocking...

Comment 2

[Mark Banner (:standard8)]

Not a top crasher, so not blocking but marking wanted.

Comment 3

[Wayne Mery (:wsmwk)]

now #1 topcrash, no doubt induced by gloda indexing

Comment 4

[David :Bienvenu]

ok, marking blocking, and taking for now. I'll poke around the CountTotalMimeAttachments code. A reproducible case would be helpful, though hard to get, I'm sure.

Comment 5

[David :Bienvenu]

Unless I'm missing something, this code already has null checks, so I don't see any obvious way of fixing this. So a test case would be really helpful.

Comment 6

[Wayne Mery (:wsmwk)]

perhaps one our CC: can whip up a testcase

Comment 7

[Wayne Mery (:wsmwk)]

bug 468394 may be a dupe but no testcase there either - mime_subclass_p(MimeObjectClass*, MimeObjectClass*)

Comment 9

[Ludovic Hirlimann [:Usul]]

(In reply to comment #8)
> *** Bug 519425 has been marked as a duplicate of this bug. ***

We have a identified user having the issue, Andrew what do you need from him ?

Comment 10

[Ludovic Hirlimann [:Usul]]

Bienvenu you can tackle it too :-)

Comment 11

[David :Bienvenu]

Are we in contact with the user? We need to somehow figure out which message is causing the crash and get a copy of it. How we proceed depends somewhat on how technical the user is. If the user can run a debug build under the debugger, we'd just want to poke at some variables in the debugger to identify the message. If not, then we'd need to turn on some sort of logging to figure out which message is crashing. It would also be helpful to know if it's an imap or local message.

Comment 12

[Ludovic Hirlimann [:Usul]]

Massimiliano, how do you feel about running a debug build ?

Comment 13

[Massimiliano Casini]

Ok no problem, I can do it. However, the crash is related to archive mail in the local folders. If I leave this folders Thunderbird runnig fine.

Comment 14

[David :Bienvenu]

Massimiliano, if you want to narrow down which message is causing the problem by a binary search, halving the folder each time, and then give me the message, that would be OK too.

How do you mean, "leave this folder"? You can't turn off indexing of a local folder - do you mean you move the folder out of your profile?

Thx for helping with this!

Comment 15

[Massimiliano Casini]

Sorry, I mean move the folder out my profile.

Comment 16

[David :Bienvenu]

Unless you're fluent in using a debugger, and able to poke around in the local variables when you crash (apologies for not knowing if you are), it would probably be easier to just find the crashing message. I see from the other bug that the folder is > 10000 messages, which would make it painful. One possibility is make a new folder with just the messages that have attachments, by sorting by the attachment column, and copying them to a new folder, under the supposition that the crashing message has an attachment (just a guess from the stack trace).

Comment 17

[David :Bienvenu]

Or,  In the Config Editor (Advanced
prefs) turn on: mailnews.database.global.logging.console and
mailnews.database.global.logging.dump

(.dump will go to stderr, so that works if you know how to run thunderbird from
a shell/console)

and then wait for the crash - I'm not sure if gloda gives enough info with logging to give you an idea of which message it crashed on, but it might.

Comment 18

[David :Bienvenu]

The gloda logging might be the easiest way - I think it will tell you the message key of the messages its streaming; after you crash, you should be able to turn on the order received column for your local folder to find what the message with that key is...

Comment 19

[Ludovic Hirlimann [:Usul]]

Massimiliano ?

Comment 20

[Wayne Mery (:wsmwk)]

If needed, there is a FF extension JSConsoleOutput that will redirect js console to disk. It's no longer available from the author's website. I think I have a version hacked up for TB. FF version available at http://mac.softpedia.com/progDownload/JS-Console-output-redirector-Download-36363.html

Comment 21

[Massimiliano Casini]

Sorry for my delay... was sick. My Thunderbird version runing on windows XP. Can I make a debug?

Comment 22

[David :Bienvenu]

No problem, sorry you were ill - you don't need a debug build to do gloda logging. You should be able to find the message by following these 10 simple steps :-)

1. Use the config editor to set  mailnews.database.global.logging.console to true.
2. Install the extension that Wayne mentioned (he'll have to get that to you somehow).
3 Move the mail folder that causes the crash back into your profile
4. Startup and run until you crash.
5 Save off the file that contains js console output the extension generated, which should have information about the crash, specifically the message key of the last message it tried to index, at the end of the log.
6 Move the bad mail folder away, restart
7 Turn off gloda indexing, shutdown.
8 Move the crashing folder back into your profile, restart, 
9 Click on the bad folder, add the "order received" column to the message list.
10. Sort by order received, and find the message with the key that the log tells you gloda last tried to index.

Comment 23

[Massimiliano Casini]

Problem on point 2. The extension doesn't install to TB3b4

Comment 24

[Ludovic Hirlimann [:Usul]]

(In reply to comment #20)
> If needed, there is a FF extension JSConsoleOutput that will redirect js
> console to disk. It's no longer available from the author's website. I think I
> have a version hacked up for TB. FF version available at
> http://mac.softpedia.com/progDownload/JS-Console-output-redirector-Download-36363.html

Wayne can you attached the version you have ?

Comment 25

[Massimiliano Casini]

(In reply to comment #24)
> (In reply to comment #20)
> > If needed, there is a FF extension JSConsoleOutput that will redirect js
> > console to disk. It's no longer available from the author's website. I think I
> > have a version hacked up for TB. FF version available at
> > http://mac.softpedia.com/progDownload/JS-Console-output-redirector-Download-36363.html
> 
> Wayne can you attached the version you have ?

Wayne already gave to me your version but doesn't install ...

Comment 26

[Massimiliano Casini]

(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #20)
> > > If needed, there is a FF extension JSConsoleOutput that will redirect js
> > > console to disk. It's no longer available from the author's website. I think I
> > > have a version hacked up for TB. FF version available at
> > > http://mac.softpedia.com/progDownload/JS-Console-output-redirector-Download-36363.html
> > 
> > Wayne can you attached the version you have ?
> 
> Wayne already gave to me your version but doesn't install ...

Sorry, Wayne already gave to me his version but doesn't install ...

Comment 27

[Massimiliano Casini]

Guys, the last version installed fine but after crash I can not see the output file log in my XP PC
I wating other solution that can help me to find the message broken in my mail box

Comment 28

[David :Bienvenu]

Is the log empty? Or it doesn't even exist? I have not tried this extension myself (in fact, I don't have a copy of it myself).

Comment 29

[Massimiliano Casini]

The log doesn't exist (or I do not find)

Comment 30

[Wayne Mery (:wsmwk)]

Ratty made us an update which works!

go to http://downloads.mozdev.org/xsidebar/mods/ and get JS_Console_output_redirector_0.1.1-mod.xpi
install
flip it on in Tools>Redirect JSConsole output to file
log file in <profile>\JSConsoleOutput\ directory

Comment 31

[Massimiliano Casini]

Now the log file exist but is empty after crash.

I have set true follow parameter:


mailnews.database.global.logging.console;true
mailnews.database.global.logging.dump;true
mailnews.database.global.logging.net;true
extensions.logging.enabled;true
extensions.jsconsoleoutput.redirect;true
extensions.jsconsoleoutput.hideChromeErrors;true

Comment 32

[Wayne Mery (:wsmwk)]

casini, so you toggled 
  Tools>Redirect JSConsole output to file
and you see good console output in the file?  But after crash the file is empty?

Comment 33

[David :Bienvenu]

Do you crash right away? I had to restart after installing the extension and setting mailnews.database.global.logging.console to true, but then the log was populated immediately.

Comment 34

[Massimiliano Casini]

Attachment: jsconsole.log

This my jsconsole.log with some errors.

Comment 35

[David :Bienvenu]

Does this crash happen if you disable Lightning and restart?

Comment 36

[Massimiliano Casini]

I can not understand why sometimes there is no jsconsole.log (the file exist but is empty after crash).

With Lightning disable behavior is the same, TB crashed but the jsconsole.log is empty

Comment 37

[David :Bienvenu]

I'm pretty sure there's a way to get more logging out of gloda, besides just warnings and errors

Comment 38

[David :Bienvenu]

OK, I've generated a try server build that uses nspr logging to log the mailbox urls we run - it should show up here in an hour or two - http://tinderbox.mozilla.org/showbuilds.cgi?tree=ThunderbirdTry

To generate a log, you need to follow these instructions:

https://wiki.mozilla.org/MailNews:Logging

The log module is "mailbox" (not imap or pop3 or any of those). You should add the "sync" option just in case we crash before the data is written to the log, so

NSPR_LOG_MODULES=mailbox:5,sync

Comment 39

[David :Bienvenu]

Here's a link to the windows build to try - http://s3.mozillamessaging.com/build/try-server/2009-10-09_16:[e-mail]-1255129754/[e-mail]

Comment 40

[Massimiliano Casini]

Find the messages that cause crash. When I to select them (also with index gloda disable) TB to crash. Them have the attachment. I will try to move all the other in new folder and send you the broken message.

Comment 41

[David :Bienvenu]

great, thanks, Massimiliano.

Comment 42

[David :Bienvenu]

If you hide the message pane (f8), it might make it easier to select a message and copy it to a new folder (might want to turn gloda off temporarily as well)

Comment 43

[Massimiliano Casini]

Attachment: local folder with one email broken

unzip file to local folder

Comment 44

[David :Bienvenu]

marking as security sensitive - 2.0 has this problem as well, though it's unlikely this is exploitable. It's a bad cast that causes reads of invalid memory addresses. I do have a very simple fix. I need to see if the test case can be simplified. It looks to be simply a multipart mixed message with text/html part and an message/rfc822 part.

Comment 45

[David :Bienvenu]

Attachment: fix crash

This fixes the crash. I have a reduced test case (basically taking the text and e-mail addresses out of the original test case). I'd like to add this to the xpcshell-tests, but there are still a few assertions triggered by the test case, and that breaks xpcshell tests in debug builds. The assertions look relatively harmless, in the sense that they're handled, and I've seen them before with other messages.

Comment 46

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 406026 [details] [diff] [review]
fix crash

Yes, the code seems much more correct now.

There isn't actually an SR target requested...

If one of the assertions/warnings is about the lack of an identity, I have that resolved in my gloda patch, with the relevant code being as follows if you want to backport it and stick it somewhere:

    acctMgr.createLocalMailAccount();

    let localAccount = acctMgr.FindAccountForServer(acctMgr.localFoldersServer);

    // We need an identity or we get angry warnings.
    let identity = acctMgr.createIdentity();
    localAccount.addIdentity(identity);
    localAccount.defaultIdentity = identity;

Comment 47

[David :Bienvenu]

no, the assertions are in libmime, about sending empty lines to the parser, and attempting to close already closed objects...I strongly suspect this code is involved:

http://mxr.mozilla.org/comm-central/source/mailnews/mime/src/mimemsg.cpp#238

where we're hacking on a newline onto the end of the message part, if we didn't find one.

Comment 48

[David :Bienvenu]

Attachment: simplified folder that crashes

this is an sanitized test case.

It turns out if I add a blank line before the ending mime part separator, this problem goes away. It turns out the code I mxr'ed above is the cause of this problem. If I fix that issue, the assertions go away as well, so I can add a test case - yay!

Comment 49

[David :Bienvenu]

This regression was caused by the fix for Bug 413874, which was landed in 2.0x as well. I'll attach a cumulative patch, along with a test case.

Comment 50

[Ludovic Hirlimann [:Usul]]

(In reply to comment #49)
> This regression was caused by the fix for Bug 413874, which was landed in 2.0x
> as well. I'll attach a cumulative patch, along with a test case.

Could we use the testcase in a xpshell test so we don't regress in the future ?

Comment 51

[David :Bienvenu]

yes, that's what I meant I was going to do.

Comment 52

[David :Bienvenu]

Attachment: cumulative fix with xpcshell test

The underlying issue was that the patch in bug 413874 didn't pass in the right output buffer size into PL_strncpyz, so we were only appending <CR>, not <CRLF>, which, on windows, was getting libmime rather confused about how the part ended...not sure what would happen on linux/mac - we wouldn't have any line endings, which would probably generate an assertion or twenty.

I still think the change to CountTotalMimeAttachments is worthwhile.

I added a mime streaming test to libmime. libmime could certainly use some more test cases, though the gloda tests exercise most of the basics.

Comment 53

[David :Bienvenu]

I audited the rest of the patch in bug 413874 and that was the only instance of this mistake.

Comment 54

[David :Bienvenu]

fix checked in.

Comment 55

[David :Bienvenu]

Attachment: fix for 1.8 branch

this is just the core port of the patch that just landed on the trunk

Comment 56

[David :Bienvenu]

Massimiliano, many thanks for the test case.

Comment 57

[Wayne Mery (:wsmwk)]

does this likely fix nsCounterManager::AddCounterResetsAndIncrements(nsIFrame*) http://crash-stats.mozilla.com/report/index/f2215189-c433-416b-871f-182392090925

strange this bug# does show up in http://talkback-public.mozilla.org/reports/thunderbird/TB20023/index.html  - perhaps talkback not like (MimeContainer*) in the summary.

Comment 58

[David :Bienvenu]

(In reply to comment #57)
> does this likely fix nsCounterManager::AddCounterResetsAndIncrements(nsIFrame*)
> http://crash-stats.mozilla.com/report/index/f2215189-c433-416b-871f-182392090925

I don't see how they could be related.

Comment 59

[Daniel Veditz [:dveditz]]

Comment on attachment 406227 [details] [diff] [review]
fix for 1.8 branch

Approved for 1.8.1.24, a=dveditz for release-drivers

Comment 60

[David :Bienvenu]

fixed for 1.8.1.24

Comment 61

[Al Billings [:abillings - ex-MoCo]]

I want to verify this for 1.8.1.24. How exactly do I add the folder in comment 48 to TB 2 for testing?