Add a locked version of CryptAcquireContext

Add a locked version of CryptAcquireContext

The function is not thread-safe when called with certain flags. This will be
useful when we move keygen onto a worker thread.

BUG=none
TEST=KeygenHandlerTest.SmokeTest (existing)

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

[wtc, 2010-06-23 00:52:23]

This looks good.  Thanks.  Please make the suggested changes below and
upload a new Patch Set.

http://codereview.chromium.org/2828019/diff/1/3
File base/crypto/crypt_util_win.cc (right):

http://codereview.chromium.org/2828019/diff/1/3#newcode7
base/crypto/crypt_util_win.cc:7: #include <objbase.h>
These two Windows system headers do not need to be included again.

http://codereview.chromium.org/2828019/diff/1/3#newcode18
base/crypto/crypt_util_win.cc:18: CryptUtilSingletonWin() {
Nit: indentation is off by 1.

Rename this class CAPIUtilSingleton to match the file name I suggested.

I remember we're supposed to make the singleton constructor private,
and declare some Singleton template class as a friend.  Please search in
the source tree for examples.

http://codereview.chromium.org/2828019/diff/1/3#newcode21
base/crypto/crypt_util_win.cc:21: static CryptUtilSingletonWin* get() {
The convention in Chromium for this kind of function seems to be
GetInstance().  Please search in the source tree for examples.

http://codereview.chromium.org/2828019/diff/1/3#newcode44
base/crypto/crypt_util_win.cc:44: DWORD provType,
Nit: if you use the Google naming convention, this parameter should be named
prov_type.  Otherwise you can use the original parameter names.

http://codereview.chromium.org/2828019/diff/1/4
File base/crypto/crypt_util_win.h (right):

http://codereview.chromium.org/2828019/diff/1/4#newcode5
base/crypto/crypt_util_win.h:5: #ifndef BASE_CRYPTO_CRYPT_UTIl_WIN_H_
Please name the new files capi_util.{h,cc}.

CAPI is the common abbreviaiton of Windows CryptoAPI.  The use of "capi"
in the file name implies "win".

Not using _win in the file names requires adding the two files to the sources!
section in the GYP file for non-Windows platform.

http://codereview.chromium.org/2828019/diff/1/4#newcode8
base/crypto/crypt_util_win.h:8: #include <objbase.h>
Include <windows.h> instead.  Although <objbase.h> is the smallest header
needed, it may not be the right header to include.

For example, the CreateFile page on MSDN says about the header "WinBase.h
(include Windows.h)":
http://msdn.microsoft.com/en-us/library/aa363858(VS.85).aspx

http://codereview.chromium.org/2828019/diff/1/4#newcode17
base/crypto/crypt_util_win.h:17: // See
http://msdn.microsoft.com/en-us/library/aa388149(v=VS.85).aspx
Nit: it is unknown whether these MSDN page URLs are static.  To deal with this
potential issue, you can include the title of the page and an excerpt of the
most important text in the comment.

[davidben, 2010-06-23 03:05:38]

http://codereview.chromium.org/2828019/diff/1/3
File base/crypto/crypt_util_win.cc (right):

http://codereview.chromium.org/2828019/diff/1/3#newcode7
base/crypto/crypt_util_win.cc:7: #include <objbase.h>
On 2010/06/23 00:52:23, wtc wrote:
> These two Windows system headers do not need to be included again.

Done.

http://codereview.chromium.org/2828019/diff/1/3#newcode18
base/crypto/crypt_util_win.cc:18: CryptUtilSingletonWin() {
On 2010/06/23 00:52:23, wtc wrote:
> Nit: indentation is off by 1.
> 
> Rename this class CAPIUtilSingleton to match the file name I suggested.
> 
> I remember we're supposed to make the singleton constructor private,
> and declare some Singleton template class as a friend.  Please search in
> the source tree for examples.

Done.

http://codereview.chromium.org/2828019/diff/1/3#newcode21
base/crypto/crypt_util_win.cc:21: static CryptUtilSingletonWin* get() {
On 2010/06/23 00:52:23, wtc wrote:
> The convention in Chromium for this kind of function seems to be
> GetInstance().  Please search in the source tree for examples.

Done.

http://codereview.chromium.org/2828019/diff/1/3#newcode44
base/crypto/crypt_util_win.cc:44: DWORD provType,
On 2010/06/23 00:52:23, wtc wrote:
> Nit: if you use the Google naming convention, this parameter should be named
> prov_type.  Otherwise you can use the original parameter names.

Done.

http://codereview.chromium.org/2828019/diff/1/4
File base/crypto/crypt_util_win.h (right):

http://codereview.chromium.org/2828019/diff/1/4#newcode5
base/crypto/crypt_util_win.h:5: #ifndef BASE_CRYPTO_CRYPT_UTIl_WIN_H_
On 2010/06/23 00:52:23, wtc wrote:
> Please name the new files capi_util.{h,cc}.
> 
> CAPI is the common abbreviaiton of Windows CryptoAPI.  The use of "capi"
> in the file name implies "win".
> 
> Not using _win in the file names requires adding the two files to the sources!
> section in the GYP file for non-Windows platform.

Done.

http://codereview.chromium.org/2828019/diff/1/4#newcode8
base/crypto/crypt_util_win.h:8: #include <objbase.h>
On 2010/06/23 00:52:23, wtc wrote:
> Include <windows.h> instead.  Although <objbase.h> is the smallest header
> needed, it may not be the right header to include.
> 
> For example, the CreateFile page on MSDN says about the header "WinBase.h
> (include Windows.h)":
> http://msdn.microsoft.com/en-us/library/aa363858%28VS.85%29.aspx

Done.

http://codereview.chromium.org/2828019/diff/1/4#newcode17
base/crypto/crypt_util_win.h:17: // See
http://msdn.microsoft.com/en-us/library/aa388149(v=VS.85).aspx
On 2010/06/23 00:52:23, wtc wrote:
> Nit: it is unknown whether these MSDN page URLs are static.  To deal with this
> potential issue, you can include the title of the page and an excerpt of the
> most important text in the comment.

Done.

[wtc, 2010-06-23 16:44:15]

LGTM!  I have two nits and a suggestion for future work below.

After fixing the nits, you can check this in without getting
a new review from me.

http://codereview.chromium.org/2828019/diff/10001/11001
File base/base.gypi (right):

http://codereview.chromium.org/2828019/diff/10001/11001#newcode359
base/base.gypi:359: 'crypto/capi_util.h',
Nit: list .cc before .h (in the directory listing order).

Make the same change to lines 487-488 below.

http://codereview.chromium.org/2828019/diff/10001/11002
File base/crypto/capi_util.cc (right):

http://codereview.chromium.org/2828019/diff/10001/11002#newcode29
base/crypto/capi_util.cc:29: CAPIUtilSingleton() {
Nit: consider writing a method with an empty body like this:
  CAPIUtilSingleton() {}

Your style is fine, too.

http://codereview.chromium.org/2828019/diff/10001/11003
File base/crypto/capi_util.h (right):

http://codereview.chromium.org/2828019/diff/10001/11003#newcode23
base/crypto/capi_util.h:23: BOOL CryptAcquireContextLocked(HCRYPTPROV* prov,
Suggestion for future work: also add CryptoAcquireContextUnlocked.
Change all of our calls to CryptAcquireContext to either our Locked or
Unlocked versions.  In Locked and Unlocked, CHECK or DCHECK the 'flags'
argument has or doesn't has the expected flags.

Then, in the future we can easily search for all instances of
CryptoAcquireContext in our source tree and audit the proper use of the
Locked version.