347710 - nsDirPrefs leaks a DirPrefObserver on shutdown because we only release it on "profile-do-change"

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Adam Guthrie]

Found using trace refcount logging.

nsDirPrefs AddRef's prefObserver here and then Releases it in its DIR_Shutdown method. http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/addrbook/src/nsDirPrefs.cpp&rev=1.116&mark=249,271#247

This is all fine and good except DIR_Shutdown is only called from its observer (?) on "profile-do-change". http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/addrbook/src/nsDirectoryDataSource.cpp&rev=1.74&mark=114,106#102

I'm guessing this is left over from the old days when you could switch profiles in place or something, but I'm not sure. 

Maybe we need to kill the "profile-do-change" topic and just move the DIR_Shutdown call to the "xpcom-shutdown" topic observer?

Comment 1

[Adam Guthrie]

Attachment: patch?

Comment 2

[Mark Banner (:standard8)]

(In reply to comment #0)
> This is all fine and good except DIR_Shutdown is only called from its observer
> (?) on "profile-do-change".

> I'm guessing this is left over from the old days when you could switch profiles
> in place or something, but I'm not sure. 

You still can change profiles on SeaMonkey can't you? At least at the moment on trunk and on 1.8 branch.

Would NS_XPCOM_SHUTDOWN_OBSERVER_ID still be observed in the case of a profile change?

Comment 3

[[e-mail]]

Maybe it would be better to use profile-before-change?

Comment 4

[Adam Guthrie]

Can someone with a SeaMonkey build answer Mark's question in comment 2?

Comment 5

[[e-mail]]

(In reply to comment #2)
>You still can change profiles on SeaMonkey can't you? At least at the moment on
>trunk and on 1.8 branch.
You'll always be able to change them on branch, but on Trunk we'll be stuffed by toolkit as usual.

>Would NS_XPCOM_SHUTDOWN_OBSERVER_ID still be observed in the case of a profile
>change?
No.

Comment 6

[Adam Guthrie]

So couldn't we just call both DIR_Shutdown and Cleanup if we're shutting down, and only call DIR_Shutdown if we're switching profiles?

Comment 7

[Mark Banner (:standard8)]

(In reply to comment #6)
> So couldn't we just call both DIR_Shutdown and Cleanup if we're shutting down,
> and only call DIR_Shutdown if we're switching profiles?

That would sound reasonable.

Comment 8

[Adam Guthrie]

Attachment: Call DIR_ShutDown when we're shutting down, too

We need to call this when we shutdown, too, not just when we're doing a profile switch (it even says so in the comment after the function declaration!).

Comment 9

[David :Bienvenu]

Comment on attachment 236571 [details] [diff] [review]
Call DIR_ShutDown when we're shutting down, too

thx, Adam.

Comment 10

[Mark Banner (:standard8)]

I've just checked this in:

Checking in mailnews/addrbook/src/nsDirectoryDataSource.cpp;
/cvsroot/mozilla/mailnews/addrbook/src/nsDirectoryDataSource.cpp,v  <--  nsDirectoryDataSource.cpp
new revision: 1.76; previous revision: 1.75
done

Comment 11

[Adam Guthrie]

Comment on attachment 236571 [details] [diff] [review]
Call DIR_ShutDown when we're shutting down, too

Requesting branch approval for this leak fix when opening the address book window.

Comment 12

[Scott MacGregor]

Comment on attachment 236571 [details] [diff] [review]
Call DIR_ShutDown when we're shutting down, too

If Mark doesn't have any objections, this looks good for the 1.8.1 branch for seamonkey and thunderbird.

Comment 13

[Mark Banner (:standard8)]

(In reply to comment #12)
> (From update of attachment 236571 [details] [diff] [review] [edit])
> If Mark doesn't have any objections, this looks good for the 1.8.1 branch for
> seamonkey and thunderbird.
> 
That's fine by me. I'll check it in tomorrow unless someone does it before me or the tree closes.

Comment 14

[Mark Banner (:standard8)]

Checked in on branch as well.
Checking in mailnews/addrbook/src/nsDirectoryDataSource.cpp;
/cvsroot/mozilla/mailnews/addrbook/src/nsDirectoryDataSource.cpp,v  <--  nsDirectoryDataSource.cpp
new revision: 1.67.2.2; previous revision: 1.67.2.1

I assume this is fixed now - so marking fixed. Apologies in advance if I am wrong.

Thanks for the patch Adam.