Last Comment Bug 532693 - edit message as new for messages with attachments corrupts heap
: edit message as new for messages with attachments corrupts heap
Status: VERIFIED FIXED
: crash, fixed-seamonkey2.0.3
Product: MailNews Core
Classification: Components
Component: MIME (show other bugs)
: Trunk
: x86 Windows 7
: -- critical (vote)
: Thunderbird 3.1a1
Assigned To: David :Bienvenu
:
Mentors:
: 536498 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-03 09:35 PST by David :Bienvenu
Modified: 2015-05-14 02:36 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
.1+
.1-fixed


Attachments
proposed fix (1.38 KB, patch)
2009-12-03 11:40 PST, David :Bienvenu
rkent: review+
standard8: superreview+
standard8: approval‑thunderbird3.0.1+
Details | Diff | Review

Description David :Bienvenu 2009-12-03 09:35:17 PST
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 2009-12-03 11:40:46 PST
Created attachment 415914 [details] [diff] [review]
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) 2009-12-03 15:36:38 PST
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 2009-12-11 15:37:45 PST
this should block 3.01
Comment 4 Kent James (:rkent) 2009-12-14 14:31:54 PST
The changes in this patch are being implemented in trunk as part of bug 312025.
Comment 5 David :Bienvenu 2009-12-15 08:09:18 PST
fixed on trunk.
Comment 6 David :Bienvenu 2009-12-21 07:18:22 PST
fixed for 3.01
Comment 7 Serge Gautherie (:sgautherie) 2009-12-21 11:27:09 PST
(In reply to comment #6)
> fixed for 3.01

Shouldn't this have landed on 'default' hg branch (too)?
Comment 8 David :Bienvenu 2009-12-21 11:29:43 PST
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) 2009-12-21 11:31:44 PST
(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 2009-12-21 14:04:50 PST
(In reply to comment #9)
> Can you back them out from the relbranch and reland on default please?

done.
Comment 11 Micah Gersten 2009-12-27 16:17:03 PST
*** Bug 536498 has been marked as a duplicate of this bug. ***
Comment 12 Ludovic Hirlimann [:Usul] 2010-01-08 05:15:26 PST
V. Fixed based on the use of the email example pointed by rkent.

Note You need to log in before you can comment on or make changes to this bug.