Closed
Bug 732687
Opened 13 years ago
Closed 11 years ago
Crash [@ nsMsgSendReport::DisplayReport] during compose, save to Drafts folder fails
Categories
(MailNews Core :: Composition, defect)
Tracking
(thunderbird25 fixed, thunderbird_esr2425+ fixed)
RESOLVED
FIXED
Thunderbird 26.0
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)
3.85 KB,
patch
|
Irving
:
review+
standard8
:
approval-comm-aurora+
standard8
:
approval-comm-esr24+
|
Details | Diff | Splinter Review |
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
Updated•13 years ago
|
Status: UNCONFIRMED → NEW
Crash Signature: [@ nsMsgSendReport::DisplayReport ]
Ever confirmed: true
Comment 2•13 years ago
|
||
Enigmail does not seem to be involved in the crash stack (only in another thread).
Comment 3•12 years ago
|
||
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.
Comment 5•12 years ago
|
||
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
Assignee | ||
Comment 6•12 years ago
|
||
nsIMsgSendReport::process_Current is -1, so mProcessReport[mCurrentProcess] is mProcessReport[-1] by nsMsgComposeAndSend::Fail(). Why does we use -1?
Assignee | ||
Comment 7•12 years ago
|
||
Assignee | ||
Comment 8•12 years ago
|
||
Assignee: nobody → m_kato
Attachment #676042 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #676045 -
Flags: review?(mconley)
Comment 9•12 years ago
|
||
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 10•12 years ago
|
||
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+
Comment 11•12 years ago
|
||
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
Reporter | ||
Comment 14•12 years ago
|
||
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)
Updated•12 years ago
|
Keywords: checkin-needed
Comment 15•12 years ago
|
||
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
Comment 16•12 years ago
|
||
(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•12 years ago
|
||
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
Comment 18•12 years ago
|
||
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
Comment 19•12 years ago
|
||
(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)
Comment 20•12 years ago
|
||
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)
Assignee | ||
Comment 21•12 years ago
|
||
I forget landing this. I will land soon.
Flags: needinfo?(m_kato)
Comment 22•12 years ago
|
||
~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
Comment 23•12 years ago
|
||
(In reply to Makoto Kato from comment #21) > I forget landing this. I will land soon. gentle reminder :)
Status: NEW → ASSIGNED
Comment 24•12 years ago
|
||
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
Comment 25•11 years ago
|
||
(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]
Updated•11 years ago
|
Whiteboard: [needs landing] → [needs landing][gs]
Updated•11 years ago
|
tracking-thunderbird24:
--- → ?
Comment 26•11 years ago
|
||
(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)
Updated•11 years ago
|
Comment 27•11 years ago
|
||
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 28•11 years ago
|
||
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+
Comment 29•11 years ago
|
||
https://hg.mozilla.org/releases/comm-aurora/rev/1033b3b326a0
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
status-thunderbird25:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 26.0
Updated•11 years ago
|
tracking-thunderbird24:
+ → ---
tracking-thunderbird_esr24:
--- → 25+
Comment 30•11 years ago
|
||
Comment on attachment 676045 [details] [diff] [review] fix v2 [Triage Comment]
Attachment #676045 -
Flags: approval-comm-esr24+
Comment 31•11 years ago
|
||
https://hg.mozilla.org/releases/comm-esr24/rev/86f515ee29a0
status-thunderbird_esr24:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•