Last Comment Bug 505221 - crash [@ CountTotalMimeAttachments(MimeContainer*) ]
: crash [@ CountTotalMimeAttachments(MimeContainer*) ]
Status: RESOLVED FIXED
[no l10n impact][sg:moderate] invalid...
: crash, fixed1.8.1.24, regression, testcase, topcrash
Product: MailNews Core
Classification: Components
Component: MIME (show other bugs)
: 1.9.1 Branch
: x86 Windows XP
: -- critical (vote)
: Thunderbird 3.0rc1
Assigned To: David :Bienvenu
:
Mentors:
http://crash-stats.mozilla.com/report...
: 519425 (view as bug list)
Depends on:
Blocks: 413874
  Show dependency treegraph
 
Reported: 2009-07-20 06:18 PDT by Ludovic Hirlimann [:Usul]
Modified: 2015-10-03 03:13 PDT (History)
11 users (show)
mozilla: blocking‑thunderbird3+
standard8: wanted‑thunderbird3+
vseerror: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
jsconsole.log (5.68 KB, application/octet-stream)
2009-10-08 01:51 PDT, Massimiliano Casini
no flags Details
local folder with one email broken (8.53 KB, application/zip)
2009-10-13 01:43 PDT, Massimiliano Casini
no flags Details
fix crash (675 bytes, patch)
2009-10-13 08:39 PDT, David :Bienvenu
bugmail: review+
Details | Diff | Review
simplified folder that crashes (1.91 KB, text/plain)
2009-10-13 11:49 PDT, David :Bienvenu
no flags Details
cumulative fix with xpcshell test (8.13 KB, patch)
2009-10-13 13:34 PDT, David :Bienvenu
bugmail: review+
standard8: superreview+
Details | Diff | Review
fix for 1.8 branch (1.50 KB, patch)
2009-10-14 08:06 PDT, David :Bienvenu
dveditz: approval1.8.1.next+
Details | Diff | Review

Description Ludovic Hirlimann [:Usul] 2009-07-20 06:18:58 PDT
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 2009-08-10 16:14:35 PDT
is this a top-crash or not? if so, we'd mark it blocking...
Comment 2 Mark Banner (:standard8) 2009-08-10 16:18:36 PDT
Not a top crasher, so not blocking but marking wanted.
Comment 3 Wayne Mery (:wsmwk, NI for questions) 2009-09-25 08:11:07 PDT
now #1 topcrash, no doubt induced by gloda indexing
Comment 4 David :Bienvenu 2009-09-25 08:21:46 PDT
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 2009-09-25 08:29:56 PDT
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, NI for questions) 2009-09-25 08:40:11 PDT
perhaps one our CC: can whip up a testcase
Comment 7 Wayne Mery (:wsmwk, NI for questions) 2009-09-25 10:57:25 PDT
bug 468394 may be a dupe but no testcase there either - mime_subclass_p(MimeObjectClass*, MimeObjectClass*)
Comment 8 Ludovic Hirlimann [:Usul] 2009-09-29 06:57:57 PDT
*** Bug 519425 has been marked as a duplicate of this bug. ***
Comment 9 Ludovic Hirlimann [:Usul] 2009-09-29 06:58:44 PDT
(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] 2009-09-29 14:00:32 PDT
Bienvenu you can tackle it too :-)
Comment 11 David :Bienvenu 2009-09-29 14:01:35 PDT
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] 2009-09-30 01:37:00 PDT
Massimiliano, how do you feel about running a debug build ?
Comment 13 Massimiliano Casini 2009-09-30 01:46:28 PDT
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 2009-09-30 06:38:31 PDT
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 2009-09-30 07:11:43 PDT
Sorry, I mean move the folder out my profile.
Comment 16 David :Bienvenu 2009-10-01 13:04:36 PDT
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 2009-10-01 13:09:33 PDT
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 2009-10-04 17:14:55 PDT
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] 2009-10-05 04:16:46 PDT
Massimiliano ?
Comment 20 Wayne Mery (:wsmwk, NI for questions) 2009-10-05 05:25:02 PDT
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 2009-10-05 06:23:33 PDT
Sorry for my delay... was sick. My Thunderbird version runing on windows XP. Can I make a debug?
Comment 22 David :Bienvenu 2009-10-05 06:39:49 PDT
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 2009-10-06 01:05:15 PDT
Problem on point 2. The extension doesn't install to TB3b4
Comment 24 Ludovic Hirlimann [:Usul] 2009-10-06 01:37:03 PDT
(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 2009-10-06 01:44:00 PDT
(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 2009-10-06 01:45:43 PDT
(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 2009-10-07 01:41:25 PDT
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 2009-10-07 06:28:19 PDT
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 2009-10-07 06:46:42 PDT
The log doesn't exist (or I do not find)
Comment 30 Wayne Mery (:wsmwk, NI for questions) 2009-10-07 08:38:02 PDT
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 2009-10-07 09:02:13 PDT
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, NI for questions) 2009-10-07 14:28:57 PDT
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 2009-10-07 14:41:58 PDT
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 2009-10-08 01:51:27 PDT
Created attachment 405232 [details]
jsconsole.log

This my jsconsole.log with some errors.
Comment 35 David :Bienvenu 2009-10-08 06:59:37 PDT
Does this crash happen if you disable Lightning and restart?
Comment 36 Massimiliano Casini 2009-10-08 07:48:49 PDT
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 2009-10-08 08:16:57 PDT
I'm pretty sure there's a way to get more logging out of gloda, besides just warnings and errors
Comment 38 David :Bienvenu 2009-10-09 16:15:56 PDT
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 40 Massimiliano Casini 2009-10-12 08:19:05 PDT
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 2009-10-12 08:22:01 PDT
great, thanks, Massimiliano.
Comment 42 David :Bienvenu 2009-10-12 10:03:47 PDT
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 2009-10-13 01:43:13 PDT
Created attachment 405993 [details]
local folder with one email broken

unzip file to local folder
Comment 44 David :Bienvenu 2009-10-13 08:31:29 PDT
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 2009-10-13 08:39:33 PDT
Created attachment 406026 [details] [diff] [review]
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] 2009-10-13 11:11:31 PDT
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 2009-10-13 11:18:09 PDT
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 2009-10-13 11:49:17 PDT
Created attachment 406054 [details]
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 2009-10-13 11:53:08 PDT
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] 2009-10-13 12:26:26 PDT
(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 2009-10-13 12:39:47 PDT
yes, that's what I meant I was going to do.
Comment 52 David :Bienvenu 2009-10-13 13:34:14 PDT
Created attachment 406070 [details] [diff] [review]
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 2009-10-13 13:38:07 PDT
I audited the rest of the patch in bug 413874 and that was the only instance of this mistake.
Comment 54 David :Bienvenu 2009-10-14 08:03:32 PDT
fix checked in.
Comment 55 David :Bienvenu 2009-10-14 08:06:17 PDT
Created attachment 406227 [details] [diff] [review]
fix for 1.8 branch

this is just the core port of the patch that just landed on the trunk
Comment 56 David :Bienvenu 2009-10-15 08:53:51 PDT
Massimiliano, many thanks for the test case.
Comment 57 Wayne Mery (:wsmwk, NI for questions) 2009-10-15 13:29:16 PDT
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 2009-10-15 13:32:19 PDT
(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] 2010-01-19 12:45:48 PST
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 2010-01-19 13:32:43 PST
fixed for 1.8.1.24
Comment 61 [On PTO until 6/29] 2010-02-12 17:45:03 PST
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?

Note You need to log in before you can comment on or make changes to this bug.