806992 - EV initialization blocks the first certificate verification thread for a long time

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Description

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

According to mcmanus, initialization of the EV--nsNSSComponent::IdentityInfoInit()--blocks the first certificate validation by ~30ms.

Patrick, please confirm.

Comment 1

[Patrick McManus [:mcmanus]]

(In reply to Brian Smith (:bsmith) from comment #0)
> According to mcmanus, initialization of the
> EV--nsNSSComponent::IdentityInfoInit()--blocks the first certificate
> validation by ~30ms.
> 
> Patrick, please confirm.

ack.

more like 20ms in my opt build, but still a lot. (core i7)

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: patch 0

you may want a different patch, but in the spirit of writing code instead of bugzilla comments I have been using this one for the last hour with good results for the false start work (i.e. calling handshakecallback out of AuthCertificateComplete instead of sendClientSecondRound is adding about 1ms of delay instead of 20-30).

It adds a one-time hook when starting a TLS connection to ensure the EV.init is done (and does it on a server cert verification thread, so it doesn't block the connection logic). The effectively overlaps the SSL round trip with the EV.init

NSS itself seems lazily init'd - trying the same logic out of the socket transport init routine before that happens triggers the nss init, which was a pretty unhappy thing off the main thread. Likewise, making it synchronous back on the main thread during startup looked like it was undoing some work that was intentionally done to speed startup.

So lazy makes some sense, but doing it in parallel with a connect should get us the best of both worlds unless you've got a top 5% network.

Comment 3

[Patrick McManus [:mcmanus]]

Attachment: patch 1

The android compiler objected to how I defined a class inside a function and passed a RefPtr to it outside..

the patch update just moves the class a few lines up.

Comment 4

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

Comment on attachment 681776 [details] [diff] [review]
patch 1

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

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +1217,5 @@
> +private:
> +  NS_IMETHOD Run()
> +  {
> +    nsresult rv;
> +    nsCOMPtr<nsINSSComponent> inss = do_GetService(PSM_COMPONENT_CONTRACTID, &rv);

You need to prevent NSS from shutting down and you need to check that NSS hasn't already shut down. The easiest way to do this is to use the CryptoTask helper class from bug 804663.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +2358,5 @@
>    }
> +
> +  // This is an optimization to make sure the identity info dataset is
> +  // parsed on a seaparte thread and can be overlapped with network latency.
> +  EnsureServerVerificationInitialized();

The initialization of the EV info will race against all the actual validations that depend on it, right? It seems like the patch is written on the assumption that all validations are serialized, but actually the serializations are done on a 5-thread thread pool.

Comment 5

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

I think that this can be resolved by moving this runtime initialization to build time, or by (re)moving the unnecessary work that the EV identity info initialization is doing. AFAICT, at most it should only need to register OIDs, but instead it is reading certificates out of the cert DB (off disk) for reasons that are unclear to me (so, perhaps unnecessarily).

Comment 6

[Patrick McManus [:mcmanus]]

(In reply to Brian Smith (:bsmith) from comment #4)

> 
> You need to prevent NSS from shutting down and you need to check that NSS
> hasn't already shut down. The easiest way to do this is to use the
> CryptoTask helper class from bug 804663.

thanks.. I'll skip the dep on 804663 and do it the old school way.

> 
> ::: security/manager/ssl/src/nsNSSIOLayer.cpp
> @@ +2358,5 @@
> >    }
> > +
> > +  // This is an optimization to make sure the identity info dataset is
> > +  // parsed on a seaparte thread and can be overlapped with network latency.
> > +  EnsureServerVerificationInitialized();
> 
> The initialization of the EV info will race against all the actual
> validations that depend on it, right? It seems like the patch is written on
> the assumption that all validations are serialized, but actually the
> serializations are done on a 5-thread thread pool.

I don't believe serialization is assumed - and I think this is ok because in the end it boils down to triggering 1 early call of EnsureIdentityInfoLoaded() on a cert verification thread so that the loading can happen while the pre certificate exchanges are going on. All cert verifications also end up calling EnsureIdentityInfoLoaded just like they used to in case they are ready for verification before the identity info loading is complete.

http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsIdentityChecking.cpp#1319

EnsureIdentityInfoLoaded() uses PR_CallOnce to drive the actual initialization through IdentityInfoInit.. CallOnce() guarantees both that IdentityInfoInit is only called once and that any 'extra' threads racing against that first execution all wait until IdentityInfoInit() has finished executing (so they can't proceed with incomplete initialization). Indeed nothing has really changed here wrt parallelism, I've just tossed one more EnsureIdentityInfo() into the mix.

The common case of CallOnce (i.e. post initialization) is really cheap - an unlocked comparison.

http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/misc/prinit.c#773

So I think we're on solid footing there.

Comment 7

[Patrick McManus [:mcmanus]]

Attachment: patch 2

let's see how I did with the nsNSSShutDown incantation

Comment 8

[Patrick McManus [:mcmanus]]

Comment on attachment 682169 [details] [diff] [review]
patch 2

double reviews! (I think I want r? here ... if I want sr? then by all means do that :))

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 682169 [details] [diff] [review]
patch 2

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

r=honzab

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +1223,5 @@
> +    if (isAlreadyShutDown())
> +      return NS_OK;
> +
> +    nsresult rv;
> +    nsCOMPtr<nsINSSComponent> inss = do_GetService(PSM_COMPONENT_CONTRACTID, &rv);

If PSM_COMPONENT has already been instantiated on the main thread, then this is OK.  I believe it has been.

Comment 10

[Patrick McManus [:mcmanus]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b2cc2d9da6be

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b2cc2d9da6be