522353 - nsCategoryCache is still observing category changes after xpcom-shutdown

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Marco Bonardo [:mak]]

In bug 478718 i'm moving most of Places shutdown to xpcom-shutdown.
Due to that i've noticed we started leaking nsPlacesDBFlush (that is a category observer). Removing the entry with DeleteCategoryEntry or deleteCategory solves the leak.

Following the code i've found that nsCategoryCache on xpcom-shutdown clears its cache of category observers (http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsCategoryCache.cpp#130) but it continues to observe category for changes till the destructor is called (http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsCategoryCache.cpp#117)

Looks like we could cause a category notification during xpcom-shutdown (we notify the observer for the first time?) and since we could have registered as xpcom-shutdown after categoryCache, it will observe the change, and add back a reference to us in its local cache. This reference will never be cleaned up and will cause leaks.

I think we should remove the possibility for this to happen in nsCategoryCache, and also investigate why Places could end up notifying so late.

I think this situation is unlikely in real apps, but i see it quite frequently in xpcshell tests. More information coming.

The patch removes observers after the cache has been cleared. but actually this means if a notification happens so late, it won't be served.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch

Comment 2

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Comment 3

[Marco Bonardo [:mak]]

Attachment: patch v1.2

sorry, i attached the wrong patch.

Comment 4

[Marco Bonardo [:mak]]

Attachment: stack when Observe is called for addEntry after xpcom-shutdown

Comment 5

[Marco Bonardo [:mak]]

where:
aTopic	0x01686aa8 "xpcom-category-entry-added"	const char *
aData	0x01686af0 "bookmark-observers"	const wchar_t *

so, we notify a bookmarks change at xpcom-shutdown
this is reproduceable for example removing deleteCategoryEntry calls from nsPlacesDBFlush.js and running toolkit/components/places/tests/bookmarks/test_360134.js

Comment 6

[Marco Bonardo [:mak]]

i can confirm categoryCache xpcom-shutdown handler is called before history service's one (And this will be true for most of other services).
But i don't see any bookmarks notification after it, and bookmarks service is the only one that notifies to bookmark-observers category.

i tried to trap bookmarks notifications after xpcom-shutdown, but i did not trap anything and it did not even leak during test interactive :\

Comment 7

[Marco Bonardo [:mak]]

actually nsCategoryManager::NotifyObservers uses NS_PROXY_TO_MAIN_THREAD, this is a bit out of my knowledge, but if that could finish up enqueuing the notification, this means run_test() will cause an entryAdded message to be enqueued to main thread, and it will be served to the categorycache when the test is closing, maybe after xpcom-shutdown, and that would explain why i see the bookmarks notifications fired sync, but the entryadded notification fired only on test shutdown.

Comment 8

[Marco Bonardo [:mak]]

and in test interactive mode i don't see the problem because the mainthread is interrupted and the notification is served before i call Quit() (that will call xpcom-shutdown closing xpcshell)

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Ah OK. So the category manager does async notifications even if you're already on the main thread. Makes sense, that explains the problem.

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 406328 [details] [diff] [review]
patch v1.2

+  if (obsSvc) {

why didn't you keep this as an early return?


(you'll still need review from an xpcom peer)

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

(xpcom-shutdown is a sync notification, so it will definitely happen before the
async one from the category manager)

Comment 12

[Marco Bonardo [:mak]]

(In reply to comment #10)
> (From update of attachment 406328 [details] [diff] [review])
> +  if (obsSvc) {
> 
> why didn't you keep this as an early return?

mostly code style i'm used in other components, so if you need to add further code to that method that does not touch observer service you don't have to touch current code again due to the early return, and is usually less error-prone (in long methods you could forget about an early return).
I know in this case is not important and does not change a thing, if you want i can go back to the early method style. just let me know.

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 406328 [details] [diff] [review]
patch v1.2

if i read module page correctly peers should be bsmedberg, shaver and dougt

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

well, I normally prefer less indentation, and this method is unlikely to get new code added that doesn't require the observer service. but, I don't really care much, feel free to leave it as it is (unless dougt wants it changed :) )

Comment 15

[Marco Bonardo [:mak]]

Doug, any news on this?

Comment 16

[Benjamin Smedberg]

Comment on attachment 406328 [details] [diff] [review]
patch v1.2

moa=me

Comment 17

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/8fb97bb36e55