Closed Bug 752768 Opened 12 years ago Closed 12 years ago

Ubuntu startup crash in nsMsgMailSession::OnItemAdded (sig: NS_ReadLine)

Categories

(MailNews Core :: Backend, defect)

x86
Linux
defect
Not set
critical

Tracking

(thunderbird15-, thunderbird16- fixed, thunderbird17- fixed)

VERIFIED FIXED
Thunderbird 18.0
Tracking Status
thunderbird15 - ---
thunderbird16 - fixed
thunderbird17 - fixed

People

(Reporter: wsmwk, Assigned: rkent)

References

(Blocks 1 open bug, )

Details

(Keywords: crash, topcrash, Whiteboard: [startupcrash][gs][regression: TB12] Maybe triggered by emails with French chars?)

Crash Data

Attachments

(1 file, 2 obsolete files)

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?
(see if socorro picks up signature @ NS_ReadLine. doubtful)
Crash Signature: [@ NS_ReadLine<char, nsIInputStream, nsCAutoString>] → [@ NS_ReadLine<char, nsIInputStream, nsCAutoString>] [@ NS_ReadLine]
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.
Could somebody please report this back to Ubuntu with high priority?
(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
That launchpad bug was for @ nsResProtocolHandler::nsResProtocolHandler
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?
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.
Whiteboard: [startupcrash] → [startupcrash] Maybe triggered by emails with French chars?
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
Summary: Ubuntu startup crash in NS_ReadLine → Ubuntu startup crash in nsMsgMailSession::OnItemAdded (sig: NS_ReadLine)
Whiteboard: [startupcrash] Maybe triggered by emails with French chars? → [startupcrash][gs][regression: TB12] Maybe triggered by emails with French chars?
gee, no solid progress on this topcrash regression?
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.
QA Contact: kent
Assignee: nobody → kent
Status: NEW → ASSIGNED
QA Contact: kent
This xpcshell test generates the crash. Fix to follow.
Jonathan, in your Conversations addon you disable the call to streamHeaders. Is it because of the crash that occurs here?
Attached patch Rev2: with crash fix (obsolete) — Splinter Review
Attachment #660626 - Attachment is obsolete: true
Attachment #660636 - Flags: review?(irving)
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 :)
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...
> 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.
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...
protz, is there a bug report on streamHeaders?
(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.
(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 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.
Attachment #660636 - Flags: review?(irving) → review+
Assignee: kent → nobody
Component: General → Backend
Flags: in-testsuite+
Product: Thunderbird → MailNews Core
Target Milestone: --- → Thunderbird 18.0
Assignee: nobody → kent
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
> I'd like to keep either this or a related bug open
> to ... make GetOfflineFileStream behave better.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Thanks for catching that I forgot that, Ben! But let's do it in a new Bug 791893.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Not significant enough for tracking...
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.
Attachment #661854 - Flags: approval-comm-beta+
Attachment #661854 - Flags: approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.