1215690 - remove nsPSMUITracker

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

nsPSMUITracker is problematic at the moment. I believe it was intended to prevent NSS shutdown while NSS-related UI operations were going on (such as choosing a client certificate). However, when nsNSSComponent receives the event that tells it to shutdown NSS, it attempts to call mShutdownObjectList->evaporateAllNSSResources(), which calls mActivityState.restrictActivityToCurrentThread(), which fails if such a UI operation is in progress. This actually prevents the important part of evaporateAllNSSResources, which is the releasing of all NSS objects in use by PSM objects. Importantly, nsNSSComponent doesn't check for or handle this failure and proceeds to call NSS_Shutdown(), leaving PSM in an inconsistent state where it thinks it's okay to keep using the NSS objects it has when in fact it isn't.
In any case, nsPSMUITracker isn't really necessary as long as we have the nsNSSShutDownPreventionLock mechanism, which mostly works and is what we should use instead (or not at all, if no such lock is needed for the operation being performed).

Comment 1

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

Attachment: MozReview Request: bug 1215690 - remove nsPSMUITracker

bug 1215690 - remove nsPSMUITracker

Comment 2

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8676350 [details]
MozReview Request: bug 1215690 - remove nsPSMUITracker

https://reviewboard.mozilla.org/r/22669/#review20349

This looks sensible. I have one question (repeated 3 times). r=me if that is, indeed, OK as is.

::: security/manager/ssl/nsCertPicker.cpp:36
(Diff revision 1)
> +

Should we call destructorSafeDestroyNSSReference() here?

(see https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/nsNSSShutDown.h#173 )

::: security/manager/ssl/nsKeygenHandler.cpp:273
(Diff revision 1)
> +

Again, should we call destructorSafeDestroyNSSReference() ?

::: security/manager/ssl/nsSDR.cpp:52
(Diff revision 1)
> +

destructorSafeDestroyNSSReference()?

Comment 3

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

https://reviewboard.mozilla.org/r/22669/#review20349

Thanks for the review - I answered the question inline.

> Should we call destructorSafeDestroyNSSReference() here?
> 
> (see https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/nsNSSShutDown.h#173 )

Since there isn't anything to release, we don't need to define/call destructorSafeDestroyNSSReference().

> Again, should we call destructorSafeDestroyNSSReference() ?

Same here.

> destructorSafeDestroyNSSReference()?

Same thing.

Comment 4

[:Cykesiopka]

Comment on attachment 8676350 [details]
MozReview Request: bug 1215690 - remove nsPSMUITracker

https://reviewboard.mozilla.org/r/22669/#review20463

Nice.

::: security/manager/ssl/nsNSSComponent.cpp:1637
(Diff revision 1)
>    nsNSSShutDownPreventionLock locker;

Looks like a check for isAlreadyShutDown() should be added here as well?

Comment 5

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

(In reply to Cykesiopka from comment #4)
Thanks for the review!

> ::: security/manager/ssl/nsNSSComponent.cpp:1637
> (Diff revision 1)
> >    nsNSSShutDownPreventionLock locker;
> 
> Looks like a check for isAlreadyShutDown() should be added here as well?

Good call. I ended up hoisting the shutdown prevention lock to the callers and making them responsible for doing the check.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4b35c1e538c

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/406f9bce7d23

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/406f9bce7d23