578890 - something is leaking xpcom-shutdown nsObservers

Status: RESOLVED FIXED

(Core :: General, defect, P1)

Description

[Robert Sayre]

Top 20 a little after startup:
74 : xpcom-shutdown
20 : dom-window-destroyed
15 : memory-pressure
14 : private-browsing
12 : profile-before-change
11 : network:offline-status-changed
8 : dom-storage2-changed
8 : dom-storage-changed
7 : chrome-flush-skin-caches
7 : profile-do-change
5 : places-shutdown
5 : xpcom-category-entry-added
5 : browser:purge-session-history
5 : chrome-flush-caches
4 : xpcom-shutdown-threads
4 : profile-after-change
4 : quit-application
4 : formsubmit
4 : user-sheet-removed
4 : user-sheet-added



Top 20 a couple of hours later, with 1 DocShell and 6 DOMWindows:
188 : xpcom-shutdown
13 : private-browsing
13 : profile-before-change
10 : formsubmit
10 : profile-do-change
10 : dom-window-destroyed
9 : memory-pressure
7 : network:offline-status-changed
6 : xpcom-category-entry-added
6 : quit-application
5 : places-shutdown
5 : dom-storage2-changed
5 : dom-storage-changed
5 : xpcom-category-cleared
5 : chrome-flush-caches
5 : xpcom-category-entry-removed
4 : xpcom-shutdown-threads
4 : chrome-flush-skin-caches
4 : profile-after-change
4 : quit-application-granted

Comment 1

[Boris Zbarsky [:bzbarsky]]

This one is harder because we have so many places that add xpcom-shutdown observers...

If this is a build with symbols, is it worth attaching and seeing what the concrete types of the observer objects are like?  I bet they're all wrappedJS, but maybe we can get some mileage out of that, even.

I'm happy to try that if you give me some idea of what happened in those couple of hours.  Were you just browsing along?  Opening/closing windows?  Opening/closing tabs?  Something else?

Comment 2

[Robert Sayre]

I tried looking into it with DEBUG_CC, but it was really crashy. See bug 578893.

Comment 3

[Robert Sayre]

I was banging on it pretty hard, on purpose. I think the most effective technique was opening new windows full of excellent publications such as TechCrunch, The Register, and Mashable. Doing things like opening GMail and Zimbra weren't as effective.

Comment 4

[Robert Sayre]

Attachment: patch which summarizes the contents of the nsObserverService

Comment 5

[Robert Sayre]

Attachment: patch which summarizes the contents of the nsObserverService

Comment 6

[Benjamin Smedberg]

If you build with RTTI, you can just print the concrete types involved using typeid(observer).name(), which might help (or might not, if they're all implemented in JS).

Comment 7

[Robert Sayre]

This isn't a blocker? Should I stop working on it?

Comment 8

[Boris Zbarsky [:bzbarsky]]

I'm going to assume that this was marked blocking- incorrectly and renominate.  This seems like something we need to sort out.

Comment 9

[Seth Spitzer]

one example of this might be nsPrefBranch.

from nsPrefBranch::nsPrefBranch()

observerService->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, PR_TRUE);

but where is the corresponding call to remove the observer?

Something to note:  if you end up with lots of leaked xpcom-shutdown observers (for example, 100k of them) quitting the application can take a long time as they removed.

From a sample:

main
XRE_main
ScopedXPCOMStartup::~ScopedXPCOMStartup()
NS_ShutdownXPCOM_P
mozilla::ShutdownXPCOM(nsIServiceManager*)
nsObserverService::NotifyObservers()
nsObserverList::NotifyObservers()
nsObserverList::FillObserverArray()
nsTArray<ObserverRef>::RemoveElement<nsIWeakReference*>(nsIWeakReference*
const&)

Comment 10

[Boris Zbarsky [:bzbarsky]]

> but where is the corresponding call to remove the observer?

Hmm...  That seems to be relying on the holdWeak=true to remove the observer, but that only happens when someone tries to deliver the notification.... which in this case is at shutdown.  So yeah, that seems like a pretty likely candidate to me!

Comment 11

[Seth Spitzer]

what about:

nsPrefBranch::~nsPrefBranch()
{
  freeObserverList();

+ nsCOMPtr<nsIObserverService> observerService =
+         do_GetService("@mozilla.org/observer-service;1");
+ if (observerService)
+   observerService->RemoveObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID);
}

FWIW, that approach fixes it for me.

see
http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventStateManager.cpp#871
for a somewhat similar pattern.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Per comment 11. I verified that we do leak shutdown observers when doing window.open with features without this patch, and don't with it.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Stealing, if I may.

Comment 14

[(no longer active)]

Hmm, looking at this list: <http://mxr.mozilla.org/mozilla-central/ident?i=NS_XPCOM_SHUTDOWN_OBSERVER_ID> there seems to be quite a few other places in the code where something similar happens.  Should we apply the same fix in all of those places?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Looking at that list:

The various services are OK, since they stick around for the process lifetime anyway.

nsDOMStorage seems to be buggy.

nsMathMLChar removes itself.

nsIncrementalDownload seems to be buggy.

No idea whether nsWindowDataSource is a singleton or not, or even who'd know.

No idea whether the OS/2 nsClipboard is a singleton.

Appshell and plugin host are services.

SVGDocumentWrapper leaks itself for the app lifetime; dholbert filed bug 609814.

No idea whether IndexedDatabaseManager is a singleton.  Sicking?

nsDOMScriptObjectFactory is a service.

nsCategoryCache removes itself.

I have no idea what nsAccessProxy is and whether it's a singleton.

nsLayoutModule is using a singleton observer.

Callers of nsContentUtils::RegisterShutdownObserver end up with a strong ref, so need to call UnregisterShutdownObserver to avoid leaking self.  Looks like they all do that.

Ehsan, want to fix the definitely buggy bits here?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/493fe7f156e5

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

cc'ing bent for IndexedDatabaseManager info, though I do think it's a singleton.

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #17)
> cc'ing bent for IndexedDatabaseManager info, though I do think it's a
> singleton.

It is.

Comment 19

[(no longer active)]

(In reply to comment #15)
> Ehsan, want to fix the definitely buggy bits here?

Sure, I'll file the appropriate bugs for each one, with patches.

Comment 20

[(no longer active)]

(In reply to comment #10)
> > but where is the corresponding call to remove the observer?
> 
> Hmm...  That seems to be relying on the holdWeak=true to remove the observer,
> but that only happens when someone tries to deliver the notification.... which
> in this case is at shutdown.  So yeah, that seems like a pretty likely
> candidate to me!

Hmm, thinking more about this, is that assertion really true?  When you pass true as the third AddObserver param (and if the object supports nsISupportsWeakReference), then the observer service will not hold a strong reference to you, so there is no way that we leak the object because of that unless there is another strong reference to the object not freed properly.

The shutdown time concern is valid, though, but I think there is a better way to solve that.  Currently, when a topic is being fired, we copy the observers array to get a stable array for iteration, walk through it, and remove any weak observers which have gone away from the list, and then fire the notification on the remaining ones <http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsObserverList.cpp#98>.  For xpcom-shutdown and the rest of the topics fired here <http://mxr.mozilla.org/mozilla-central/source/xpcom/build/nsXPComInit.cpp#593>, which are the last of their kind ever fired, this is wasted effort, though, since we'll just free the list members which will be freed shortly afterwards at any rate.  Do you think it's worth filing a bug to exclude last time notified topics like this from this cleanup algorithm, to help with the shutdown time?

Comment 21

[Boris Zbarsky [:bzbarsky]]

> Hmm, thinking more about this, is that assertion really true

Yes, it is, but see below.

> so there is no way that we leak the object

We don't leak the nsIObserver object, but we leak the struct in which the observer service is keeping track of the object (or in other words get constantly growing mObservers for the relevant observer list).  That's what this bug was about.

> Do you think it's worth filing a bug to exclude last time notified topics like
> this from this cleanup algorithm

Worth at least looking into, yes.

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Could we printf a list of remaining xpcom-shutdown observers at shutdown in a debug build?  Would be useful information to have on tinderbox IMHO.

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #22)
> Could we printf a list of remaining xpcom-shutdown observers at shutdown in a
> debug build?  Would be useful information to have on tinderbox IMHO.

s/list/count/