Closed Bug 732687 Opened 9 years ago Closed 7 years ago

Crash [@ nsMsgSendReport::DisplayReport] during compose, save to Drafts folder fails

Categories

(MailNews Core :: Composition, defect)

x86
All
defect
Not set
critical

Tracking

(thunderbird25 fixed, thunderbird_esr2425+ fixed)

RESOLVED FIXED
Thunderbird 26.0
Tracking Status
thunderbird25 --- fixed
thunderbird_esr24 25+ fixed

People

(Reporter: a_geek, Assigned: m_kato)

References

()

Details

(Keywords: crash, dataloss, topcrash, Whiteboard: [needs landing][gs])

Crash Data

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Linux i686; rv:10.0.2) Gecko/20100101 Firefox/10.0.2
Build ID: 20120215223356

Steps to reproduce:

I pressed "Send" for an email that was supposed to be signed and encrypted using GPG (ie, with EnigMail).


Actual results:

TB crashed (ID: bp-b4bd700c-ee04-46cc-be86-4ac722120303).


Expected results:

The email should just have been sent, and TB should not have crashed.
Severity: normal → critical
Keywords: crash
Summary: Trying to send an encrypted email → Crash [@ nsMsgSendReport::DisplayReport] when trying to send an encrypted email
Status: UNCONFIRMED → NEW
Crash Signature: [@ nsMsgSendReport::DisplayReport ]
Ever confirmed: true
Does it happens often ?
Enigmail does not seem to be involved in the crash stack (only in another thread).
ageek?

(In reply to Ludovic Hirlimann [:Usul] from comment #1)
> Does it happens often ?
Whiteboard: [closeme 2012-07-01]
I have by and large stopped using TB, so no statistical data from me. But I was able to send an encrypted email to myself w/o TB crashing.
whatever the cause is, you are not alone

bp-9ed40865-ef53-4b60-81d9-ca4822120524 v12.0.1 (judith)
0	xul.dll	nsMsgSendReport::DisplayReport	mailnews/compose/src/nsMsgSendReport.cpp:251
1	xul.dll	nsMsgComposeAndSend::Fail	mailnews/compose/src/nsMsgSend.cpp:3696
2	xul.dll	nsMsgComposeAndSend::GatherMimeAttachments	mailnews/compose/src/nsMsgSend.cpp:1079
3	xul.dll	nsMsgComposeAndSend::HackAttachments	mailnews/compose/src/nsMsgSend.cpp:2621
4	xul.dll	nsMsgComposeAndSend::Init	mailnews/compose/src/nsMsgSend.cpp:3335
5	xul.dll	nsMsgComposeAndSend::CreateAndSendMessage	mailnews/compose/src/nsMsgSend.cpp:4174
6	xul.dll	nsMsgCompose::_SendMsg	mailnews/compose/src/nsMsgCompose.cpp:1076
7	xul.dll	nsMsgCompose::SendMsg	mailnews/compose/src/nsMsgCompose.cpp:1270 


and in older versions nsMsgSendReport::DisplayReport(nsIPrompt*, int, int, unsigned int*) such as
bp-eee45a9b-0a0a-4a7b-80c0-72c3e2111228 version 8
Crash Signature: [@ nsMsgSendReport::DisplayReport ] → [@ nsMsgSendReport::DisplayReport ] [@ nsMsgSendReport::DisplayReport(nsIPrompt*, bool, bool, unsigned int*) ] [@ nsMsgSendReport::DisplayReport(nsIPrompt*, int, int, unsigned int*) ]
Component: General → Composition
Product: Thunderbird → MailNews Core
QA Contact: general → composition
Whiteboard: [closeme 2012-07-01]
Version: 10 → Trunk
nsIMsgSendReport::process_Current is -1, so mProcessReport[mCurrentProcess] is mProcessReport[-1] by nsMsgComposeAndSend::Fail().  Why does we use -1?
Attached patch possible fix (obsolete) — Splinter Review
Attached patch fix v2Splinter Review
Assignee: nobody → m_kato
Attachment #676042 - Attachment is obsolete: true
Attachment #676045 - Flags: review?(mconley)
Comment on attachment 676045 [details] [diff] [review]
fix v2

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

Passing this over to irving, since I think he's more qualified to review this.
Attachment #676045 - Flags: review?(mconley) → review?(irving)
Comment on attachment 676045 [details] [diff] [review]
fix v2

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

r+ with the changes suggested

::: mailnews/compose/src/nsMsgSendReport.cpp
@@ +166,5 @@
>      process = mCurrentProcess;
>  
> +  // if still process_Current, return error.
> +  if (process == process_Current)
> +    return NS_ERROR_ILLEGAL_VALUE;

I'd prefer the preceding six lines be replaced with

if (process == nsMsgSendReport::process_Current) {
  if (mCurrentProcess == nsMsgSendReport::process_Current)
    // We don't know what we're currently trying to do
    return NS_ERROR_ILLEGAL_VALUE;
  else
    process = mCurrentProcess;
}

to make the logic more obvious

@@ +190,5 @@
>      process = mCurrentProcess;
>  
> +  // if still process_Current, return error.
> +  if (process == process_Current)
> +    return NS_ERROR_ILLEGAL_VALUE;

Same change as for SetError()
Attachment #676045 - Flags: review?(irving) → review+
thanks for the patch and review.

this is a top 30 crash for TB17, so it would be cool to have this landed soon in aurora and nightly to get some baking, and hopefully in time to take it for 17.0.1
Flags: needinfo?(a_geek)
Keywords: topcrash
Duplicate of this bug: 803469
(pinged wrong geek)
Flags: needinfo?(m_kato)
I'm not sure what you want me to do, but I just tried to send signed and encrypted messages to myself. One attempt worked, one attempt failed, but TB 17 didn't crash.
Flags: needinfo?(a_geek)
Keywords: checkin-needed
If there are still changes required on this patch before checkin, we shouldn't have checkin-needed; otherwise, our friendly tree management folk might accidentally check it in without the needed changes.
Keywords: checkin-needed
(In reply to a_geek from comment #14)
> I'm not sure what you want me to do, but I just tried to send signed and
> encrypted messages to myself. One attempt worked, one attempt failed, but TB
> 17 didn't crash.

That's what I was afraid of - there may be an underlying bug that led to the crash, and improving the error handling could just be turning the crash into a silent failure. I'm OK with landing a fixed version of this patch, but we might want to keep this bug open (or open a new one) to track failure to encrypt messages.
increased to #23 topcrash now
Crash Signature: [@ nsMsgSendReport::DisplayReport ] [@ nsMsgSendReport::DisplayReport(nsIPrompt*, bool, bool, unsigned int*) ] [@ nsMsgSendReport::DisplayReport(nsIPrompt*, int, int, unsigned int*) ] → [@ nsMsgSendReport::DisplayReport ] [@ nsMsgSendReport::DisplayReport(nsIPrompt*, bool, bool, unsigned int*) ] [@ nsMsgSendReport::DisplayReport(nsIPrompt*, int, int, unsigned int*) ] [@ nsMsgSendReport::DisplayReport(nsIPrompt*, bool, bool tag_nsresul…
Keywords: helpwanted
OS: Linux → All
It's #25 top crasher in TB 17.0.2 so not a top crasher according to https://wiki.mozilla.org/CrashKill/Topcrash
Keywords: topcrash
(In reply to Irving Reid (:irving) from comment #16)
> (In reply to a_geek from comment #14)
> > I'm not sure what you want me to do, but I just tried to send signed and
> > encrypted messages to myself. One attempt worked, one attempt failed, but TB
> > 17 didn't crash.
> 
> That's what I was afraid of - there may be an underlying bug that led to the
> crash, and improving the error handling could just be turning the crash into
> a silent failure. I'm OK with landing a fixed version of this patch, but we
> might want to keep this bug open (or open a new one) to track failure to
> encrypt messages.

Most crash comments mention failure when *draft* is being saved. I'm in the process of determining whether it ever happens during sending.

That said, will it help to have a crash reporter involved in the bug in its current state?  Put another way, is there anything useful I can ask a crash reporter?
Flags: needinfo?(irving)
At this point I think we could land the fix as is, but keep our eyes open for reports of encryption failures.
Flags: needinfo?(irving)
I forget landing this.  I will land soon.
Flags: needinfo?(m_kato)
~90% of crash comments continue to mention Drafts folder. 

For users I've been able to reach, the factors involve Drafts folder being missing, corrupt or otherwise inaccessible (compact, held by a backup program, etc).
Keywords: helpwanted
Summary: Crash [@ nsMsgSendReport::DisplayReport] when trying to send an encrypted email → Crash [@ nsMsgSendReport::DisplayReport] involving Drafts folder
(In reply to Makoto Kato from comment #21)
> I forget landing this.  I will land soon.

gentle reminder :)
Status: NEW → ASSIGNED
dataloss for user, because it is during compose and the save failed.
Keywords: dataloss
Summary: Crash [@ nsMsgSendReport::DisplayReport] involving Drafts folder → Crash [@ nsMsgSendReport::DisplayReport] during compose, save to Drafts folder fails
(In reply to :Irving Reid from comment #20)
> At this point I think we could land the fix as is, but keep our eyes open
> for reports of encryption failures.

It would be nice for this to land in the next couple weeks so it can ride the full train to release of TB24.
Keywords: topcrash
Whiteboard: [needs landing]
Whiteboard: [needs landing] → [needs landing][gs]
(In reply to Wayne Mery (:wsmwk) from comment #25)
> (In reply to :Irving Reid from comment #20)
> > At this point I think we could land the fix as is, but keep our eyes open
> > for reports of encryption failures.
> 
> It would be nice for this to land in the next couple weeks so it can ride
> the full train to release of TB24. [meaning it needs to land by ~June 23]

Ah, I see m_kato comes back this week.
Flags: needinfo?(m_kato)
I've done Irving's requested changes, and I've just landed this on trunk:

https://hg.mozilla.org/comm-central/rev/53f97895faa2

I think it needs to soak a bit and the beta stats for it were looking low, so we'll probably get it into the first point release of 24, rather than 24 straight away.
Flags: needinfo?(m_kato)
Comment on attachment 676045 [details] [diff] [review]
fix v2

[Triage Comment]

We will, however, get it onto 24 for additional testing.
Attachment #676045 - Flags: approval-comm-aurora+
https://hg.mozilla.org/releases/comm-aurora/rev/1033b3b326a0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 26.0
Comment on attachment 676045 [details] [diff] [review]
fix v2

[Triage Comment]
Attachment #676045 - Flags: approval-comm-esr24+
You need to log in before you can comment on or make changes to this bug.