537404 - crash [@ nsMsgCompose::SetDocumentCharset(char const*)]

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect)

Description

[Wayne Mery (:wsmwk)]

crash [@ nsMsgCompose::SetDocumentCharset(char const*)]

on restart a message was still in my Outbox, which sent OK when I did "send now".  subject "need complete exit of Mozilla for new dictionary to work - but it LOOKS like it does".  absolutely nothing unusual about this message.

xref  Bug 301264 -  Crash [@ nsMsgCompose::SetDocumentCharset] closed several months ago

only 5 crashes in last 3 months, all 3.0.0 + my trunk crash
bp-1dd24be5-f62f-4ac7-a7ae-435692091231
0	thunderbird.exe	nsMsgCompose::SetDocumentCharset	 mailnews/compose/src/nsMsgCompose.cpp:897
1	xpcom_core.dll	NS_InvokeByIndex_P	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
2	thunderbird.exe	XPCWrappedNative::CallMethod	js/src/xpconnect/src/xpcwrappednative.cpp:2727
3	thunderbird.exe	XPC_WN_CallMethod	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1756
4	mozjs.dll	js_Invoke	js/src/jsinterp.cpp:1376
5	mozjs.dll	js_Interpret	js/src/jsops.cpp:2297
6	mozjs.dll	js_Invoke	js/src/jsinterp.cpp:1384
7	thunderbird.exe	nsXPCWrappedJSClass::CallMethod	js/src/xpconnect/src/xpcwrappedjsclass.cpp:1696 


two others:
bp-3e47b339-0cf4-4514-b71d-e44782091217
bp-3c7d35da-44e6-4379-90a3-cd4bf2091221  Mac

Comment 1

[Wayne Mery (:wsmwk)]

happened again with 3.2a1 bp-ed7d2160-a7b5-48c0-baa8-62fa12091231
no attachments involved.

back in 3.1a1, I sent a message with no problem.

Comment 2

[Wayne Mery (:wsmwk)]

mailnews.sendInBackground = true
mailnews.show_send_progress = false

Comment 3

[Wayne Mery (:wsmwk)]

can't figure out why this has just started happening...i've been using mailnews.sendInBackground for a long time

seen again bp-c253bb50-49d5-4f4d-96e5-913a12100102

Comment 4

[Wayne Mery (:wsmwk)]

got to see this one - compose window went black. 
but not sure if that's behavior normal for send in background 
bp-fe3156ba-955e-4035-ae69-755492100102

Comment 5

[timeless]

 742 nhotta 1.291 // notify the change to editor 
 743 m_editor->SetDocumentCharacterSet(NS_ConvertASCIItoUCS2(charset).get()); 

m_editor is null.

Comment 6

[timeless]

Attachment: untested

Comment 7

[Wayne Mery (:wsmwk)]

It may be pure chance, but I haven't seen this with 3.0.x.

There was a rash of crashes for 3.0b2pre with same top of stack that may have been fixed by bug 459443.  if it fixed anything it didn't get everything, bp-63d2ac6c-9c09-4ef2-9d8a-10e4a2090609 is 3.0b2.

Comment 8

[Wayne Mery (:wsmwk)]

continue to not see this in 3.0.x - so this is a dogfood bug for me, because I am completely addicted to mailnews.sendInBackground = true :)

Comment 9

[David :Bienvenu]

Comment on attachment 419802 [details] [diff] [review]
untested

you can use nsDependentCString to wrap charset, I believe.

And I think it would be more helpful to actually make the code make sense instead of pointing out that it doesn't, since it's should be trivial to do so:

   m_editor->EndTransaction();
-
+  // XXX the preceding and subsequent lines do not make sense
   if (m_editor)

We check for a null editor at the top, which makes me think all the remaining null checks are no longer needed...the patch in bug 137629 which introduced this code looks to have been a bit confused.

Comment 10

[timeless]

nsDependentCString isn't null safe, which is the original crash.

the reason i was hesitant to simply remove the null checks is that in theory it could be a problem relating to reentrancy. but if you're certain there's no risk, yeah i can remove that stuff.

Comment 11

[timeless]

Attachment: untested

Comment 12

[David :Bienvenu]

I was worried about reentrancy as well, so I looked at the code that could clear m_editor, and looked at the original patch. I'm reasonably confident that m_editor won't be cleared reentrantly and I'm sure that the original patch wasn't trying to deal with reentrancy - it just did a global search and replace...

Comment 13

[timeless]

fine by me, you're the reviewer, stamp your stamps and have someone push ;-).

Comment 14

[Wayne Mery (:wsmwk)]

hopefully still in standar8's review queue :)
seen again today, using 3.1.  bp-63e48c31-7f63-4393-b7d9-2d0c02100219

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

From my understanding of the bug comments, this isn't all that frequent, which makes me think we wouldn't block on it, so setting blocking- but wanted+.  Wayne, if you think that's not the right analysis, please renominate with comments about why it's not.

Comment 16

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/7e4acb72afe4

Comment 17

[Wayne Mery (:wsmwk)]

FTR, my main concern for driving this in is because it impacts background send. And since background send is now not a goal for 3.1, not blocking is correct. Thanks all involved for the patch.

Comment 18

[Wayne Mery (:wsmwk)]

crash-stats is clean for all builds since the checkin. And only one 3.0.x crash, so this doesn't warrant driving into 3.0.x