Closed Bug 1339267 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Security: PSM, defect, P1)

Product:
Core
Component:
Security: PSM
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

bug 1339267 - re-work NSS initialization wrt thread/process etc.
59 bytes, text/x-review-board-request
Cykesiopka
: review+
jcj
: review+
Details 
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.)
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?
(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.
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+
(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.
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!
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.
(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 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+
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
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/06328362721a
re-work NSS initialization wrt thread/process etc. r=Cykesiopka,jcj
https://hg.mozilla.org/mozilla-central/rev/06328362721a
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: