nsMsgCompose::_SendMsg leaks via cycle

VERIFIED FIXED in mozilla0.9

Status

P3
normal
VERIFIED FIXED
18 years ago
10 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

({memory-leak})

Trunk
mozilla0.9
x86
Linux
memory-leak

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

nsMsgCompose::_SendMsg leaks the nsMsgCompose and the nsMsgComposeSendListener
and everything they own by creating an owning reference cycle.
nsMsgCompose::_SendMsg creates an nsMsgComposeSendListener assigns it
into m_SendListener, addrefs it, and then calls
m_sendListener->SetComposeObj(this), which creates an owning pointer back in
the other direction.  Both pointers are released only by the destructors of
the relevant objects.  Thus this is just an intentional leak to avoid a
crash (which is what happens when I fix it with the patch I will attach.)

m_sendListener is not used *anywhere else* in the nsMsgCompose class, and
whenever it is used in _SendMsg the old value (if any) is overwritten.  So
if you're going to leak intentionally to avoid a crash, you could at least
change from a member m_SendListener to a local variable to leak 4 bytes
less! :-)
Created attachment 23706 [details] [diff] [review]
patch that should fix leak but causes crash
Keywords: mlk
FWIW, the crash stack is:

#4  <signal handler called>
#5  0x42f4f33a in ?? ()
#6  0x4225f358 in nsMsgComposeAndSend::SetListenerArray (this=0x42f4f3b8, 
    aListenerArray=0x42814810, aListeners=2)
    at /builds/seamonkey/mozilla/mailnews/compose/src/nsMsgSend.cpp:3152
#7  0x42260053 in nsMsgComposeAndSend::CreateAndSendMessage (this=0x42f4f3b8, 
    aEditor=0x42d6b1c0, aUserIdentity=0x42d68250, fields=0x42d970c0, 
    digest_p=0, dont_deliver_p=0, mode=0, msgToReplace=0x0, 
    attachment1 [details] [diff] [review]_type=0x422b7b51 "text/html", attachment1 [details] [diff] [review]_body=0x42d99800 "", 
    attachment1 [details] [diff] [review]_body_length=0, attachments=0x0, preloaded_attachments=0x0, 
    relatedPart=0x0, aListenerArray=0x42814810, aListeners=2)    at
/builds/seamonkey/mozilla/mailnews/compose/src/nsMsgSend.cpp:3482
#8  0x4227c11e in nsMsgCompose::_SendMsg (this=0x42ddf3f0, deliverMode=0, 
    identity=0x42d68250, entityConversionDone=0)
    at /builds/seamonkey/mozilla/mailnews/compose/src/nsMsgCompose.cpp:618
#9  0x4227c5e5 in nsMsgCompose::SendMsg (this=0x42ddf3f0, deliverMode=0, 
    identity=0x42d68250)
    at /builds/seamonkey/mozilla/mailnews/compose/src/nsMsgCompose.cpp:706
#10 0x401409e4 in XPTC_InvokeByIndex (that=0x42ddf3f0, methodIndex=7, 
    paramCount=2, params=0xbfffcc24)
    at
/builds/seamonkey/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:134
...
Assigning to myself to try to get this reviewed and checked in.
Assignee: ducarroz → dbaron
Priority: -- → P3
Target Milestone: --- → mozilla0.9
Status: NEW → ASSIGNED

Updated

18 years ago
Keywords: patch
Why do you need kungFuDeathGrip, why not just direclty declare sendListener as a
nsCOMPtr?
Also, for what I have seen, we don't need to hold a reference on the listener as
the object nsMsgSend will hold it via its listener array.
I think your original patch was craching because you were releasing the listener
to early, you need to do it just before we release the array. But I am pretty
sure the best solution is to remove the listener addref and release.
> Why do you need kungFuDeathGrip, why not just direclty declare sendListener as
> a nsCOMPtr?

Because nsCOMPtrs can only be used as interface pointers (although I've seen
cases where it works to use them as implementation pointers, I don't think
it's supposed to work).

> Also, for what I have seen, we don't need to hold a reference on the listener
> as the object nsMsgSend will hold it via its listener array.

Umm... you should never use an XPCOM object while it has a refcount of 0.  Is
that what you are suggesting?  (i.e., not addref until the call
|mMsgSend->CreateAndSendMessage|)

> I think your original patch was craching because you were releasing the
> listener to early, you need to do it just before we release the array. But I
> am pretty sure the best solution is to remove the listener addref and release.

Yes, I realize that.  I didn't notice the weird ownership model of the array at
first.

But you should never use XPCOM objects while they have a refcount of 0.
Suppose the call to |sendListener->SetComposeObj| called some other function
with |this| as an argument, and that other function |AddRef|ed and |Release|d
that argument (the object |sendListener| points to).  |sendListener| would
suddenly be a pointer to freed memory (if you didn't crash already while
returning).
ok, good point. R=ducarroz
as always, great work dbaron.

two minor points.

1) can you return NS_ERROR_OUT_OF_MEMORY, instead of NS_ERROR_FAILURE when the
new() fails?

2) as far as testing, did you check the following permutations:

{posting a news article, emailing a message, both in the same message} x {copy
gets put in sent folder, copy does not get put in the sent folder}

after you do that, sr=sspitzer
1) Change made

2) Tests run.  The 4 messages that should have appeared in netscape.test, the 4
messages that should have reached me by email, and the 3 that should have ended
up in the sent folder.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
Fix checked in 2001-03-04 11:51 PST.  Other leaks filed as bug 70852.

Comment 13

18 years ago
David, I don't have the resources for verifying leaks.  Could you please verify 
this.  Thanks!
QA Contact: esther → stephend
sending leak/opt bugs over to my other bugzilla account
QA Contact: stephend → stephen.donner
QA Contact: stephen.donner → stephend
Marking verified after conversation with stephend, since I haven't seen this for
ages.
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.