Closed Bug 1218596 Opened 5 years ago Closed 5 years ago

remove nsPSMInitPanic and other unnecessary items from nsNSSComponent

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- affected
firefox45 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

Attachments

(1 file)

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).
I've discovered a few other unnecessary items in nsNSSComponent:

mObserversRegistered
mIsNetworkDown
NSSBundleFormatStringFromName
DeregisterObservers
nsNSSComponent as an nsISupportsWeakReference
Assignee: nobody → dkeeler
Summary: remove nsPSMInitPanic → remove nsPSMInitPanic and other unnecessary items from nsNSSComponent
bug 1218596 - remove nsPSMInitPanic and other unnecessary things from nsNSSComponent r?Cykesiopka r?jcj
Attachment #8679709 - Flags: review?(jjones)
Attachment #8679709 - Flags: review?(cykesiopka.bmo)
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?
Attachment #8679709 - Flags: review?(cykesiopka.bmo) → review+
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.
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 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.
Attachment #8679709 - Flags: review?(jjones) → review+
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
All of the failures so far have been on XP. 8 appears to be building okay.
My bad. Looks like I forgot to update a corresponding definition. This fixed it, though:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5973bf75573
Flags: needinfo?(dkeeler)
https://hg.mozilla.org/mozilla-central/rev/7b574db9b1c6
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.