Closed Bug 537404 Opened 10 years ago Closed 10 years ago

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

Categories

(MailNews Core :: Composition, defect, critical)

x86
All
defect
Not set
critical

Tracking

(blocking-thunderbird3.1 -, thunderbird3.1 beta2-fixed)

VERIFIED FIXED
Tracking Status
blocking-thunderbird3.1 --- -
thunderbird3.1 --- beta2-fixed

People

(Reporter: wsmwk, Assigned: timeless)

References

(Blocks 1 open bug)

Details

(Keywords: crash, dogfood)

Crash Data

Attachments

(1 file, 1 obsolete file)

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
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.
mailnews.sendInBackground = true
mailnews.show_send_progress = false
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
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
 742 nhotta 1.291 // notify the change to editor 
 743 m_editor->SetDocumentCharacterSet(NS_ConvertASCIItoUCS2(charset).get()); 

m_editor is null.
Blocks: 103282
Attached patch untested (obsolete) — Splinter Review
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #419802 - Flags: review?
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.
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 :)
Keywords: dogfood
Attachment #419802 - Flags: superreview?(bienvenu)
Attachment #419802 - Flags: review?(bugzilla)
Attachment #419802 - Flags: review?
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.
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.
Attached patch untestedSplinter Review
Attachment #419802 - Attachment is obsolete: true
Attachment #420303 - Flags: superreview?(bienvenu)
Attachment #420303 - Flags: review?(bugzilla)
Attachment #419802 - Flags: superreview?(bienvenu)
Attachment #419802 - Flags: review?(bugzilla)
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...
fine by me, you're the reviewer, stamp your stamps and have someone push ;-).
Attachment #420303 - Flags: superreview?(bienvenu) → superreview+
hopefully still in standar8's review queue :)
seen again today, using 3.1.  bp-63e48c31-7f63-4393-b7d9-2d0c02100219
Whiteboard: [needs r+ standard8]
blocking1.9.2: --- → ?
blocking1.9.2: ? → ---
blocking-thunderbird3.1: --- → ?
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.
blocking-thunderbird3.1: ? → -
Flags: wanted-thunderbird+
Attachment #420303 - Flags: review?(bugzilla) → review+
Checked in: http://hg.mozilla.org/comm-central/rev/7e4acb72afe4
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [needs r+ standard8]
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.
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
Status: RESOLVED → VERIFIED
Crash Signature: [@ nsMsgCompose::SetDocumentCharset(char const*)]
You need to log in before you can comment on or make changes to this bug.