detachAttachmentsWOPrompts crashes with zero counts [@ nsMessenger::DetachAttachmentsWOPrompts(nsIFile*, unsigned int, char const**, char const**, char const**, char const**, nsIUrlListener*)]

RESOLVED FIXED in Thunderbird 3.3a2

Status

MailNews Core
Attachments
--
critical
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: rkent, Assigned: Bienvenu)

Tracking

({crash, stackwanted})

Trunk
Thunderbird 3.3a2
x86
Windows XP
crash, stackwanted

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
If you call detachAttachmentsWOPrompts with empty arrays, it crashes.

Updated

8 years ago
Severity: normal → critical
Keywords: crash, stackwanted
(Reporter)

Comment 1

8 years ago
Re "stackwanted" - this is a new function (that I added), and has no current use in the core code. So to make it crash, you need to call it from an extension. I did that in my FiltaQuilla extension, and discovered this issue. Because I know the issue, I can prevent it in my extension. But it should be fixed in core, and the fix is trivial.

Comment 2

8 years ago
i mostly need a stack signature in the summary in case someone does trigger it.
Summary: detachAttachmentsWOPrompts crashes with zero counts → detachAttachmentsWOPrompts crashes with zero counts [@ nsMessenger::DetachAttachmentsWOPrompts]

Comment 3

7 years ago
Kent, could you generate a crash report fur us? (or the patch)
(not seen in crash stats of last few weeks)
(Reporter)

Comment 4

7 years ago
http://crash-stats.mozilla.com/report/index/d13babdf-a757-41a4-9e41-9d19b2110111

I generated this by removing the zero-length attachment list check from FiltaQuilla. This fix is just to return from the C++ method immediately if the attachment count is zero.

Comment 5

7 years ago
yup - yours are the only 2 crashes in 8 months
Summary: detachAttachmentsWOPrompts crashes with zero counts [@ nsMessenger::DetachAttachmentsWOPrompts] → detachAttachmentsWOPrompts crashes with zero counts [@ nsMessenger::DetachAttachmentsWOPrompts(nsIFile*, unsigned int, char const**, char const**, char const**, char const**, nsIUrlListener*)]
(Assignee)

Comment 6

7 years ago
Created attachment 503155 [details] [diff] [review]
correctness fix

since this is an idl method, it should be checking its args...
Assignee: kent → bienvenu
Attachment #503155 - Flags: review?(kent)
(Reporter)

Comment 7

7 years ago
The only thing that I might take issue with is the error return if the count is zero. That is, if I have a list of zero attachments and ask to detach them, then doing nothing is the proper response, so why do I need an error? That just forces me to check in js whether the count is zero before using this routine, or surround it with a try block.

But that is a fairly minor point in a fairly minor patch, so rather than churn this needlessly I'll just approve it, and let you decide whether to return NS_OK or not with a count of zero.

The other issue is that you seem to be missing the check for null aDisplayNameArray. r=me with that fixed.
(Reporter)

Updated

7 years ago
Attachment #503155 - Flags: review?(kent) → review+
(Assignee)

Comment 8

7 years ago
(In reply to comment #7)
> The only thing that I might take issue with is the error return if the count is
> zero. That is, if I have a list of zero attachments and ask to detach them,
> then doing nothing is the proper response, so why do I need an error? That just
> forces me to check in js whether the count is zero before using this routine,
> or surround it with a try block.

For your use case, ignoring that error would be more convenient for you, but for other use cases, it would probably be a sign of a coding error, which is why I made it an error. But I don't care much, and I do hate js exceptions :-)
(Assignee)

Comment 9

7 years ago
fixed on trunk - http://hg.mozilla.org/comm-central/rev/1bd4ab0795c1
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a2
Crash Signature: [@ nsMessenger::DetachAttachmentsWOPrompts(nsIFile*, unsigned int, char const**, char const**, char const**, char const**, nsIUrlListener*)]
You need to log in before you can comment on or make changes to this bug.