Closed Bug 505221 Opened 10 years ago Closed 10 years ago

crash [@ CountTotalMimeAttachments(MimeContainer*) ]

Categories

(MailNews Core :: MIME, defect, critical)

1.9.1 Branch
x86
Windows XP
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0rc1

People

(Reporter: Usul, Assigned: Bienvenu)

References

()

Details

(5 keywords, Whiteboard: [no l10n impact][sg:moderate] invalid read)

Crash Data

Attachments

(5 files, 1 obsolete file)

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
Flags: blocking-thunderbird3?
is this a top-crash or not? if so, we'd mark it blocking...
Not a top crasher, so not blocking but marking wanted.
Flags: wanted-thunderbird3+
Flags: blocking-thunderbird3?
Flags: blocking-thunderbird3-
now #1 topcrash, no doubt induced by gloda indexing
Flags: blocking-thunderbird3- → blocking-thunderbird3?
Keywords: topcrash
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.
Assignee: nobody → bienvenu
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Keywords: qawanted
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.
perhaps one our CC: can whip up a testcase
Keywords: testcase-wanted
bug 468394 may be a dupe but no testcase there either - mime_subclass_p(MimeObjectClass*, MimeObjectClass*)
Whiteboard: [no l10n impact]
Duplicate of this bug: 519425
(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 ?
Bienvenu you can tackle it too :-)
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.
Massimiliano, how do you feel about running a debug build ?
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.
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!
Sorry, I mean move the folder out my profile.
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).
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.
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...
Massimiliano ?
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
Sorry for my delay... was sick. My Thunderbird version runing on windows XP. Can I make a debug?
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.
Problem on point 2. The extension doesn't install to TB3b4
(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 ?
(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 ...
(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 ...
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
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).
The log doesn't exist (or I do not find)
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
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
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?
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.
Attached file jsconsole.log
This my jsconsole.log with some errors.
Does this crash happen if you disable Lightning and restart?
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
I'm pretty sure there's a way to get more logging out of gloda, besides just warnings and errors
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
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.
great, thanks, Massimiliano.
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)
unzip file to local folder
Group: core-security
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.
Attached patch fix crash (obsolete) — Splinter Review
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.
Attachment #406026 - Flags: superreview?
Attachment #406026 - Flags: review?(bugmail)
Whiteboard: [no l10n impact] → [no l10n impact][has patch for review]
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;
Attachment #406026 - Flags: review?(bugmail) → review+
Attachment #406026 - Flags: superreview? → superreview?(bugzilla)
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.
Whiteboard: [no l10n impact][has patch for review] → [no l10n impact][has patch for sr standard8]
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!
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.
Keywords: qawantedregression
Whiteboard: [no l10n impact][has patch for sr standard8] → [no l10n impact]
(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 ?
Flags: in-testsuite?
yes, that's what I meant I was going to do.
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.
Attachment #406026 - Attachment is obsolete: true
Attachment #406070 - Flags: superreview?(bugzilla)
Attachment #406070 - Flags: review?(bugmail)
Attachment #406026 - Flags: superreview?(bugzilla)
I audited the rest of the patch in bug 413874 and that was the only instance of this mistake.
Attachment #406070 - Flags: review?(bugmail) → review+
Attachment #406070 - Flags: superreview?(bugzilla) → superreview+
Target Milestone: --- → Thunderbird 3.0rc1
fix checked in.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
this is just the core port of the patch that just landed on the trunk
Attachment #406227 - Flags: approval1.8.1.next?
Massimiliano, many thanks for the test case.
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.
Blocks: 413874
(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.
Whiteboard: [no l10n impact] → [no l10n impact][sg:moderate] invalid read
Comment on attachment 406227 [details] [diff] [review]
fix for 1.8 branch

Approved for 1.8.1.24, a=dveditz for release-drivers
Attachment #406227 - Flags: approval1.8.1.next? → approval1.8.1.next+
fixed for 1.8.1.24
Keywords: fixed1.8.1.24
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?
Group: core-security
Crash Signature: [@ CountTotalMimeAttachments(MimeContainer*) ]
Flags: in-testsuite?
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.