532693 - edit message as new for messages with attachments corrupts heap

Status: VERIFIED FIXED

(MailNews Core :: MIME, defect)

Description

[David :Bienvenu]

spun off from bug 312025. This may be debug only, since it seems to be related to the url leak code added to nsStandardUrl.cpp, but I haven't quite pinpointed the actual blame, so I can't be sure it's debug only. And I can't know if this should block bug 312025 until I figure that out.

One difference between the crash and non crash case is that when we edit a message as new, we create a url to save the attachment.

This bug also affects forward inline, which goes through the same mimedrft.cpp code. And in theory, editing a saved draft would have the same issue.

Comment 1

[David :Bienvenu]

Attachment: proposed fix

The bug is that mime isn't using xpcom reference counting semantics, so it's deleting an object that it should not.  I also cleaned up a separate ref counting issue where mime was addreffing a pointer that had already been addreffed (though only getting rid of the delete fixes the crash).

For some reason, the url leak stuff added to nsStandardUrl.cpp exposed this issue, perhaps by changing the size of the url enough so that we weren't saved by heap buffer size rounding...

I'll try to come up with a mozmill test that at least exercises this code, though it won't guarantee anything...

Comment 2

[Kent James (:rkent)]

Comment on attachment 415914 [details] [diff] [review]
proposed fix

Running with this patch and the patch from bug 312025, I am no longer crashing, while without this patch I crash within three forwards using the test message in that bug.

I also checked the code and its uses, and this seems like the right thing to do.

Comment 3

[David :Bienvenu]

this should block 3.01

Comment 4

[Kent James (:rkent)]

The changes in this patch are being implemented in trunk as part of bug 312025.

Comment 5

[David :Bienvenu]

fixed on trunk.

Comment 6

[David :Bienvenu]

fixed for 3.01

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to comment #6)
> fixed for 3.01

Shouldn't this have landed on 'default' hg branch (too)?

Comment 8

[David :Bienvenu]

Didn't this land on the trunk a week ago, as I said in #c5? Looks to me like it did.

Comment 9

[Mark Banner (:standard8)]

(In reply to comment #8)
> Didn't this land on the trunk a week ago, as I said in #c5? Looks to me like it
> did.

Take a look at: http://hg.mozilla.org/releases/comm-1.9.1/rev/eb1a0eb3b4ef
(and http://hg.mozilla.org/releases/comm-1.9.1/rev/05a86172f79f)

It landed on COMM1915_20091112_RELBRANCH within comm-central rather than "default".

Can you back them out from the relbranch and reland on default please?

Comment 10

[David :Bienvenu]

(In reply to comment #9)
> Can you back them out from the relbranch and reland on default please?

done.

Comment 12

[Ludovic Hirlimann [:Usul]]

V. Fixed based on the use of the email example pointed by rkent.