Closed Bug 578890 Opened 11 years ago Closed 11 years ago

something is leaking xpcom-shutdown nsObservers

Categories

(Core :: General, defect, P1)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- final+

People

(Reporter: sayrer, Assigned: bzbarsky)

Details

(Keywords: memory-leak)

Attachments

(2 files, 1 obsolete file)

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
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?
blocking2.0: --- → ?
I tried looking into it with DEBUG_CC, but it was really crashy. See bug 578893.
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.
Attachment #457737 - Attachment is obsolete: true
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).
blocking2.0: ? → -
This isn't a blocker? Should I stop working on it?
I'm going to assume that this was marked blocking- incorrectly and renominate.  This seems like something we need to sort out.
blocking2.0: - → ?
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&)
> 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!
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.
Assignee: nobody → dwitte
blocking2.0: ? → final+
Attachment #485468 - Flags: review?(benjamin)
Stealing, if I may.
Assignee: dwitte → bzbarsky
Priority: -- → P1
Whiteboard: [need review]
Attachment #485468 - Flags: review?(benjamin) → review+
Whiteboard: [need review] → [need landing]
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?
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?
Pushed http://hg.mozilla.org/mozilla-central/rev/493fe7f156e5
Whiteboard: [need landing]
Target Milestone: --- → mozilla2.0b8
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
cc'ing bent for IndexedDatabaseManager info, though I do think it's a singleton.
(In reply to comment #17)
> cc'ing bent for IndexedDatabaseManager info, though I do think it's a
> singleton.

It is.
(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.
Keywords: mlk
(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?
> 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.
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.
(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/
You need to log in before you can comment on or make changes to this bug.