257220 - nsAddrDatabase.cpp looks leaky

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

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

I was LXR-ing for NextRow while investigating bug 257218, and I noticed that
nsAddrDatabase.cpp looks like it leaks the result of a bunch of its NextRow
calls as well.  I'll attach a patch that I think cleans this up, although I
don't know how to test this code, and I'm also not really sure since I have seen
other similar code that expects input parameters to be AddRef'd.

Also, I noticed that, in nsDBFolderInfo.cpp, only one of AddToNewMDB and
InitFromExistingDB can be called once over the lifetime of the object or it will
leak m_mdbRow.

Also, the patch to nsAddrDatabase that I'll attach adds an XXX comment regarding
unreachable code (due to NS_RELEASE assigning null).

Comment 1

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

Attachment: patch

Comment 2

[David :Bienvenu]

David, your patch looks good. Do you want me to run with it to test it and check
it in if its OK? 

The unreachable code bothers me - not so much that it's unreachable, since I
don't think you can't have duplicate rows in a table,  but I don't see how that
code removes a card from all lists. I'll have to test that out.

re nsDBFolderInfo, it's a singleton so I don't think that's a problem.

Comment 3

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

(In reply to comment #2)
> David, your patch looks good. Do you want me to run with it to test it and check
> it in if its OK? 

Sure, that would be great.

Comment 4

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

It looks like the patch to bug 203927 fixed most of the leaks; the patch to
nsAddrDatabase::CheckAndUpdateRecordKey is still valid, though.

Comment 5

[Wayne Mery (:wsmwk)]

Mark, see comment 4

Comment 6

[Wayne Mery (:wsmwk)]

(In reply to comment #4)
> the patch to nsAddrDatabase::CheckAndUpdateRecordKey is still valid, though.

from attachment 157243 [details] [diff] [review] the 2 lines to replace are
      nsIMdbRow* findRow = nsnull;
and 
        merror = rowCursor->NextRow(GetEnv(), &findRow, &rowPos);

Comment 7

[Mark Banner (:standard8)]

Attachment: Fix the nsAddrDatabase::CheckAndUpdateRecordKey leak

I've taken a look at dbaron's patch, and I agree its just nsAddrDatabase::CheckAndUpdateRecordKey leak that is outstanding. I haven't been able to work out how to trigger that function to test (even with playing around with mab files), but I think this will be ok if we ever do run over it.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 279508 [details] [diff] [review]
Fix the nsAddrDatabase::CheckAndUpdateRecordKey leak

Opps, looks like I forgot to request reviews...

Comment 9

[David :Bienvenu]

Comment on attachment 279508 [details] [diff] [review]
Fix the nsAddrDatabase::CheckAndUpdateRecordKey leak

Comment 10

[Mark Banner (:standard8)]

Patch checked in. I believe this bug is fixed now :-)