387709 - Mailnews Datasources Leak

Status: NEW

(MailNews Core :: Backend, defect)

Description

[Scott MacGregor]

See Bug 383939 for an explanation of what's going on.

I've got some patches coming up for this.

Comment 1

[Scott MacGregor]

Attachment: [checked in]fix nsMsgRDFDatasource, nsMsgAccountDataSource and nsMsgFolderDatasource leaks

* convert the mObservers list from a nsISupportsArray to nsCOMArray so we can use the NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMARRAY macro.
* remove nsSoundDatasource which was not being built.
* make nsMsgRDFDatasource use the cycle collector. the account manager and folder data sources inherit from this base class.

To test this fix. I turned on the bloat log:

export XPCOM_MEM_LEAK_LOG=leaklog.txt

Started up Thunderbird and quit. Verified that the following objects were leaked:
* CompositeDataSourceImpl   
* nsMsgAccountManagerDataSource (1)
* nsMsgFolderDataSource  (2 of these: one for recent, one for all)
* nsMsgRDFDataSource  (3)

I then repeated this test with this patch and verified that these objects get cycle collected on shut down and are no longer listed in the leak log.

Comment 2

[David :Bienvenu]

Comment on attachment 271891 [details] [diff] [review]
[checked in]fix nsMsgRDFDatasource, nsMsgAccountDataSource and nsMsgFolderDatasource leaks

very nice - does this fix the big leak?

Comment 3

[Scott MacGregor]

sadly it doesn't fix the jscontext leak :(

Comment 4

[Worcester12345]

Needs mlk key word

Comment 5

[Mark Banner (:standard8)]

Scott: are you intending on fixing up the address book rdf datasource in this bug as well?

Comment 6

[Scott MacGregor]

yeah I actually wrote a patch for the address book data source but was having a hard time getting some of the thread proxy stuff working with it. I'll post what I had and we can go from there...

Comment 7

[Scott MacGregor]

Attachment: [checked in]patch for the address book data source

Here's the patch I put together for the address book data source. I hadn't gotten around to testing the scenario where the NotifyObservers is called from a thread other than the main thread...

Comment 8

[Scott MacGregor]

Comment on attachment 273722 [details] [diff] [review]
[checked in]patch for the address book data source

This fixes the leak for me Mark.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 273722 [details] [diff] [review]
[checked in]patch for the address book data source

Yep looks good to me, certainly didn't manage to break it in my testing ;-)

Though I can only give r+ not sr+

Comment 10

[Peter Van der Beken [:peterv]]

Comment on attachment 273722 [details] [diff] [review]
[checked in]patch for the address book data source

>Index: nsAbRDFDataSource.cpp
>===================================================================

>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsAbRDFDataSource)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMARRAY(mObservers)
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMARRAY(mProxyObservers)

This needs to use NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMARRAY, not _UNLINK_

Comment 11

[Mark Banner (:standard8)]

Attachment: [checked in]Fix unlink/traverse per comment 10

Fixes the incorrect macro in the address book data source (see comment 10).

Comment 12

[Scott MacGregor]

Comment on attachment 287313 [details] [diff] [review]
[checked in]Fix unlink/traverse per comment 10

thanks Mark.

Comment 13

[David Ascher (:davida)]

mark, you probably should take this on?

Comment 14

[David Ascher (:davida)]

Mark, status update?

Comment 15

[Mark Banner (:standard8)]

(In reply to comment #14)
> Mark, status update?

This seems quite complicated. I'm not sure Scott's original changes were 100% correct, plus we have lots of other potential leaks at the moment.

I just took a look at this again, but based on my 1 hour look, I doubt I'm going to get to this before b1.