some callers of nsMsgLocalMailFolder::GetDatabaseWOReparse recreate the database and leak the new one

RESOLVED FIXED

Status

MailNews Core
Backend
RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: dbaron, Assigned: Bienvenu)

Tracking

({memory-leak})

Trunk
memory-leak

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

I ran into this leak when opening a local folder, selecting a message in that
folder, and then deleting the message with the button on the toolbar (in
Thunderbird).

The problem is that many callers of nsMsgLocalMailFolder::GetDatabaseWOReparse
leak the database, because both caller and callee are filling in the same owning
pointer at the same time.  (It happens to be an nsCOMPtr, but the same problem
happens without.)

The particular caller with which I saw the problem was
nsMsgLocalMailFolder::DeleteMessages .  The chain of events that causes the leak is:
 1. nsMsgLocalMailFolder::DeleteMessages calls getter_AddRefs(mDatabase), which
    is the equivalent of NS_IF_RELEASE(mDatabase)
 2. nsMsgLocalMailFolder::GetDatabaseWOReparse does the same
 3. nsMsgLocalMailFolder::GetDatabaseWOReparse calls OpenFolderDB, which fills
    in the mDatabase pointer passed to it.  mDatabase now owns a reference to
    the database.
 4. nsMsgLocalMailFolder::GetDatabaseWOReparse AddRefs mDatabase for its caller
    and fills in its |*aDatabase| pointer, which assigns to mDatabase and gives
    mDatabase a *second* owning reference to the database without releasing the
    first one.

This is a general danger of filling in variables that other functions have
access to using out parameters, since you have to release the old value a
significant period of time before filling in the new one.  Strategies for fixing
include:
 * returning the pointer using already_AddRefed<T>
 * filling in a local variable instead of a member variable with the out
   parameter instead (probably the easiest solution here)
 * making the inner function not have an out parameter at all

Problematic callers are:
  nsMsgLocalMailFolder::GetDatabase
  nsMsgLocalMailFolder::GetMessages
  nsMsgLocalMailFolder::DeleteMessages
It's also worth noting that when these callers call GetDatabaseWOReparse, the
cached |mDatabase| pointer is always null (since the getter_AddRefs() nulls it
out), so a new instance is always created (unless m_parsingFolder).
I'm amused to note that I documented this as a common leak pattern but called it
"obscure" in http://www.mozilla.org/performance/leak-brownbag.html#common
(bullet 2.3).
(Assignee)

Comment 2

13 years ago
Created attachment 179405 [details] [diff] [review]
proposed fix

use a local var for msg db - also change a PR_ASSERT to an NS_ASSERTION. Thx a
lot for finding this, David.
Assignee: sspitzer → bienvenu
Status: NEW → ASSIGNED
Attachment #179405 - Flags: superreview?(mscott)
(Assignee)

Comment 3

13 years ago
*** Bug 285990 has been marked as a duplicate of this bug. ***

Updated

13 years ago
Attachment #179405 - Flags: superreview?(mscott) → superreview+
(Assignee)

Updated

13 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.