replace nssEnsure* mechanism with EnsureNSSInitializedChromeOrContent and make it safe to use off the main thread

RESOLVED FIXED in Firefox 55

Status

()

Core
Security: PSM
P1
normal
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [psm-assigned])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

The nssEnsure* mechanism is a little hard to understand (e.g. does nssEnsureOnChromeOnly mean ensure *that* we're only in the parent process or only ensure *when* we're in the parent process?). Also, EnsureNSSInitializedChromeOrContent almost does what we want - ensure NSS is appropriately initialized whether we're in the child process or parent process and whether we're on the main thread or not, but it doesn't quite. The aim of this bug is to fix both of these issues so we can just use EnsureNSSInitializedChromeOrContent without worrying if we're on the main thread. (I think we can use this to fix bug 1301407.)
Comment hidden (mozreview-request)

Comment 2

8 months ago
Hey keeler, just a heads up that this is going to take me a little longer to review.

In the mean time, I noticed Bug 966467 added a similar mechanism for controlling the processes services and components can be loaded in - maybe we use that instead for the process restrictions?
(Assignee)

Comment 3

8 months ago
(In reply to :Cykesiopka from comment #2)
> Hey keeler, just a heads up that this is going to take me a little longer to
> review.

No worries - thanks for letting me know.

> In the mean time, I noticed Bug 966467 added a similar mechanism for
> controlling the processes services and components can be loaded in - maybe
> we use that instead for the process restrictions?

Hmmm - we could definitely make use of that, but I'm concerned that we'd be going from default parent-process-only to default any process, and it would be easy to do the wrong thing. In any case, I think I should get an additional review on this, so we'll see what JC says.
(Assignee)

Updated

8 months ago
Attachment #8836924 - Flags: review?(jjones)

Comment 4

8 months ago
mozreview-review
Comment on attachment 8836924 [details]
bug 1339267 - re-work NSS initialization wrt thread/process etc.

https://reviewboard.mozilla.org/r/112234/#review115960

Sorry for the delay.

This looks great, and I'm glad to see the weird EnsureNSSOperator enum go.

::: security/manager/ssl/moz.build
(Diff revision 1)
>      'nsNSSASN1Object.cpp',
>      'nsNSSCallbacks.cpp',
>      'nsNSSCertHelper.cpp',
>      'nsNSSCertificate.cpp',
>      'nsNSSCertificateDB.cpp',
> -    'nsNSSCertificateFakeTransport.cpp',

Looks like there's a `friend class nsNSSCertificateFakeTransport;` line in `nsNSSCertificate.h` that should be removed as well.

::: security/manager/ssl/nsNSSCertificate.cpp:38
(Diff revision 1)
>  #include "nsProxyRelease.h"
>  #include "nsReadableUtils.h"
>  #include "nsString.h"
>  #include "nsThreadUtils.h"
>  #include "nsUnicharUtils.h"
>  #include "nsXULAppAPI.h"

Nit: I believe this include can be removed now.

::: security/manager/ssl/nsNSSComponent.h:170
(Diff revision 1)
>    nsresult setEnabledTLSVersions();
>    nsresult InitializePIPNSSBundle();
>    nsresult ConfigureInternalPKCS11Token();
>    nsresult RegisterObservers();
>  
>    void DoProfileBeforeChange();

This should be removed as well.
Attachment #8836924 - Flags: review?(cykesiopka.bmo) → review+

Comment 5

8 months ago
(In reply to David Keeler [:keeler] (use needinfo?) from comment #3)
> Hmmm - we could definitely make use of that, but I'm concerned that we'd be
> going from default parent-process-only to default any process, and it would
> be easy to do the wrong thing.

Good point. I guess we can just revisit this later.

Comment 6

8 months ago
So just FYI, I've been looking at this on-and-off all day and I'm still wrapping my head around the code. It's going to take me a little bit more time to really review it. Sorry!
(Assignee)

Comment 7

8 months ago
mozreview-review-reply
Comment on attachment 8836924 [details]
bug 1339267 - re-work NSS initialization wrt thread/process etc.

https://reviewboard.mozilla.org/r/112234/#review115960

Thanks!

> Looks like there's a `friend class nsNSSCertificateFakeTransport;` line in `nsNSSCertificate.h` that should be removed as well.

Good catch.

> Nit: I believe this include can be removed now.

Sounds good.

> This should be removed as well.

Good catch again.
(Assignee)

Comment 8

8 months ago
(In reply to J.C. Jones [:jcj] from comment #6)
> So just FYI, I've been looking at this on-and-off all day and I'm still
> wrapping my head around the code. It's going to take me a little bit more
> time to really review it. Sorry!

No worries - the other review in your queue is probably a bit more important at the moment :)

Comment 9

8 months ago
mozreview-review
Comment on attachment 8836924 [details]
bug 1339267 - re-work NSS initialization wrt thread/process etc.

https://reviewboard.mozilla.org/r/112234/#review117512

This looks OK to me, keeping in mind the friend Cykesiopka pointed out. Hopefully this more consistent way of declaring thread usage will make bugs a lot simpler to find.

My only nit is that we may want to consider adding debug assertions where classes get initialized off the correct process / thread and where we return `false` without forwarding. As-is, we're assuming the calling code is handling the assertions correctly. But we can do that as a follow-up if it appears useful due to regressions or whatnot.
Attachment #8836924 - Flags: review?(jjones) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Thanks for the reviews. Try looked like a small basket of oranges, but I'm fairly sure that's not due to these changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b374a1f49069

Comment 13

8 months ago
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/06328362721a
re-work NSS initialization wrt thread/process etc. r=Cykesiopka,jcj

Comment 14

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/06328362721a
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Updated

7 months ago
Duplicate of this bug: 1301407
You need to log in before you can comment on or make changes to this bug.