NSS: fix session cache lock initialisation.

NSS: fix session cache lock initialisation.

This code previously used an unprotected global, that was accessed from
multiple threads, to guard whether global locks had been created. See bug for
details.

BUG=292999
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224311

[agl, 2013-09-18 23:24:57]

(I've not included the patch file in this change yet.)

wtc: this changes the code to simply use the PRCallOnceType to protect the lock
creation. Since a PRCallOnceType can be created statically, I think this solves
all the problems.

However, please feel free to take over this bug if you believe that it would be
faster for you to just do it yourself rather than code review something into
shape.

[Ryan Sleevi, 2013-09-19 01:14:13]

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/sslno...
File net/third_party/nss/ssl/sslnonce.c (right):

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/sslno...
net/third_party/nss/ssl/sslnonce.c:61: memset(&lockOnce, 0, sizeof(lockOnce));
Normal NSS style is to keep a 'static const PRCallOnceType' to assign to. I note
this is equivalent, just a style nit.

lockOnce = pristineCallOnce;

[wtc, 2013-09-19 01:19:57]

agl: thank you for writing this CL. I found the CL I wrote
for this problem. I will take care of this.

[wtc, 2013-09-19 04:37:51]

Patch set 1 LGTM.

After comparing your CL with mine, I think it's better to
consider my CL as a delta on top of your CL. My CL tries to
call ssl_InitSessionCacheLocks() in fewer places. So let's
check in your CL first as a baseline.

I know the style nits I ask you to fix come from the original
code.

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/ssl3c...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:5596: ssl_FreeSessionCacheLocks();

I think we should remove this ssl_FreeSessionCacheLocks call
and mark ssl_FreeSessionCacheLocks as static.

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/sslim...
File net/third_party/nss/ssl/sslimpl.h (right):

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/sslim...
net/third_party/nss/ssl/sslimpl.h:1850: extern SECStatus
ssl_FreeSessionCacheLocks(void);

If we mark ssl_FreeSessionCacheLocks as static, remove this
declaration.

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/sslno...
File net/third_party/nss/ssl/sslnonce.c (right):

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/sslno...
net/third_party/nss/ssl/sslnonce.c:43: ssl_FreeSessionCacheLocks()

Nit: add "void" inside the parentheses because this is C code.

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/sslno...
net/third_party/nss/ssl/sslnonce.c:50: return SECSuccess;

BUG: remove this return statement.

It may be better to rewrite this if-else statement as follows:

    if (!cacheLock) {
        PORT_SetError(SEC_ERROR_NOT_INITIALIZED);
        return SECFailure;
    }
    PZ_DestroyLock(cacheLock);
    cacheLock = NULL;

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/sslno...
net/third_party/nss/ssl/sslnonce.c:62: return SECFailure;

BUG: this should return SECSuccess.

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/sslno...
net/third_party/nss/ssl/sslnonce.c:67: static SECStatus ssl_ShutdownLocks(void*
appData, void* nssData)

Nit: NSS style folds this line as follows:

static SECStatus
ssl_ShutdownLocks(void* appData, void* nssData)

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/sslno...
net/third_party/nss/ssl/sslnonce.c:72: /* initSessionCacheLocks is called,
protected by lockOnce, to create the

Nit: capitalize this function name "InitSessionCacheLocks".

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/sslno...
net/third_party/nss/ssl/sslnonce.c:74: static PRStatus
initSessionCacheLocks(void)

Fold this line as follows:

static PRStatus
initSessionCacheLocks(void)

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/sslno...
net/third_party/nss/ssl/sslnonce.c:84: PRErrorCode rc = PORT_GetError();

Nit: name this variable |error|.

[agl, 2013-09-19 17:40:51]

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/ssl3c...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:5596: ssl_FreeSessionCacheLocks();
On 2013/09/19 04:37:51, wtc wrote:
> 
> I think we should remove this ssl_FreeSessionCacheLocks call
> and mark ssl_FreeSessionCacheLocks as static.

Done.

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/sslim...
File net/third_party/nss/ssl/sslimpl.h (right):

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/sslim...
net/third_party/nss/ssl/sslimpl.h:1850: extern SECStatus
ssl_FreeSessionCacheLocks(void);
On 2013/09/19 04:37:51, wtc wrote:
> 
> If we mark ssl_FreeSessionCacheLocks as static, remove this
> declaration.

Done.

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/sslno...
File net/third_party/nss/ssl/sslnonce.c (right):

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/sslno...
net/third_party/nss/ssl/sslnonce.c:43: ssl_FreeSessionCacheLocks()
On 2013/09/19 04:37:51, wtc wrote:
> 
> Nit: add "void" inside the parentheses because this is C code.

Done.

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/sslno...
net/third_party/nss/ssl/sslnonce.c:50: return SECSuccess;
On 2013/09/19 04:37:51, wtc wrote:
> 
> BUG: remove this return statement.
> 
> It may be better to rewrite this if-else statement as follows:
> 
>     if (!cacheLock) {
>         PORT_SetError(SEC_ERROR_NOT_INITIALIZED);
>         return SECFailure;
>     }
>     PZ_DestroyLock(cacheLock);
>     cacheLock = NULL;

Done.

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/sslno...
net/third_party/nss/ssl/sslnonce.c:61: memset(&lockOnce, 0, sizeof(lockOnce));
On 2013/09/19 01:14:13, Ryan Sleevi wrote:
> Normal NSS style is to keep a 'static const PRCallOnceType' to assign to. I
note
> this is equivalent, just a style nit.
> 
> lockOnce = pristineCallOnce;

Done.

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/sslno...
net/third_party/nss/ssl/sslnonce.c:62: return SECFailure;
On 2013/09/19 04:37:51, wtc wrote:
> 
> BUG: this should return SECSuccess.

Done.

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/sslno...
net/third_party/nss/ssl/sslnonce.c:67: static SECStatus ssl_ShutdownLocks(void*
appData, void* nssData)
On 2013/09/19 04:37:51, wtc wrote:
> 
> Nit: NSS style folds this line as follows:
> 
> static SECStatus
> ssl_ShutdownLocks(void* appData, void* nssData)

Done.

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/sslno...
net/third_party/nss/ssl/sslnonce.c:72: /* initSessionCacheLocks is called,
protected by lockOnce, to create the
On 2013/09/19 04:37:51, wtc wrote:
> 
> Nit: capitalize this function name "InitSessionCacheLocks".

Done.

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/sslno...
net/third_party/nss/ssl/sslnonce.c:74: static PRStatus
initSessionCacheLocks(void)
On 2013/09/19 04:37:51, wtc wrote:
> 
> Fold this line as follows:
> 
> static PRStatus
> initSessionCacheLocks(void)

Done.

https://codereview.chromium.org/24216005/diff/1/net/third_party/nss/ssl/sslno...
net/third_party/nss/ssl/sslnonce.c:84: PRErrorCode rc = PORT_GetError();
On 2013/09/19 04:37:51, wtc wrote:
> 
> Nit: name this variable |error|.

Done.

[wtc, 2013-09-19 19:23:53]

Patch set 2 LGTM. Nice cleanup in sslnonce.c!

https://codereview.chromium.org/24216005/diff/10001/net/third_party/nss/READM...
File net/third_party/nss/README.chromium (right):

https://codereview.chromium.org/24216005/diff/10001/net/third_party/nss/READM...
net/third_party/nss/README.chromium:119: * Fix session cache lock creation race.

Please add this NSS bug URL:
https://bugzilla.mozilla.org/show_bug.cgi?id=764646

[agl, 2013-09-19 19:26:06]

https://codereview.chromium.org/24216005/diff/10001/net/third_party/nss/READM...
File net/third_party/nss/README.chromium (right):

https://codereview.chromium.org/24216005/diff/10001/net/third_party/nss/READM...
net/third_party/nss/README.chromium:119: * Fix session cache lock creation race.
On 2013/09/19 19:23:53, wtc wrote:
> 
> Please add this NSS bug URL:
> https://bugzilla.mozilla.org/show_bug.cgi?id=764646

Done.

[commit-bot: I haz the power, 2013-09-19 19:27:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/24216005/23001

[commit-bot: I haz the power, 2013-09-20 06:47:10]

Change committed as 224311