The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in Thunderbird 18.0

Status

MailNews Core
Backend
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: wsmwk, Assigned: rkent)

Tracking

(Blocks: 1 bug, {crash, topcrash})

Trunk
Thunderbird 18.0
x86
Linux
crash, topcrash
Bug Flags:
in-testsuite +

Thunderbird Tracking Flags

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

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

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?
#4 crasher for thunderbird 12.0.1, and #1 crash for linux

https://crash-stats.mozilla.com/report/list?version=Thunderbird%3A12.0.1&query_search=signature&query_type=contains&reason_type=contains&date=2012-05-25&range_value=28&range_unit=days&hang_type=any&process_type=any&signature=NS_ReadLine%3Cchar%2C%20nsIInputStream%2C%20nsCAutoString%3E
Keywords: topcrash
Summary: startup crash in NS_ReadLine → Ubuntu startup crash in NS_ReadLine
Whiteboard: [startupcrash]
(see if socorro picks up signature @ NS_ReadLine. doubtful)
Crash Signature: [@ NS_ReadLine<char, nsIInputStream, nsCAutoString>] → [@ NS_ReadLine<char, nsIInputStream, nsCAutoString>] [@ NS_ReadLine]

Comment 3

5 years ago
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

5 years ago
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

5 years ago
Could somebody please report this back to Ubuntu with high priority?

Comment 6

5 years ago
also reported here: http://forum.ubuntu-fr.org/viewtopic.php?pid=9720811

Comment 7

5 years ago
(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

Updated

5 years ago

Comment 8

5 years ago
That launchpad bug was for @ nsResProtocolHandler::nsResProtocolHandler

Comment 9

5 years ago
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 10

5 years ago
A nother one in Ubuntu french forum.
http://forum.ubuntu-fr.org/viewtopic.php?pid=10388521#p10388521
https://crash-stats.mozilla.com/report/index/bp-24137983-cfe5-4d6c-93e3-e70292120813

It's only for french user ??

Comment 11

5 years ago
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.

Updated

5 years ago
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
Blocks: 124287
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?
(Assignee)

Comment 14

5 years ago
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.
(Assignee)

Updated

5 years ago
QA Contact: kent
(Assignee)

Updated

5 years ago
Assignee: nobody → kent
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
QA Contact: kent
(Assignee)

Comment 15

5 years ago
Created attachment 660626 [details] [diff] [review]
Test that crashes with correct stack

This xpcshell test generates the crash. Fix to follow.
(Assignee)

Comment 16

5 years ago
Jonathan, in your Conversations addon you disable the call to streamHeaders. Is it because of the crash that occurs here?
(Assignee)

Comment 17

5 years ago
Created attachment 660636 [details] [diff] [review]
Rev2: with crash fix
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 :)

Comment 19

5 years ago
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

5 years ago
> 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?

Comment 23

5 years ago
(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.
(Assignee)

Comment 24

5 years ago
(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.
(Assignee)

Updated

5 years ago
tracking-thunderbird15: --- → ?
tracking-thunderbird16: --- → ?
tracking-thunderbird17: --- → ?
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)

Comment 26

5 years ago
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
Attachment #660636 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Assignee: kent → nobody
Component: General → Backend
Flags: in-testsuite+
Product: Thunderbird → MailNews Core
Target Milestone: --- → Thunderbird 18.0
(Assignee)

Updated

5 years ago
Assignee: nobody → kent
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 27

5 years ago
> I'd like to keep either this or a related bug open
> to ... make GetOfflineFileStream behave better.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 28

5 years ago
Thanks for catching that I forgot that, Ben! But let's do it in a new Bug 791893.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Not significant enough for tracking...
tracking-thunderbird15: ? → -
tracking-thunderbird16: ? → -
tracking-thunderbird17: ? → -
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+
Checked in:

http://hg.mozilla.org/releases/comm-aurora/rev/4ad0797d11d0
https://hg.mozilla.org/releases/comm-beta/rev/80fd034a8f21
status-thunderbird16: --- → fixed
status-thunderbird17: --- → fixed
v.fixed

no tb16 crashes in https://crash-stats.mozilla.com/report/list?product=Thunderbird&query_search=signature&query_type=exact&query=NS_ReadLine%26lt%3Bchar%2C%20nsIInputStream%2C%20nsCAutoString%26gt%3B&reason_type=contains&date=10%2F18%2F2012%2012%3A15%3A22&range_value=4&range_unit=weeks&hang_type=any&process_type=all&do_query=1&admin=1&signature=NS_ReadLine%3Cchar%2C%20nsIInputStream%2C%20nsCAutoString%3E
and none on trunk
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.