Leaked observer service leaks things on shutdown

RESOLVED FIXED in mozilla1.8.1

Status

()

Core
XPCOM
P2
normal
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Darin Fisher, Assigned: Benjamin Smedberg)

Tracking

({memory-leak})

Trunk
mozilla1.8.1
memory-leak
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

12 years ago
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?
(Assignee)

Updated

12 years ago
Blocks: 324586
(Assignee)

Updated

12 years ago
Assignee: nobody → benjamin
Priority: -- → P2
Target Milestone: --- → mozilla1.8.1
(Assignee)

Comment 2

12 years ago
Created attachment 213077 [details] [diff] [review]
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).
Attachment #213077 - Flags: review?(darin)
(Reporter)

Comment 3

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

Nope.  Sorry for the late reply.
(Reporter)

Comment 4

12 years ago
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.
(Assignee)

Comment 5

12 years ago
Because we'd end up creating more observer objects and I want it to fail. I have a solution coming up.
(Assignee)

Comment 6

12 years ago
Created attachment 213087 [details] [diff] [review]
Forceful removal, rev. 1.1
Attachment #213077 - Attachment is obsolete: true
Attachment #213087 - Flags: review?(darin)
Attachment #213077 - Flags: review?(darin)
(Reporter)

Comment 7

12 years ago
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.
Attachment #213087 - Flags: review?(darin) → review-
(Assignee)

Comment 8

12 years ago
Created attachment 213088 [details] [diff] [review]
Forceful removal, rev. 1.2
Attachment #213087 - Attachment is obsolete: true
Attachment #213088 - Flags: review?(darin)
(Reporter)

Comment 9

12 years ago
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 :)
Attachment #213088 - Flags: review?(darin) → review-
(Assignee)

Comment 10

12 years ago
Created attachment 213812 [details] [diff] [review]
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.
Attachment #213812 - Flags: review?(darin)
(Reporter)

Comment 11

12 years ago
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
Attachment #213812 - Flags: review?(darin) → review+
(Assignee)

Comment 12

12 years ago
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
Attachment #213812 - Attachment description: Shutdown timers much earlier, rev. 1 → Shutdown timers much earlier, rev. 1 [checked in]
(Assignee)

Comment 13

12 years ago
Created attachment 213893 [details] [diff] [review]
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).
Attachment #213088 - Attachment is obsolete: true
Attachment #213893 - Flags: review?(darin)
(Reporter)

Comment 14

12 years ago
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?
Attachment #213893 - Flags: review?(darin) → review+
(Assignee)

Comment 15

12 years ago
Fixed on trunk with nits.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Summary: Observer service leaked on shutdown → Leaked bbserver service leaks things on shutdown
(Assignee)

Updated

12 years ago
Summary: Leaked bbserver service leaks things on shutdown → Leaked observer service leaks things on shutdown

Updated

12 years ago
Depends on: 329505
(Reporter)

Comment 16

12 years ago
backed out to fix bug 329505
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 17

12 years ago
The "shutdown timers much earlier" patch probably caused bug 331012.
Depends on: 331012
(Assignee)

Comment 18

12 years ago
Created attachment 215682 [details] [diff] [review]
Forceful removal, rev. 3

This changes the NotifyObservers code to fill a temporary array the same way the EnumerateObservers does.
Attachment #213893 - Attachment is obsolete: true
Attachment #215682 - Flags: review?(darin)
(Assignee)

Comment 19

12 years ago
Please ignore the extraneous nsArrayEnumerator.h #include, it's an accident.
(Reporter)

Comment 20

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

r=darin
Attachment #215682 - Flags: review?(darin) → review+
(Assignee)

Comment 21

12 years ago
Fixed on trunk.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
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.)
You need to log in before you can comment on or make changes to this bug.