66887 - nsMsgCompose::_SendMsg leaks via cycle

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch that should fix leak but causes crash

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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
...

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: a working fix

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Assigning to myself to try to get this reviewed and checked in.

Comment 5

[Jean-Francois Ducarroz]

Why do you need kungFuDeathGrip, why not just direclty declare sendListener as a
nsCOMPtr?

Comment 6

[Jean-Francois Ducarroz]

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.

Comment 7

[Jean-Francois Ducarroz]

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.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> 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).

Comment 9

[Jean-Francois Ducarroz]

ok, good point. R=ducarroz

Comment 10

[(not reading, please use [e-mail] instead)]

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

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in 2001-03-04 11:51 PST.  Other leaks filed as bug 70852.

Comment 13

[esther]

David, I don't have the resources for verifying leaks.  Could you please verify 
this.  Thanks!

Comment 14

[[e-mail] (gone - use [e-mail] instead)]

sending leak/opt bugs over to my other bugzilla account

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Marking verified after conversation with stephend, since I haven't seen this for
ages.