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

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[a_geek]

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.

Comment 1

[Ludovic Hirlimann [:Usul]]

Does it happens often ?

Comment 2

[Patrick Brunschwig]

Enigmail does not seem to be involved in the crash stack (only in another thread).

Comment 3

[Wayne Mery (:wsmwk)]

ageek?

(In reply to Ludovic Hirlimann [:Usul] from comment #1)
> Does it happens often ?

Comment 4

[a_geek]

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.

Comment 5

[Wayne Mery (:wsmwk)]

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

Comment 6

[Makoto Kato [:m_kato]]

nsIMsgSendReport::process_Current is -1, so mProcessReport[mCurrentProcess] is mProcessReport[-1] by nsMsgComposeAndSend::Fail().  Why does we use -1?

Comment 7

[Makoto Kato [:m_kato]]

Attachment: possible fix

Comment 8

[Makoto Kato [:m_kato]]

Attachment: fix v2

Comment 9

[Mike Conley (: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.

Comment 10

[:Irving Reid (No longer working on Firefox)]

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()

Comment 11

[Wayne Mery (:wsmwk)]

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

Comment 13

[Wayne Mery (:wsmwk)]

(pinged wrong geek)

Comment 14

[a_geek]

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.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 16

[:Irving Reid (No longer working on Firefox)]

(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.

Comment 17

[Wayne Mery (:wsmwk)]

increased to #23 topcrash now

Comment 18

[Scoobidiver (away)]

It's #25 top crasher in TB 17.0.2 so not a top crasher according to https://wiki.mozilla.org/CrashKill/Topcrash

Comment 19

[Wayne Mery (:wsmwk)]

(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?

Comment 20

[:Irving Reid (No longer working on Firefox)]

At this point I think we could land the fix as is, but keep our eyes open for reports of encryption failures.

Comment 21

[Makoto Kato [:m_kato]]

I forget landing this.  I will land soon.

Comment 22

[Wayne Mery (:wsmwk)]

~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).

Comment 23

[Wayne Mery (:wsmwk)]

(In reply to Makoto Kato from comment #21)
> I forget landing this.  I will land soon.

gentle reminder :)

Comment 24

[Wayne Mery (:wsmwk)]

dataloss for user, because it is during compose and the save failed.

Comment 25

[Wayne Mery (:wsmwk)]

(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.

Comment 26

[Wayne Mery (:wsmwk)]

(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.

Comment 27

[Mark Banner (:standard8)]

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.

Comment 28

[Mark Banner (:standard8)]

Comment on attachment 676045 [details] [diff] [review]
fix v2

[Triage Comment]

We will, however, get it onto 24 for additional testing.

Comment 29

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-aurora/rev/1033b3b326a0

Comment 30

[Mark Banner (:standard8)]

Comment on attachment 676045 [details] [diff] [review]
fix v2

[Triage Comment]

Comment 31

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-esr24/rev/86f515ee29a0