1157515 - [e10s] CipherSuiteChangeObserver is leaked in e10s Mochitest 1 and Mochitest 3

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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?

Comment 2

[Andrew McCreight [:mccr8]]

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.

Comment 3

[Andrew McCreight [:mccr8]]

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

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: CipherSuiteChangeObserver should clean itself up outside of the main process.

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

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: CipherSuiteChangeObserver should clean itself up outside of the main process.

Fixed some extra whitespace that crept in.

Comment 6

[Andrew McCreight [:mccr8]]

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.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: CipherSuiteChangeObserver should clean itself up outside of the main process.

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 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.)

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c702af0b732

Comment 10

[Andrew McCreight [:mccr8]]

(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

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0c702af0b732