Closed Bug 522353 Opened 10 years ago Closed 10 years ago

nsCategoryCache is still observing category changes after xpcom-shutdown

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: mak, Assigned: mak)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
Blocks: 478718
Attached patch patch v1.1 (obsolete) — Splinter Review
Attachment #406306 - Attachment is obsolete: true
Attached patch patch v1.2Splinter Review
sorry, i attached the wrong patch.
Attachment #406327 - Attachment is obsolete: true
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
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 :\
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.
Attachment #406328 - Flags: review?(cbiesinger)
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)
Assignee: nobody → mak77
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 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)
Attachment #406328 - Flags: review?(cbiesinger) → review+
(xpcom-shutdown is a sync notification, so it will definitely happen before the
async one from the category manager)
(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 on attachment 406328 [details] [diff] [review]
patch v1.2

if i read module page correctly peers should be bsmedberg, shaver and dougt
Attachment #406328 - Flags: review?(doug.turner)
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 :) )
Doug, any news on this?
Attachment #406328 - Flags: review+
Comment on attachment 406328 [details] [diff] [review]
patch v1.2

moa=me
http://hg.mozilla.org/mozilla-central/rev/8fb97bb36e55
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Keywords: checkin-needed
Blocks: 520165
You need to log in before you can comment on or make changes to this bug.