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

Status: RESOLVED FIXED

(MailNews Core :: Attachments, defect)

Description

[Kent James (:rkent)]

If you call detachAttachmentsWOPrompts with empty arrays, it crashes.

Comment 1

[Kent James (:rkent)]

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

[timeless]

i mostly need a stack signature in the summary in case someone does trigger it.

Comment 3

[Wayne Mery (:wsmwk)]

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

Comment 4

[Kent James (:rkent)]

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

[Wayne Mery (:wsmwk)]

yup - yours are the only 2 crashes in 8 months

Comment 6

[David :Bienvenu]

Attachment: correctness fix

since this is an idl method, it should be checking its args...

Comment 7

[Kent James (:rkent)]

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.

Comment 8

[David :Bienvenu]

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

Comment 9

[David :Bienvenu]

fixed on trunk - http://hg.mozilla.org/comm-central/rev/1bd4ab0795c1