One instance of CipherSuiteChangeObserver is leaking in the content process in e10s Mochitest 1 and Mochitest 3. In M3, there's a bunch of other stuff leaking, so maybe it isn't a direct cause, but I don't see a lot else in M1. It only seems to be destroyed when ShutdownNSS() is called, and that is called in ~nsNSSComponent and nsNSSComponent::DoProfileBeforeChange(). The latter isn't going to happen in a content process, because we don't do the profile change stuff there, so maybe that's the problem? Maybe not.

I don't believe ~nsNSSComponent will ever get called in content processes, because no nsNSSComponent will ever be constructed. EnsureNSSInitializedChromeOrContent does a sort-of bypass initialization of NSS with just the parts necessary for content processes, which eventually calls CipherSuiteChangeObserver::StartObserve. Unfortunately, it seems to be the case that CipherSuiteChangeObserver::StopObserve will never get called in content processes. Is there a different event that signals shutdown that we could listen for?

xpcom-shutdown would probably work. I think in non-debug builds we just kill the content process before it gets there, but that should fix the leak.

Registering CipherSuiteChangeObserver with the observer service, listening for xpcom-shutdown, seems to fix the leak for me.

This reduces the e10s leaks by a mighty 24 bytes. I'll wait until the try run comes back, but it at least passes the DOM crypto test directory. https://treeherder.mozilla.org/#/jobs?repo=try&revision=c91346c65ddd

Fixed some extra whitespace that crept in.

It seems pretty goofy to add in all of this code that is only run in debug builds (as Doug pointed out, this code is certainly larger than the leak it is fixing), so maybe I'll do an #ifdef so it is only built when we care about leak detection.

Only bother to do this in builds where we care about checking for shutdown leaks. try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=32c8f7f86d13

Comment on attachment 8676907 [details] [diff] [review] CipherSuiteChangeObserver should clean itself up outside of the main process. Review of attachment 8676907 [details] [diff] [review]: ----------------------------------------------------------------- This is a fine approach, but I think we can avoid the ifdefs if we make CipherSuiteChangeObserver always be in charge of its own uninitialization, rather than nsNSSComponent. In any case, r=me. ::: security/manager/ssl/nsNSSComponent.cpp @@ +797,5 @@ > sObserver = nullptr; > return rv; > } > +#ifdef NS_FREE_PERMANENT_DATA > + // If we're not in the main process, there is no nsNSSComponent to clean us up, so listen for nit: this file doesn't do a good job of it, but let's try to keep things to <80 characters per line @@ +798,5 @@ > return rv; > } > +#ifdef NS_FREE_PERMANENT_DATA > + // If we're not in the main process, there is no nsNSSComponent to clean us up, so listen for > + // shutdown to clean ourselves up. Why don't we just never rely on nsNSSComponent to shut this down and always observe xpcom shutdown instead? (That way, StopObserve() doesn't need to be static or public.)

(In reply to David Keeler [:keeler] (use needinfo?) from comment #8) > nit: this file doesn't do a good job of it, but let's try to keep things to > <80 characters per line I changed it so that no lines I added are greater than 80 lines. > Why don't we just never rely on nsNSSComponent to shut this down and always > observe xpcom shutdown instead? I made this change. try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=91d5c292a94b