1218596 - remove nsPSMInitPanic and other unnecessary items from nsNSSComponent

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

nsPSMInitPanic was added in bug 521849 but is no longer necessary (furthermore, it's not actually clear to me that it was ever effective at its stated goal).

Comment 1

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

I've discovered a few other unnecessary items in nsNSSComponent:

mObserversRegistered
mIsNetworkDown
NSSBundleFormatStringFromName
DeregisterObservers
nsNSSComponent as an nsISupportsWeakReference

Comment 2

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

Attachment: MozReview Request: bug 1218596 - remove nsPSMInitPanic and other unnecessary things from nsNSSComponent r?Cykesiopka r?jcj

bug 1218596 - remove nsPSMInitPanic and other unnecessary things from nsNSSComponent r?Cykesiopka r?jcj

Comment 3

[:Cykesiopka]

Comment on attachment 8679709 [details]
MozReview Request: bug 1218596 - remove nsPSMInitPanic and other unnecessary things from nsNSSComponent r?Cykesiopka r?jcj

https://reviewboard.mozilla.org/r/23517/#review21117

LGTM - just one question.

::: security/manager/ssl/nsNSSComponent.cpp
(Diff revision 1)
> -    // we make sure that we won't get unloaded until the application shuts down.

(Assuming this even worked) I guess we don't care that much about being unloaded?
Or is there some mechanism I'm missing that does this?

Comment 4

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

https://reviewboard.mozilla.org/r/23517/#review21117

> (Assuming this even worked) I guess we don't care that much about being unloaded?
> Or is there some mechanism I'm missing that does this?

We do care about getting unloaded (and the code that ensures this hasn't changed, unless I did something wrong). I just didn't feel like the comment added much. I can add it back, or amend it and add it back.

Comment 5

[:Cykesiopka]

https://reviewboard.mozilla.org/r/23517/#review21117

> We do care about getting unloaded (and the code that ensures this hasn't changed, unless I did something wrong). I just didn't feel like the comment added much. I can add it back, or amend it and add it back.

Ok, sounds good. I was mainly curious why the comment was being removed. Everything is fine as-is.

Comment 6

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8679709 [details]
MozReview Request: bug 1218596 - remove nsPSMInitPanic and other unnecessary things from nsNSSComponent r?Cykesiopka r?jcj

https://reviewboard.mozilla.org/r/23517/#review21787

I'm not 100% that I understand parts of this, but I don't see anything that looks problematic. I've been searching through related code for the past several days - between sessions - and while I just don't understand the consequences of everything, it'd have to be real subtle at this point to be a problem.

Comment 7

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

Thanks for the reviews. I decided to hold off on removing mIsNetworkDown since I felt like it made more sense to make that change with an upcoming notification observation refactoring.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=045371b7d76a

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae1885cf1fd6

Comment 9

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

Backed out for windows build bustage https://treeherder.mozilla.org/logviewer.html#?job_id=16881684&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a440af6cb51

Comment 10

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

All of the failures so far have been on XP. 8 appears to be building okay.

Comment 11

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

My bad. Looks like I forgot to update a corresponding definition. This fixed it, though:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5973bf75573

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b574db9b1c6

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7b574db9b1c6