Chromium Code Reviews
Help | Chromium Project | Sign in
(104)

Issue 2838010: Add a unit test to check KeygenHandler's thread-safety (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years ago by David Benjamin
Modified:
4 years ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, PaweĊ‚ Hajdan Jr., brettw-cc_chromium.org
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Address Wan-teh's comments from 2854007 #

Total comments: 2

Patch Set 3 : Rebase to trunk #

Patch Set 4 : Add global NSS write lock #

Total comments: 9

Patch Set 5 : Detach worker thread with PR_DetachThread #

Patch Set 6 : Address wtc's comments #

Patch Set 7 : Don't initialize the write lock in initialization list; breaks non-USE_NSS builds #

Total comments: 20

Patch Set 8 : Another revision #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+158 lines, -13 lines) Patch
M base/nss_util.h View 4 5 6 7 2 chunks +28 lines, -0 lines 3 comments Download
M base/nss_util.cc View 4 5 6 7 4 chunks +38 lines, -0 lines 2 comments Download
M net/base/keygen_handler_unittest.cc View 1 2 3 4 5 6 7 4 chunks +77 lines, -5 lines 0 comments Download
M net/net.gyp View 1 chunk +1 line, -0 lines 0 comments Download
M net/third_party/mozilla_security_manager/nsKeygenHandler.cpp View 3 4 5 6 7 5 chunks +14 lines, -8 lines 0 comments Download
Commit: CQ not working?

Messages

Total messages: 10 (0 generated)
wtc
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 ...
5 years ago (2010-06-17 18:34:32 UTC) #1
David Benjamin
Addressed comments and uploaded a series of changes. I'll look into adding locks as appropriate ...
5 years ago (2010-06-18 18:53:24 UTC) #2
wtc
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 ...
5 years ago (2010-06-18 19:04:09 UTC) #3
David Benjamin
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 ...
5 years ago (2010-06-18 21:00:20 UTC) #4
David Benjamin
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 ...
5 years ago (2010-06-18 22:19:23 UTC) #5
wtc
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 ...
5 years ago (2010-06-19 00:20:45 UTC) #6
davidben (use chromium.org)
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 ...
5 years ago (2010-06-19 00:46:48 UTC) #7
David Benjamin
Adding mattm.
5 years ago (2010-06-19 00:49:13 UTC) #8
wtc
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. ...
5 years ago (2010-06-21 22:40:53 UTC) #9
David Benjamin
5 years ago (2010-06-22 02:54:56 UTC) #10
(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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1f9106d