326491 - Leaked observer service leaks things on shutdown

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Darin Fisher]

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

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 2

[Benjamin Smedberg]

Attachment: Forceful removal, rev. 1

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).

Comment 3

[Darin Fisher]

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

Nope.  Sorry for the late reply.

Comment 4

[Darin Fisher]

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.

Comment 5

[Benjamin Smedberg]

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

Comment 6

[Benjamin Smedberg]

Attachment: Forceful removal, rev. 1.1

Comment 7

[Darin Fisher]

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 8

[Benjamin Smedberg]

Attachment: Forceful removal, rev. 1.2

Comment 9

[Darin Fisher]

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 :)

Comment 10

[Benjamin Smedberg]

Attachment: Shutdown timers much earlier, rev. 1 [checked in]

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

Comment 11

[Darin Fisher]

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 12

[Benjamin Smedberg]

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

Comment 13

[Benjamin Smedberg]

Attachment: Forceful removal, rev. 2

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 14

[Darin Fisher]

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?

Comment 15

[Benjamin Smedberg]

Fixed on trunk with nits.

Comment 16

[Darin Fisher]

backed out to fix bug 329505

Comment 17

[Benjamin Smedberg]

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

Comment 18

[Benjamin Smedberg]

Attachment: Forceful removal, rev. 3

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

Comment 19

[Benjamin Smedberg]

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

Comment 20

[Darin Fisher]

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

r=darin

Comment 21

[Benjamin Smedberg]

Fixed on trunk.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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