Closed Bug 287847 Opened 15 years ago Closed 15 years ago
some callers of ns
Msg Local Mail Folder::Get Database WOReparse recreate the database and leak the new one
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).
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)
*** Bug 285990 has been marked as a duplicate of this bug. ***
Attachment #179405 - Flags: superreview?(mscott) → superreview+
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.