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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 10 months ago by David Benjamin
Modified:
2 years, 10 months ago
Reviewers:
wtc, mattm
CC:
chromium-reviews_chromium.org, 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) Lint Patch
M base/nss_util.h View 4 5 6 7 2 chunks +28 lines, -0 lines 3 comments ? errors Download
M base/nss_util.cc View 4 5 6 7 4 chunks +38 lines, -0 lines 2 comments ? errors Download
M net/base/keygen_handler_unittest.cc View 1 2 3 4 5 6 7 4 chunks +77 lines, -5 lines 0 comments ? errors Download
M net/net.gyp View 1 chunk +1 line, -0 lines 0 comments ? errors Download
M net/third_party/mozilla_security_manager/nsKeygenHandler.cpp View 3 4 5 6 7 5 chunks +14 lines, -8 lines 0 comments ? errors Download
Commit:

Messages

Total messages: 10
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 ...
3 years, 10 months ago #1
David Benjamin
Addressed comments and uploaded a series of changes. I'll look into adding locks as appropriate ...
3 years, 10 months ago #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 ...
3 years, 10 months ago #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 ...
3 years, 10 months ago #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 ...
3 years, 10 months ago #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 ...
3 years, 10 months ago #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 ...
3 years, 10 months ago #7
David Benjamin
Adding mattm.
3 years, 10 months ago #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. ...
3 years, 10 months ago #9
David Benjamin
3 years, 10 months ago #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 1280:2d3e6564b7b6