Closed Bug 1444379 Opened 6 years ago Closed 6 years ago

Crash in nsMsgComposeAndSend::NotifyListenerOnStopCopy

Categories

(MailNews Core :: Composition, defect)

Product:
MailNews Core
Component:
Composition
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(thunderbird_esr6062+ fixed, thunderbird63 fixed, thunderbird64 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 64.0
Tracking Flags:
Tracking Status
thunderbird_esr60 62+ fixed
thunderbird63 --- fixed
thunderbird64 --- fixed

People

(Reporter: wsmwk, Assigned: mkmelin)

References

(
URL
)

Details

(Keywords: crash, topcrash-thunderbird, Whiteboard: [regression:TB5?])

Crash Data

Attachments

(2 files, 1 obsolete file)

bug1444379_getprompter_crash.patch
7.10 KB, patch
jorgk-bmo
: review+
jorgk-bmo
: approval-comm-beta+
jorgk-bmo
: approval-comm-esr60+
Details | Diff | Splinter Review
1444379-follow-up.patch
2.42 KB, patch
aceman
: review+
Details | Diff | Splinter Review 
#14 crash for 59.0b2.  afaict most users crash only once - one and done.
It may be a regression (new crash), or just a morph of an older crash signature

Oldest crashes I find are from 56 beta
bp-b7ba5a87-43bf-4c7c-822b-ec7f70180102 buildid 20171006073359
bp-1e348000-aaf8-449d-b617-e20040180131 buildid 20170819123318

There are some crashes on 52.x but I am unsure they are the same issue. 


This bug was filed from the Socorro interface and is
report bp-56d0f1ff-5f25-4a93-a37e-6043f0180125. 58.0b3 (halnew)
=============================================================

Top 10 frames of crashing thread:

0 xul.dll nsMsgComposeAndSend::NotifyListenerOnStopCopy C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/compose/src/nsMsgSend.cpp:3912
1 xul.dll nsMsgComposeAndSend::DoFcc C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/compose/src/nsMsgSend.cpp:3701
2 xul.dll nsMsgComposeAndSend::DoDeliveryExitProcessing C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/compose/src/nsMsgSend.cpp:3597
3 xul.dll nsMsgComposeAndSend::DeliverAsMailExit C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/compose/src/nsMsgSend.cpp:3603
4 xul.dll nsMsgComposeAndSend::SendDeliveryCallback C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/compose/src/nsMsgSend.cpp:3194
5 xul.dll MsgDeliveryListener::OnStopRunningUrl C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/compose/src/nsMsgSend.cpp:253
6 xul.dll nsMsgMailNewsUrl::SetUrlState C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/base/util/nsMsgMailNewsUrl.cpp:168
7 xul.dll nsSmtpProtocol::ProcessProtocolState C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/compose/src/nsSmtpProtocol.cpp:2102
8 xul.dll nsMsgProtocol::OnDataAvailable C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/base/util/nsMsgProtocol.cpp:303
9 xul.dll nsInputStreamPump::OnStateTransfer netwerk/base/nsInputStreamPump.cpp:591

=============================================================

bp-6597f25c-b2cb-414c-9894-52a120180301

Frame	Module	Signature	Source
0	xul.dll	nsMsgComposeAndSend::NotifyListenerOnStopCopy(nsresult)	C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/compose/src/nsMsgSend.cpp:3912
1	xul.dll	CopyListener::OnStopCopy(nsresult)	C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/compose/src/nsMsgCopy.cpp:116
2	xul.dll	nsMsgCopyService::ClearRequest(nsCopyRequest*, nsresult)	C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/base/src/nsMsgCopyService.cpp:209
3	xul.dll	nsMsgCopyService::NotifyCompletion(nsISupports*, nsIMsgFolder*, nsresult)	C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/base/src/nsMsgCopyService.cpp:694
4	xul.dll	nsImapMailFolder::OnCopyCompleted(nsISupports*, nsresult)	C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsImapMailFolder.cpp:8468
5	xul.dll	nsImapMailFolder::OnStopRunningUrl(nsIURI*, nsresult)	C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsImapMailFolder.cpp:5461
Actually, bp-0ab6de87-e81b-4ce3-a0fd-821c30180309 and bp-53c01e25-1a3a-47a7-9931-a1fd80180308 for TB 59b2 are more useful since they show the crash site that correlates to the source: nsMsgSend.cpp:3919 

      rv = MimeDoFCC(mTempFile,
                     nsIMsgSend::nsMsgDeliverNow,
                     mCompFields->GetBcc(),
                     nullptr,
                     mCompFields->GetNewspostUrl());

I can't see why this would crash since there is code above that dereferenced 'mCompFields'.

Gene, you've worked in this code, any idea?
Flags: needinfo?(gds)
One user from fall 2017 claimed TB put two copies of sent messages in Sent folder. (those crash reports have aged off the system)

Some crashes involve attachments. I haven't been able to determine if they all do.  Involving attachments: userA bp-b4425cb1-8c70-4930-bdc4-c95a40180416 bp-c29e050e-cf67-4345-a3a9-f34f00180414 @ shutdownhang | nsAutoWindowStateHelper::DispatchEventToChrome

userB bp-fa9c247b-a072-43e8-b93f-5a5810180401
OS: Windows 10 → All
(In reply to Jorg K (GMT+1) from comment #1)
> Actually, bp-0ab6de87-e81b-4ce3-a0fd-821c30180309 and
> bp-53c01e25-1a3a-47a7-9931-a1fd80180308 for TB 59b2 are more useful since
> they show the crash site that correlates to the source: nsMsgSend.cpp:3919 
> 
>       rv = MimeDoFCC(mTempFile,
>                      nsIMsgSend::nsMsgDeliverNow,
>                      mCompFields->GetBcc(),
>                      nullptr,
>                      mCompFields->GetNewspostUrl());
> 
> I can't see why this would crash since there is code above that dereferenced
> 'mCompFields'.
> 
> Gene, you've worked in this code, any idea?

I just tried this with 60b5 and I was prompted to save draft to local folders with the network down. Worked OK with no crash.
Flags: needinfo?(gds)
(In reply to Wayne Mery (:wsmwk) from comment #2)
> One user from fall 2017 claimed TB put two copies of sent messages in Sent
> folder. (those crash reports have aged off the system)
> 
> Some crashes involve attachments. I haven't been able to determine if they
> all do.  Involving attachments: userA
> bp-b4425cb1-8c70-4930-bdc4-c95a40180416
> bp-c29e050e-cf67-4345-a3a9-f34f00180414 @ shutdownhang |
> nsAutoWindowStateHelper::DispatchEventToChrome
> 
> userB bp-fa9c247b-a072-43e8-b93f-5a5810180401

The users mention "can't send radiographs". I assume that is an x-ray or something similar, maybe tiff? Anyhow, I just tried to send some large attachments with 60b5 and they worked fine. Also, only one copy in the Sent folder.

Wayne, you mentioned in email to me something about "test cases" and if I think they are necessary. Not sure what you mean by "test cases". Are you referring to unit tests?
I mean an example message that fails, or set of clear steps.
User in https://support.mozilla.org/en-US/questions/1217449 also crashes Bug 537017 - crash [@ nsMsgFilterAfterTheFact::ApplyFilter

No clear steps yet, but perhaps cross referencing to Bug 537017 will help.
URL: https://support.mozilla.org/en-US/que...
See Also: → 537017
Looks like it can't get a prompt. Especially for the shutdown case (which is in at least some of the reports, didn't look through that many) that's not super unexpected.
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #9012847 - Flags: review?(jorgk)
Comment on attachment 9012847 [details] [diff] [review]
bug1444379_getprompter_crash.patch

You can remove all lines that contain 'promptObject' instead of adding error checking to dead code.
Attachment #9012847 - Flags: review?(jorgk) → review-
Indeed!
Attachment #9012847 - Attachment is obsolete: true
Attachment #9012852 - Flags: review?(jorgk)
Comment on attachment 9012852 [details] [diff] [review]
bug1444379_getprompter_crash.patch

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

Indeed ;-)

I'll add a nice commit message like:
Added error checking to various call sites of GetDefaultPrompt() in nsMsgSend.cpp to avoid crashes.
Attachment #9012852 - Flags: review?(jorgk) → review+
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/42792e593b1a
add error checking to call sites of GetDefaultPrompt() to avoid crash in nsMsgComposeAndSend::NotifyListenerOnStopCopy() and elsewhere. r=jorgk
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
Comment on attachment 9012852 [details] [diff] [review]
bug1444379_getprompter_crash.patch

The fix removes dead code and prevents imminent crash without changing behaviour.
Attachment #9012852 - Flags: approval-comm-esr60+
Attachment #9012852 - Flags: approval-comm-beta+
I came to look at this again and it doesn't make sense to retrieve the prompt if we don't even use it. Now moving the retrieval just before it's used.
Attachment #9013117 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9013117 [details] [diff] [review]
1444379-follow-up.patch

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

OK, looks reasonable.
Attachment #9013117 - Flags: review?(mkmelin+mozilla) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d55e51bddfae
Follow-up: move prompt retrieval to where prompt is used. r=aceman DONTBUILD
Any likely suspect to have caused this "regression"?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: