Closed Bug 1157515 Opened 9 years ago Closed 8 years ago
Suite Change Observer is leaked in e10s Mochitest 1 and Mochitest 3
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.
Assignee: nobody → continuation
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.
Attachment #8597502 - Attachment is obsolete: true
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
Attachment #8676907 - Flags: review?(dkeeler)
Attachment #8597504 - Attachment is obsolete: true
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.)
Attachment #8676907 - Flags: review?(dkeeler) → review+
(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
You need to log in before you can comment on or make changes to this bug.