1030392 - PeerConnectionImpl::Initialize needs to use standard NSS initialization

Status: NEW

(Core :: WebRTC: Signaling, defect, P5)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

+++ This bug was initially created as a clone of Bug #995385 +++

nsresult
CryptoTask::Dispatch(const nsACString& taskThreadName)
{
  MOZ_ASSERT(taskThreadName.Length() <= 15);

  // Ensure that NSS is initialized, since presumably CalculateResult
  // will use NSS functions
  if (!EnsureNSSInitializedChromeOrContent()) {
    return NS_ERROR_FAILURE;
  }

[...]

However, EnsureNSSInitializedChromeOrContent does not initialize NSS completely. For example, it only tries to initialize NSS using NSS_DoDB_Init(). But, if we initialize NSS with NSS_NoDB_Init(), doesn't this disable all the additional initialization that the other forms of NSS_Init() would do? In particular, doesn't this make the certificate database completely unavailable for the duration of the process?

AFAICT, EnsureNSSInitializedChromeOrContent must be changed to be like PeerConnectionImpl::Initialize:

  // Initialize NSS if we are in content process. For chrome process, NSS should already
  // been initialized.
  if (XRE_GetProcessType() == GeckoProcessType_Default) {
    // This code interferes with the C++ unit test startup code.
    nsCOMPtr<nsISupports> nssDummy = do_GetService("@mozilla.org/psm;1", &res);
    NS_ENSURE_SUCCESS(res, res);
  } else {
    NS_ENSURE_SUCCESS(res = InitNSSInContent(), res);
  }

Ideally, PeerConnectionImpl::Initialize would also be changed to call EnsureNSSInitializedChromeOrContent.

Besides the correctness issue, this also makes it hard to make changes to how NSS/PSM initialization works, because of the code duplication between EnsureNSSInitializedChromeOrContent and nsNSSComponent::InitializeNSS.

Comment 1

[Richard Barnes [:rbarnes]]

Attachment: bug-1030392.patch

Looking at this, it seems that EnsureNSSInitializedChromeOrContent is equivalent to the code you cite in PeerConnectionImpl::Initialize().  It just takes the body of InitNSSInContent() and imports it by value.  

So it seems like the only change needed is to make PeerConnectionImpl::Initialize() use EnsureNSSInitializedChromeOrContent instead of its own implementation.  I've changed the bug title and attributes to reflect this, and this patch makes the change.

Some more specific comments inline below.


(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #0)

> However, EnsureNSSInitializedChromeOrContent does not initialize NSS
> completely. For example, it only tries to initialize NSS using
> NSS_DoDB_Init().

This is not true.  It uses the standard PSM initialization process if it's called from a chrome process, and only uses NSS_NoDB_Init() if it's called from content.


> But, if we initialize NSS with NSS_NoDB_Init(), doesn't
> this disable all the additional initialization that the other forms of
> NSS_Init() would do? In particular, doesn't this make the certificate
> database completely unavailable for the duration of the process?

This is only true for the NSS instance used by the content process.  Since the certificate DB is primarily used by Necko and other chrome code, this doesn't seem like a huge concern.  And in any case, it applies to the PeerConnection code and this code equally.


> AFAICT, EnsureNSSInitializedChromeOrContent must be changed to be like
> PeerConnectionImpl::Initialize:

As noted above, it has the same structure as PeerConnectionImpl::Initialize(), just with the else block expanded:

http://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSComponent.cpp#75


> Ideally, PeerConnectionImpl::Initialize would also be changed to call
> EnsureNSSInitializedChromeOrContent.

See attached patch.


> Besides the correctness issue, this also makes it hard to make changes to
> how NSS/PSM initialization works, because of the code duplication between
> EnsureNSSInitializedChromeOrContent and nsNSSComponent::InitializeNSS.

Can nsNSSComponent::InitializeNSS be called from a content process?  If so, then these applications should probably call it.  If not, then EnsureNSSInitializedChromeOrContent is still needed, though it might be good to file a PSM bug to make it so that nsNSSComponent::InitializeNSS can be called from content.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8484164 [details] [diff] [review]
bug-1030392.patch

Review of attachment 8484164 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for me, but asking ekr as well

Comment 3

[Eric Rescorla (:ekr)]

Comment on attachment 8484164 [details] [diff] [review]
bug-1030392.patch

Review of attachment 8484164 [details] [diff] [review]:
-----------------------------------------------------------------

r- for failure to check the return value.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +740,5 @@
>    MOZ_ASSERT(mSTSThread);
>  #ifdef MOZILLA_INTERNAL_API
>  
> +  // Ensure that NSS has been initialized
> +  EnsureNSSInitializedChromeOrContent();

This function returns bool.

http://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSComponent.cpp?from=%20EnsureNSSInitializedChromeOrContent&case=true#72

Comment 4

[Richard Barnes [:rbarnes]]

Attachment: bug-1030392.1.patch

Sorry about that.  Now returns NS_ERROR_FAILURE if NSS fails to initialize, which seems coherent with the NS_ENSURE_SUCCESS() calls that this is replacing.

Comment 5

[Eric Rescorla (:ekr)]

Comment on attachment 8485574 [details] [diff] [review]
bug-1030392.1.patch

Review of attachment 8485574 [details] [diff] [review]:
-----------------------------------------------------------------

r=me pending comment below.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +741,5 @@
>  #ifdef MOZILLA_INTERNAL_API
>  
> +  // Ensure that NSS has been initialized
> +  if (!EnsureNSSInitializedChromeOrContent()) {
> +    return NS_ERROR_FAILURE;

This code isn't equivalent:

In the previous code, if someone else deinitialized NSS, this code
wouldn't re-run, but now it does (nssStarted versus NSS_IsInitialized()).

Are you sure this is what w ewant?

Comment 6

[Richard Barnes [:rbarnes]]

(In reply to Eric Rescorla (:ekr) from comment #5)
> Comment on attachment 8485574 [details] [diff] [review]
> bug-1030392.1.patch
> 
> Review of attachment 8485574 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me pending comment below.
> 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +741,5 @@
> >  #ifdef MOZILLA_INTERNAL_API
> >  
> > +  // Ensure that NSS has been initialized
> > +  if (!EnsureNSSInitializedChromeOrContent()) {
> > +    return NS_ERROR_FAILURE;
> 
> This code isn't equivalent:
> 
> In the previous code, if someone else deinitialized NSS, this code
> wouldn't re-run, but now it does (nssStarted versus NSS_IsInitialized()).
> 
> Are you sure this is what w ewant?

ISTM that the old behavior is actually broken.  Presumably the code that runs after this assumes that NSS is initialized.  If someone has uninitialized NSS, and this code doesn't re-initialize it, then that relying code will be calling NSS uninitialized.

I'm currently working through some try failures, then hope to land this soon, as long as you agree with the above.

Comment 7

[Richard Barnes [:rbarnes]]

Attachment: bug-1037892.2.patch

Ok, this patch is looking good in try.
https://tbpl.mozilla.org/?tree=Try&rev=60ce1b0a6fdf

EKR, could you please confirm you're OK with the header ordering hack here?  The cleaner alternative would be to find the symbol that's clashing and #undef it before we include nsNSSComponent.h.  I'm inclined to go with this solution for now, since (a) the problem is on Windows only (and I don't have a Windows box handy), (b) the problem appears to be in a CC header (thus local to WebRTC), and (c) I haven't been able to find an obvious solution. But I can push on it more (or open a follow-up bug) if you really prefer the alternative.  In any case, I've put a note in a comment.

Comment 8

[Eric Rescorla (:ekr)]

Barnes, is this still relevant?

Comment 9

[Eric Rescorla (:ekr)]

Comment on attachment 8496782 [details] [diff] [review]
bug-1037892.2.patch

Review of attachment 8496782 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review flag. Barnes, please re-ask for review if needed.

Comment 10

[Bulk Bug Changes for mreavy's org]

Mass change P4->P5 to align with new Mozilla triage process.