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

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

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

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

Comment 1

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

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

Comment 2

[David :Bienvenu]

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

Comment 3

[David :Bienvenu]

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