Add a unit test to check KeygenHandler's thread-safety

Add a unit test to check KeygenHandler's thread-safety

We'll want some semblance of thread-safety when we make keygen asynchronous.

R=wtc,mattm
BUG=148
TEST=unit test

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=50903

[wtc, 2010-06-17 18:34:32]

LGTM!

http://codereview.chromium.org/2838010/diff/2001/3002
File net/base/keygen_handler_unittest.cc (right):

http://codereview.chromium.org/2838010/diff/2001/3002#newcode49
net/base/keygen_handler_unittest.cc:49: void
AssertValidKeyAndSignedChallenge(const std::string& result,
Nit: the function name should be
  AssertValidSignedPublicKeyAndChallenge
because both the public key and challenge are signed.

I assume the |challenge| input parameter is for future use?

http://codereview.chromium.org/2838010/diff/2001/3002#newcode162
net/base/keygen_handler_unittest.cc:162: // Perform the same verifications as
the smoke test above.
You can delete this comment now, because the use of the
AssertValidxxx function implies that.

[davidben, 2010-06-18 18:53:24]

Addressed comments and uploaded a series of changes.

I'll look into adding locks as appropriate to the Windows and Mac versions after
lunch.

[wtc, 2010-06-18 19:04:09]

http://codereview.chromium.org/2838010/diff/9001/10002
File base/nss_util.h (right):

http://codereview.chromium.org/2838010/diff/9001/10002#newcode46
base/nss_util.h:46: Lock* nss_write_lock();
This function should be named GetNSSWriteLock or
GetNSSDatabaseWriteLock to follow the Style Guide.
A name like nss_write_lock should only be used for a getter
of a class data member.

You can put the declaration of this function inside
    #if defined(USE_NSS)
because on other platforms (Mac and Windows) we use NSS
for SSL only and do not use NSS databases.

http://codereview.chromium.org/2838010/diff/9001/10002#newcode54
base/nss_util.h:54: if (lock)
The null check for nss_write_lock() seems to be the only
reason you have to use a scoped_ptr.  The use of scoped_ptr
requires an additional memory allocation (of AutoLock).

It is fine to omit the null check for nss_write_lock()
because Chromium doesn't check for memory allocation
failures (we abort the process in the memory allocator if
it runs out of memory).

http://codereview.chromium.org/2838010/diff/9001/10002#newcode65
base/nss_util.h:65: class AutoNSSWriteUnlock {
We don't need this auto-unlock class.

I believe you copied another auto-unlock class that is
used in the implementation of ConditionVariable.  In other
places there's no need for auto-unlock.

http://codereview.chromium.org/2838010/diff/9001/10003
File net/base/keygen_handler_unittest.cc (right):

http://codereview.chromium.org/2838010/diff/9001/10003#newcode49
net/base/keygen_handler_unittest.cc:49: void
AssertValidKeyAndSignedChallenge(const std::string& result,
Nit: rename this function AssertValidSignedPublicKeyAndChallenge.

[davidben, 2010-06-18 21:00:20]

http://codereview.chromium.org/2838010/diff/9001/10002
File base/nss_util.h (right):

http://codereview.chromium.org/2838010/diff/9001/10002#newcode54
base/nss_util.h:54: if (lock)
On 2010/06/18 19:04:09, wtc wrote:
> The null check for nss_write_lock() seems to be the only
> reason you have to use a scoped_ptr.  The use of scoped_ptr
> requires an additional memory allocation (of AutoLock).
> 
> It is fine to omit the null check for nss_write_lock()
> because Chromium doesn't check for memory allocation
> failures (we abort the process in the memory allocator if
> it runs out of memory).

I actually put the NULL check in there so that it'll be easy to disable the lock
when the bug is fixed. Though perhaps it'd be better to just grep and turn off
the lock on a per-call basis later?

[davidben, 2010-06-18 22:19:23]

http://codereview.chromium.org/2838010/diff/9001/10002
File base/nss_util.h (right):

http://codereview.chromium.org/2838010/diff/9001/10002#newcode46
base/nss_util.h:46: Lock* nss_write_lock();
On 2010/06/18 19:04:09, wtc wrote:
> This function should be named GetNSSWriteLock or
> GetNSSDatabaseWriteLock to follow the Style Guide.
> A name like nss_write_lock should only be used for a getter
> of a class data member.
> 
> You can put the declaration of this function inside
>     #if defined(USE_NSS)
> because on other platforms (Mac and Windows) we use NSS
> for SSL only and do not use NSS databases.

Done.

http://codereview.chromium.org/2838010/diff/9001/10002#newcode54
base/nss_util.h:54: if (lock)
On 2010/06/18 21:00:20, David Benjamin wrote:
> On 2010/06/18 19:04:09, wtc wrote:
> > The null check for nss_write_lock() seems to be the only
> > reason you have to use a scoped_ptr.  The use of scoped_ptr
> > requires an additional memory allocation (of AutoLock).
> > 
> > It is fine to omit the null check for nss_write_lock()
> > because Chromium doesn't check for memory allocation
> > failures (we abort the process in the memory allocator if
> > it runs out of memory).
> 
> I actually put the NULL check in there so that it'll be easy to disable the
lock
> when the bug is fixed. Though perhaps it'd be better to just grep and turn off
> the lock on a per-call basis later?

After discussion in person, AutoNSSWriteLock still checks for NULL but
implements it itself to avoid the allocation.

http://codereview.chromium.org/2838010/diff/9001/10002#newcode65
base/nss_util.h:65: class AutoNSSWriteUnlock {
On 2010/06/18 19:04:09, wtc wrote:
> We don't need this auto-unlock class.
> 
> I believe you copied another auto-unlock class that is
> used in the implementation of ConditionVariable.  In other
> places there's no need for auto-unlock.

Done.

http://codereview.chromium.org/2838010/diff/9001/10003
File net/base/keygen_handler_unittest.cc (right):

http://codereview.chromium.org/2838010/diff/9001/10003#newcode49
net/base/keygen_handler_unittest.cc:49: void
AssertValidKeyAndSignedChallenge(const std::string& result,
On 2010/06/18 19:04:09, wtc wrote:
> Nit: rename this function AssertValidSignedPublicKeyAndChallenge.

Done.

[wtc, 2010-06-19 00:20:45]

LGTM!  Good work.

Please reference BUG=148 in the Description of the CL.

http://codereview.chromium.org/2838010/diff/21001/22001
File base/nss_util.cc (right):

http://codereview.chromium.org/2838010/diff/21001/22001#newcode101
base/nss_util.cc:101: chromeos_user_logged_in_(false) {
Initialize write_lock_ to NULL because the constructor has early returns.
Better yet, define write_lock_ as a scoped_ptr<Lock>.  See comment below.

http://codereview.chromium.org/2838010/diff/21001/22001#newcode172
base/nss_util.cc:172: // When
https://bugzilla.mozilla.org/show_bug.cgi?id=564011 is fixed, we
Optional: you can add TODO(davidben) to this comment.

http://codereview.chromium.org/2838010/diff/21001/22001#newcode182
base/nss_util.cc:182: if (real_db_slot_) {
Delete write_lock_ in the destructor.  Alternatively, define write_lock_ as
a scoped_ptr<Lock>.

http://codereview.chromium.org/2838010/diff/21001/22002
File base/nss_util.h (right):

http://codereview.chromium.org/2838010/diff/21001/22002#newcode9
base/nss_util.h:9: #include "base/lock.h"
This can be put inside #if defined(USE_NSS) as well.

http://codereview.chromium.org/2838010/diff/21001/22002#newcode38
base/nss_util.h:38: // NSS has a bug which can cause a dead-lock or stall in
some cases when
Nits:
    dead-lock => deadlock
    key-pair => key pair
    workaround this => work around this
    in future => in the future

http://codereview.chromium.org/2838010/diff/21001/22002#newcode46
base/nss_util.h:46: Lock* GetNSSDatabaseWriteLock();
Let's shorten this to GetNSSWriteLock, so that it matches the
AutoNSSWriteLock class name below.

http://codereview.chromium.org/2838010/diff/21001/22002#newcode52
base/nss_util.h:52: AutoNSSWriteLock() : lock_(GetNSSDatabaseWriteLock()) {
I wonder if we should move the implementations of the constructor and
destructor to nss_util.cc, so that we just need a forward declaration of
class Lock in this file, and not include "base/lock.h".  This is recommended
by the Style Guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Header_File_De...

http://codereview.chromium.org/2838010/diff/21001/22003
File net/base/keygen_handler_unittest.cc (right):

http://codereview.chromium.org/2838010/diff/21001/22003#newcode10
net/base/keygen_handler_unittest.cc:10: #include <private/pprthred.h>  //
PR_DetachThread
Include this (a C system header) before #include <string> (a C++ header).
See the Style Guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

http://codereview.chromium.org/2838010/diff/21001/22004
File net/net.gyp (right):

http://codereview.chromium.org/2838010/diff/21001/22004#newcode772
net/net.gyp:772: '../build/linux/system.gyp:nss',
Just curious: why is this necessary?

http://codereview.chromium.org/2838010/diff/21001/22005
File net/third_party/mozilla_security_manager/nsKeygenHandler.cpp (right):

http://codereview.chromium.org/2838010/diff/21001/22005#newcode168
net/third_party/mozilla_security_manager/nsKeygenHandler.cpp:168:
base::AutoNSSWriteLock lock;
Nit: move the two LOG(INFO) statements to the outside of the critical section.

[davidben (use chromium.org), 2010-06-19 00:46:48]

http://codereview.chromium.org/2838010/diff/21001/22001
File base/nss_util.cc (right):

http://codereview.chromium.org/2838010/diff/21001/22001#newcode101
base/nss_util.cc:101: chromeos_user_logged_in_(false) {
On 2010/06/19 00:20:45, wtc wrote:
> Initialize write_lock_ to NULL because the constructor has early returns.
> Better yet, define write_lock_ as a scoped_ptr<Lock>.  See comment below.

Used a scoped_ptr. (I'd have to guard the thing with a USE_NSS which is really
annoying in an initialization list).

http://codereview.chromium.org/2838010/diff/21001/22001#newcode172
base/nss_util.cc:172: // When
https://bugzilla.mozilla.org/show_bug.cgi?id=564011 is fixed, we
On 2010/06/19 00:20:45, wtc wrote:
> Optional: you can add TODO(davidben) to this comment.

Done.

http://codereview.chromium.org/2838010/diff/21001/22001#newcode182
base/nss_util.cc:182: if (real_db_slot_) {
On 2010/06/19 00:20:45, wtc wrote:
> Delete write_lock_ in the destructor.  Alternatively, define write_lock_ as
> a scoped_ptr<Lock>.

Done.

http://codereview.chromium.org/2838010/diff/21001/22002
File base/nss_util.h (right):

http://codereview.chromium.org/2838010/diff/21001/22002#newcode9
base/nss_util.h:9: #include "base/lock.h"
On 2010/06/19 00:20:45, wtc wrote:
> This can be put inside #if defined(USE_NSS) as well.

Forward-declared.

http://codereview.chromium.org/2838010/diff/21001/22002#newcode38
base/nss_util.h:38: // NSS has a bug which can cause a dead-lock or stall in
some cases when
On 2010/06/19 00:20:45, wtc wrote:
> Nits:
>     dead-lock => deadlock
>     key-pair => key pair
>     workaround this => work around this
>     in future => in the future

Done.

http://codereview.chromium.org/2838010/diff/21001/22002#newcode46
base/nss_util.h:46: Lock* GetNSSDatabaseWriteLock();
On 2010/06/19 00:20:45, wtc wrote:
> Let's shorten this to GetNSSWriteLock, so that it matches the
> AutoNSSWriteLock class name below.

Done.

http://codereview.chromium.org/2838010/diff/21001/22002#newcode52
base/nss_util.h:52: AutoNSSWriteLock() : lock_(GetNSSDatabaseWriteLock()) {
On 2010/06/19 00:20:45, wtc wrote:
> I wonder if we should move the implementations of the constructor and
> destructor to nss_util.cc, so that we just need a forward declaration of
> class Lock in this file, and not include "base/lock.h".  This is recommended
> by the Style Guide:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Header_File_De...

Done.

http://codereview.chromium.org/2838010/diff/21001/22003
File net/base/keygen_handler_unittest.cc (right):

http://codereview.chromium.org/2838010/diff/21001/22003#newcode10
net/base/keygen_handler_unittest.cc:10: #include <private/pprthred.h>  //
PR_DetachThread
On 2010/06/19 00:20:45, wtc wrote:
> Include this (a C system header) before #include <string> (a C++ header).
> See the Style Guide:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

Done.

http://codereview.chromium.org/2838010/diff/21001/22004
File net/net.gyp (right):

http://codereview.chromium.org/2838010/diff/21001/22004#newcode772
net/net.gyp:772: '../build/linux/system.gyp:nss',
On 2010/06/19 00:20:45, wtc wrote:
> Just curious: why is this necessary?
For the PR_DetachThread call. The compiler appears to get upset otherwise.

http://codereview.chromium.org/2838010/diff/21001/22005
File net/third_party/mozilla_security_manager/nsKeygenHandler.cpp (right):

http://codereview.chromium.org/2838010/diff/21001/22005#newcode168
net/third_party/mozilla_security_manager/nsKeygenHandler.cpp:168:
base::AutoNSSWriteLock lock;
On 2010/06/19 00:20:45, wtc wrote:
> Nit: move the two LOG(INFO) statements to the outside of the critical section.

Done. (Oops.)

[davidben, 2010-06-19 00:49:13]

Adding mattm.

[wtc, 2010-06-21 22:40:53]

LGTM.

http://codereview.chromium.org/2838010/diff/29001/30001
File base/nss_util.cc (right):

http://codereview.chromium.org/2838010/diff/29001/30001#newcode24
base/nss_util.cc:24: #endif // defined(USE_NSS)
Two spaces before the comment.

http://codereview.chromium.org/2838010/diff/29001/30002
File base/nss_util.h (right):

http://codereview.chromium.org/2838010/diff/29001/30002#newcode12
base/nss_util.h:12: #endif // defined(USE_NSS)
Nit: two spaces before the comment.  See the Style Guide under
"Line Comments":
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Implementation...

[davidben, 2010-06-22 02:54:56]

(Haven't updated the CL because they're fairly trivial.)

Also made a CL for the relevant lock on Windows. (Tests already pass for me, but
it seems one of the functions isn't actually thread-safe.)
http://codereview.chromium.org/2828019/show

http://codereview.chromium.org/2838010/diff/29001/30001
File base/nss_util.cc (right):

http://codereview.chromium.org/2838010/diff/29001/30001#newcode24
base/nss_util.cc:24: #endif // defined(USE_NSS)
On 2010/06/21 22:40:53, wtc wrote:
> Two spaces before the comment.

Done.

http://codereview.chromium.org/2838010/diff/29001/30002
File base/nss_util.h (right):

http://codereview.chromium.org/2838010/diff/29001/30002#newcode12
base/nss_util.h:12: #endif // defined(USE_NSS)
On 2010/06/21 22:40:53, wtc wrote:
> Nit: two spaces before the comment.  See the Style Guide under
> "Line Comments":
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Implementation...
> 

Done.

http://codereview.chromium.org/2838010/diff/29001/30002#newcode62
base/nss_util.h:62: #endif // defined(USE_NSS)
Also added a space here.