Last Comment Bug 752768 - Ubuntu startup crash in nsMsgMailSession::OnItemAdded (sig: NS_ReadLine)
: Ubuntu startup crash in nsMsgMailSession::OnItemAdded (sig: NS_ReadLine)
Status: VERIFIED FIXED
[startupcrash][gs][regression: TB12] ...
: crash, topcrash
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: x86 Linux
: -- critical (vote)
: Thunderbird 18.0
Assigned To: Kent James (:rkent)
:
Mentors:
https://getsatisfaction.com/mozilla_m...
Depends on:
Blocks: 124287
  Show dependency treegraph
 
Reported: 2012-05-07 18:12 PDT by Wayne Mery (:wsmwk, NI for questions)
Modified: 2012-10-18 05:17 PDT (History)
11 users (show)
rkent: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
-
-
fixed
-
fixed


Attachments
Test that crashes with correct stack (4.51 KB, patch)
2012-09-12 17:13 PDT, Kent James (:rkent)
no flags Details | Diff | Review
Rev2: with crash fix (5.36 KB, patch)
2012-09-12 17:47 PDT, Kent James (:rkent)
irving: review+
Details | Diff | Review
Patch as landed (added MPL header to test), r=irving (5.59 KB, patch)
2012-09-17 11:34 PDT, Kent James (:rkent)
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Review

Description Wayne Mery (:wsmwk, NI for questions) 2012-05-07 18:12:16 PDT
This bug was filed from the Socorro interface and is 
report bp-407e5914-e576-4934-a618-0aa152120504 .
============================================================= 
all crashes are version 12.0.1

0	libxul.so	NS_ReadLine<char, nsIInputStream, nsCAutoString>	nsReadLine.h:150
1	libxul.so	MsgStreamMsgHeaders	nsMsgUtils.cpp:2220
2	libxul.so	nsMailboxService::StreamHeaders	nsMailboxService.cpp:355
3	libxul.so	libxul.so@0x100292b	
4	libxul.so	XPCWrappedNative::CallMethod	XPCWrappedNative.cpp:2901
5	libxul.so	XPC_WN_CallMethod	XPCWrappedNativeJSOps.cpp:1542
6	libxul.so	js::InvokeKernel	jscntxtinlines.h:311
7	libxul.so	js::Interpret	jsinterp.cpp:2791
8	libxul.so	js::InvokeKernel	jsinterp.cpp:537
9	libxul.so	js::Invoke	jsinterp.h:157
10	libxul.so	JS_CallFunctionValue	jsapi.cpp:5449
11	libxul.so	nsXPCWrappedJSClass::CallMethod	XPCWrappedJSClass.cpp:1510
12	libxul.so	nsXPCWrappedJS::CallMethod	XPCWrappedJS.cpp:617
13	libxul.so	PrepareAndDispatch	xptcstubs_gcc_x86_unix.cpp:92
14	libxul.so	nsMsgMailSession::OnItemAdded	nsMsgMailSession.cpp:193
15	libxul.so	nsMsgDBFolder::NotifyItemAdded	nsMsgDBFolder.cpp:5004 


selected comments
I've just renamed a local folder
I went offline, started compact all folders and have restarted afterwards.
I went offline, compacted all folders and have restarted afterwards - problem seems not to occure if i stay offline!?
I got to read an e-mail this time (last crash about 3 minutes ago). What's up with TB today?
Comment 2 Wayne Mery (:wsmwk, NI for questions) 2012-05-25 12:52:17 PDT
(see if socorro picks up signature @ NS_ReadLine. doubtful)
Comment 3 Ben Bucksch (:BenB) 2012-06-19 04:42:37 PDT
A friend of mine (not computer expert) is seeing this on this Ubuntu 12.04 machine, too.

> I've just renamed a local folder

We did the same.
Comment 4 Ben Bucksch (:BenB) 2012-06-19 05:29:51 PDT
Using the official Thunderbird 13 x64 works
http://releases.mozilla.org/pub/mozilla.org/thunderbird/releases/13.0.1/linux-x86_64/en-GB/
Comment 5 Ben Bucksch (:BenB) 2012-06-19 05:30:17 PDT
Could somebody please report this back to Ubuntu with high priority?
Comment 6 Vincent (caméléon) 2012-06-20 04:45:08 PDT
also reported here: http://forum.ubuntu-fr.org/viewtopic.php?pid=9720811
Comment 7 Vincent (caméléon) 2012-06-27 00:46:59 PDT
(In reply to Ben Bucksch (:BenB) from comment #5)
> Could somebody please report this back to Ubuntu with high priority?

Done: https://bugs.launchpad.net/thunderbird/+bug/1016818
Comment 8 Micah Gersten 2012-06-27 16:44:14 PDT
That launchpad bug was for @ nsResProtocolHandler::nsResProtocolHandler
Comment 9 Vincent (caméléon) 2012-07-17 02:01:11 PDT
Another user report the problem here (on Geckozone): http://www.geckozone.org/forum/viewtopic.php?f=4&t=106780&p=698304&e=698304
Is there anything that can be done to help you understand the issue?
Comment 11 Ben Bucksch (:BenB) 2012-08-15 12:24:36 PDT
My friend from comment 3 also is located in France, but he's English and I think using the English locale in Ubuntu (OS and user account). He's using a French keyboard, Paris time, and Orange as ISP.

The crash is in nsMsgDBFolder::NotifyItemAdded(), so my guess would be that the trigger are French emails (of which he also gets a lot, e.g. from LeMonde) with French accents and these chars in the emails are triggering the crash.
Comment 12 Wayne Mery (:wsmwk, NI for questions) 2012-08-30 06:45:05 PDT
hmm. I haven't found the windows equivalent signature. No crash sigs contain nsMsgMailSession::OnItemAdded.

(There are crashes for the frame below, nsMsgDBFolder::NotifyItemAdded. but they are not startup - for example nsMsgDBFolder::NotifyItemAdded(nsISupports*) 2febe726-4c20-4adc-84ae-b113d2120829 )


first crash is a single TB12 beta build 20120315033921 bp-5f9865eb-73a8-42d7-9524-e8a9b2120322

Not strictly match to bug 124287's title, but WTH
Comment 13 Wayne Mery (:wsmwk, NI for questions) 2012-09-12 00:51:45 PDT
gee, no solid progress on this topcrash regression?
Comment 14 Kent James (:rkent) 2012-09-12 13:25:55 PDT
AFAICT streamHeaders is not used in the core code, and was originally added to support extensions (primarily Conversations) It also appears from the crash dump to be coming from a javascript user.

Looking at the extension list on the crash reports, I see at least one that claims no extensions loaded, which confuses me. But still, there is no error checking in the call to NS_ReadLine, and the likely failure is a null aStream here:

116       nsresult rv = aStream->Read(aBuffer->buf, kLineBufferSize, &bytesRead);

Looking at where that comes from, in nsMailboxService.cpp we have


321   rv = folder->GetOfflineFileStream(msgKey, &messageOffset, &messageSize, getter_AddRefs(inputStream));
322   NS_ENSURE_SUCCESS(rv, rv);
323   return MsgStreamMsgHeaders(inputStream, aConsumer);

Is the rv test enough? No, because in GetOfflineFileStream we have:

  nsresult rv = GetDatabase();
  NS_ENSURE_SUCCESS(rv, NS_OK);

That is, any failure to get the database returns OK but a null filestream.

This is exactly why I dislike using NS_ENSURE_SUCCESS(rv, rv) when what I really care about is a non-null pointer. In my own code, I would have used instead:

NS_ENSURE_STATE(inputStream) and this would have been prevented.

I'll take this and do a fix.
Comment 15 Kent James (:rkent) 2012-09-12 17:13:05 PDT
Created attachment 660626 [details] [diff] [review]
Test that crashes with correct stack

This xpcshell test generates the crash. Fix to follow.
Comment 16 Kent James (:rkent) 2012-09-12 17:15:29 PDT
Jonathan, in your Conversations addon you disable the call to streamHeaders. Is it because of the crash that occurs here?
Comment 17 Kent James (:rkent) 2012-09-12 17:47:31 PDT
Created attachment 660636 [details] [diff] [review]
Rev2: with crash fix
Comment 18 Jonathan Protzenko [:protz] 2012-09-12 23:05:38 PDT
I disable the call to streamHeaders because that function just doesn't work: it sometimes returns part of the header, and the first few bytes of the headers are missing. I once tried to debug this with Bienvenu but we didn't get far. If you were to find out what's happening, that would be fantastic :)
Comment 19 Chris Coulson 2012-09-13 02:15:55 PDT
Oh, my, I was scratching my head whilst looking at this yesterday because I thought the crash was happening here:

http://hg.mozilla.org/mozilla-central/file/f36b93c70d05/netwerk/base/public/nsReadLine.h#l150

... based on the line numbers in https://crash-stats.mozilla.com/report/index/407e5914-e576-4934-a618-0aa152120504. Now I've just realized that report is from an old version before the license header change ;)

Anyway, thanks for spotting that. Incidentally, I have an addon which calls streamHeaders too...
Comment 20 Ben Bucksch (:BenB) 2012-09-13 02:28:12 PDT
> GetOfflineFileStream we have:
>   nsresult rv = GetDatabase();
>   NS_ENSURE_SUCCESS(rv, NS_OK);
> That is, any failure to get the database returns OK but a null filestream.

That's either just wrong, a bug in GetDatabase(), or - if it's clearly documented -, a mean API that the caller here misunderstood. If the latter, please fix GetDatabase(), not the caller.

> AFAICT streamHeaders is not used in the core code

FYI, I personally saw the bug on a machine with a fresh installation of Ubuntu and Ubuntu's Thunderbird package. The only extensions were those added automatically by Ubuntu. So, these would be a place to look at.
Comment 21 Jonathan Protzenko [:protz] 2012-09-13 03:02:06 PDT
Chris, I've had consistently wrong results with streamHeaders, I've tried debugging these for a while, and I couldn't get anywhere. Looks like the backend gets the offset of the message wrong and starts streaming at a wrong offset...
Comment 22 Wayne Mery (:wsmwk, NI for questions) 2012-09-13 03:15:24 PDT
protz, is there a bug report on streamHeaders?
Comment 23 Ben Bucksch (:BenB) 2012-09-13 03:27:53 PDT
(In reply to Jonathan Protzenko [:protz] from comment #21)
> I've had consistently wrong results with streamHeaders ...
> starts streaming at a wrong offset...

Oh, that's interesting! This might be the cause of bug 790912.
Comment 24 Kent James (:rkent) 2012-09-13 06:34:44 PDT
(In reply to Ben Bucksch (:BenB) from comment #20)
> > GetOfflineFileStream we have:
> >   nsresult rv = GetDatabase();
> >   NS_ENSURE_SUCCESS(rv, NS_OK);
> > That is, any failure to get the database returns OK but a null filestream.
> 
> That's either just wrong, a bug in GetDatabase(), or - if it's clearly
> documented -, a mean API that the caller here misunderstood. If the latter,
> please fix GetDatabase(), not the caller.

For a crash fix like this that I hope to get landed in a .0.* version, I tend to be extremely cautious in what I change. Because GetOfflineFileStream is used in places other than streamHeader, I am reluctant to change its behavior in a .0.* version. I agree though that the NS_ENSURE_SUCCESS(rv, NS_OK) violates expectations and should be changed, just not in this patch.

> 
> > AFAICT streamHeaders is not used in the core code
> 
> FYI, I personally saw the bug on a machine with a fresh installation of
> Ubuntu and Ubuntu's Thunderbird package.

As I mentioned, I also noted crash reports with no extensions installed, so I am also concerned here. But the issue I have identified in the patch is real, the unit test crashes with the correct signature without the fix, so this patch should be landed IMHO.
Comment 25 :Irving Reid (No longer working on Firefox) 2012-09-17 07:30:05 PDT
Comment on attachment 660636 [details] [diff] [review]
Rev2: with crash fix

Review of attachment 660636 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks fine. We should land this, but I'd like to keep either this or a related bug open to understand why we're failing to open the offline stream in these cases and make GetOfflineFileStream behave better.
Comment 26 Kent James (:rkent) 2012-09-17 11:34:47 PDT
Created attachment 661854 [details] [diff] [review]
Patch as landed (added MPL header to test), r=irving

Checked in https://hg.mozilla.org/comm-central/rev/c05d43695bef
Comment 27 Ben Bucksch (:BenB) 2012-09-17 17:43:21 PDT
> I'd like to keep either this or a related bug open
> to ... make GetOfflineFileStream behave better.
Comment 28 Kent James (:rkent) 2012-09-17 17:51:56 PDT
Thanks for catching that I forgot that, Ben! But let's do it in a new Bug 791893.
Comment 29 Mark Banner (:standard8) 2012-09-25 07:54:26 PDT
Not significant enough for tracking...
Comment 30 Mark Banner (:standard8) 2012-09-25 07:55:51 PDT
Comment on attachment 661854 [details] [diff] [review]
Patch as landed (added MPL header to test), r=irving

[Triage Comment]
...I will take it onto aurora and beta though, as I think this is a low risk enough simple null-check addition.

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