Observer service leaked on shutdown > ... XPCOM shutdown releases the observer service but doesn't forcefully drop > observer references (it probably should, and we should probably also > warn-on-leak. > --BDS

You weren't testing with a places build when you filed this, were you?

This does the forceful removal during shutdown. I couldn't add a "you're leaking the observerservice" warning (yet) because it's held through xpconnect GC (which I'll hopefully be able to fix in the shutdown ordering bugs).

> You weren't testing with a places build when you filed this, were you? Nope. Sorry for the late reply.

Comment on attachment 213077 [details] [diff] [review] Forceful removal, rev. 1 >Index: xpcom/ds/nsObserverService.cpp >+void >+nsObserverService::Shutdown() >+{ >+ mObserverTopicTable.Clear(); >+ if (mLock) { >+ PR_DestroyLock(mLock); >+ mLock = nsnull; >+ } > } Deleting the lock here assumes that there are no other threads referencing nsObserverService. How do you ensure that? Why not just move the lock destruction into ~nsObserverService? Then at least you would know that there are no other references to |this|. >+#define NS_ENSURE_INITIALIZED \ >+ if (!mLock) { \ >+ NS_ERROR("Using observer service after XPCOM shutdown!"); \ >+ return NS_ERROR_NOT_INITIALIZED; \ >+ } See, you can't promise this. If the thread passed this check and then Shutdown ran, you'd be in trouble.

Because we'd end up creating more observer objects and I want it to fail. I have a solution coming up.

Comment on attachment 213087 [details] [diff] [review] Forceful removal, rev. 1.1 >Index: xpcom/ds/nsObserverService.cpp >+ if (mLock) { >+ PRLock *tlock = mLock; >+ PR_Lock(tlock); >+ >+ mLock = nsnull; >+ >+ if (mObserverTopicTable.IsInitialized()) >+ mObserverTopicTable.Clear(); >+ >+ PR_Unlock(tlock); >+ PR_DestroyLock(tlock); >+ } This doesn't help at all. If you context switch after PR_Unlock and another thread grabs the lock, you might crash in PR_DestroyLock. Lock destruction is best handled via reference counting. Perhaps you want to keep a boolean parameter to flag shutdown. I think trying to destroy the lock at shutdown is a non-starter unless you have some way of waiting for all background threads to release their reference to this.

Comment on attachment 213088 [details] [diff] [review] Forceful removal, rev. 1.2 >Index: xpcom/ds/nsObserverService.cpp >+void >+nsObserverService::Shutdown() >+{ >+ nsAutoLock al(mLock); >+ >+ mShuttingDown = PR_TRUE; >+ >+ if (mObserverTopicTable.IsInitialized()) >+ mObserverTopicTable.Clear(); > } Hrm... clearing the observer list may result in nsIObserver objects being destroyed. Those destructors might try to poke the observer service, and then you would dead lock. Never call into foreign code while holding a lock! The only exception to that rule is AddRef. >+nsObserverList* >+nsObserverService::GetObserverList(const char* aTopic) > { >+ nsAutoLock al(mLock); > > nsObserverList *topicObservers; >+ mObserverTopicTable.Get(aTopic, &topicObservers); > if (topicObservers) >+ return topicObservers; > > nsresult rv = NS_OK; > topicObservers = new nsObserverList(rv); > if (!topicObservers) >+ return nsnull; > >+ if (NS_FAILED(rv) || !mObserverTopicTable.Put(aTopic, topicObservers)) { > delete topicObservers; >+ return nsnull; > } >+ >+ return topicObservers; > } This method seems to enter a lock to get a pointer to an internal data structure. Then it releases the lock. That would seem to make the nsObserverList pointer potentially invalid. You need to use reference counting on nsObserverList if you want this to be threadsafe. BTW, I happen to think that the observer service should be limited to the main application thread. What does it mean to register observers from a background thread anyways? They will just run on the main thread. If we have any consumers of nsIObserverService from a background thread, then they should be forced to construct a XPCOM proxy to use it. I don't think we have any consumers that use the observer service from a background thread. My recommendation is to not bother supporting multiple threads here. Just add NS_ENSURE_STATE(nsIThread::IsMainThread()) checks everywhere :)

As discussed. I'll want this on the branch, but we should probably let it bake a while on trunk first.

Comment on attachment 213812 [details] [diff] [review] Shutdown timers much earlier, rev. 1 [checked in] >Index: xpcom/build/nsXPComInit.cpp ... > NotifyObservers(mgr, NS_XPCOM_SHUTDOWN_OBSERVER_ID, > nsnull); > } > } > >+ // Shutdown the timer thread and all timers that might still be alive >+ nsTimerImpl::Shutdown(); >+ > if (currentQ) > currentQ->ProcessPendingEvents(); I think it might be a good idea to call ProcessPendingEvents before shutting down the timer subsystem since there may be some pending timer events that should run before we attempt to kill the timer subsystem. r=darin with that addition

Comment on attachment 213812 [details] [diff] [review] Shutdown timers much earlier, rev. 1 [checked in] This was checked in on trunk with the additional ProcessPendingEvents

This single-threads the observer service, and with that removes a lot of the complexity with nsObserverList locking. It also implements bug 321040 (remove stale weak references).

Comment on attachment 213893 [details] [diff] [review] Forceful removal, rev. 2 >Index: xpcom/build/nsXPCOMCID.h >+#define NS_OBSERVERSERVICE_CONTRACTID "@mozilla.org/observer-service;1" Thanks you!! :) >Index: xpcom/build/nsXPComInit.cpp >+ nsRefPtr<nsObserverService> observerService; >+ CallGetService("@mozilla.org/observer-service;1", >+ (nsObserverService**) getter_AddRefs(observerService)); It seems like we should be able to just read a global variable to get the observer service, but okay. >Index: xpcom/ds/nsObserverList.cpp >+nsObserverList::EnumerateObservers(EnumObserversFunc aFunc, void *aClosure) You should document in the header that aFunc is not permitted to modify the observer list. Perhaps nsObserverService.h should be removed from the EXPORTS list in xpcom/ds/Makefile.in?

Fixed on trunk with nits.

backed out to fix bug 329505

The "shutdown timers much earlier" patch probably caused bug 331012.

This changes the NotifyObservers code to fill a temporary array the same way the EnumerateObservers does.

Please ignore the extraneous nsArrayEnumerator.h #include, it's an accident.

Comment on attachment 215682 [details] [diff] [review] Forceful removal, rev. 3 r=darin

Fixed on trunk.

The assertions added here are making balsa orange since session saver (bug 328159) landed; I'm sort of tracking that on bug 315421, since that was available. And I'm not quite sure how services are supposed to use the observer service now. Should they be trying to remove themselves? When?

(Please answer that in bug 315421, not here.)