Disallow Lock class support for recursive locking

Disallow recursive locking via the Lock class

Change contract so that Posix locks (which deadlock on
attempts by a single thread to acquire a mutex recursively)
and windows critical sections can both be used to implement
the Lock() cass, by disallowing recursive locking.


In DEBUG mode only, we watch for (now) illegal use of
recursive locking.

Also remove a pile of cruft that has built up in this file
as various folks have re-re-refactored and moved around
code.

Note that Window's condition variable implementation still
uses the AutoUnlock() helper class, but now the 
implementation (sans nested locking) is very trivial.  
Posix will probably use their own CV implementation.


r=mbelshe

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

[Dean McNamee, 2008-10-01 17:49:45]

A bunch of nits, but if you're not up for them, I can try to clear up the
comments some day.

I would definitely move out AutoUnlock, it's not something we should be needing
in general.  Could you do that?

Thanks

http://codereview.chromium.org/5630/diff/4/203
File base/lock.cc (right):

http://codereview.chromium.org/5630/diff/4/203#newcode20
Line 20: , recursion_count_shadow_(0),
This is not really chill with the style guide.  Just have only lock_() in the
initializer, and do all these as they were done before?

http://codereview.chromium.org/5630/diff/4/203#newcode55
Line 55: DCHECK(false);  // Outlaw recursive locking.
I would make the comment better, "Catch accidental double locking" or something

http://codereview.chromium.org/5630/diff/4/204
File base/lock.h (right):

http://codereview.chromium.org/5630/diff/4/204#newcode3
Line 3: // found in the LICENSE file.
I tend to like an "above the guard" summary.  Something like "The Lock class
backs to an underlying OS mutex.  It is non recursive (can only be locked by a
single thread once).  You should generally use the scoped AutoLock to lock and
unlock the lock from within a scope"... or something.

http://codereview.chromium.org/5630/diff/4/204#newcode14
Line 14: // acquire the lock a second time (while already holding it).
I would just move this up into the above the guard comment, something simple
saying we don't support recursive locks, and we will try to fire a DCHECK if we
see it.

http://codereview.chromium.org/5630/diff/4/204#newcode67
Line 67: // that is analogous to AutoLock.
I didn't understand this without reading the code.  You shouldn't say it's
analogous, just say that it will unlock the lock in the ctor, and lock it in the
dtor.

http://codereview.chromium.org/5630/diff/4/204#newcode77
Line 77: explicit AutoUnlock(Lock& lock) : lock_(&lock) {
btw, cpp lint doesn't like non-const reference.  I guess we should have had
AutoLock(&lock) all these days, but too late now.

I would move AutoUnlock into conditional_variable_win, we will not need it on
Posix.

http://codereview.chromium.org/5630/diff/4/204#newcode78
Line 78: // We require our caller have the lock.
english

[cpu_(ooo_6.6-7.5), 2008-10-01 18:32:43]

- The DCHECKs inside #ifndef NDEBUG() should be CHECK()s

- Do we have unittests? I know deanm has in the past lamented this. This would
be a great time to add them.

I still think is a better idea to keep the recursion count check enabled in
release for a few days. This will help catch issues on the buildbot and on the
distributed tests. I checked the sanbox code and is not using this lock class so
from that end it would not be a problem.

http://codereview.chromium.org/5630/diff/4/203
File base/lock.cc (right):

http://codereview.chromium.org/5630/diff/4/203#newcode28
Line 28: Lock::~Lock() {
Couple of comments here. What you want is a memory barrier not a lock so lets
use a memory barrier, we have atomicops.h now.

[Mike Belshe, 2008-10-01 19:48:46]

lgtm

[cpu_(ooo_6.6-7.5), 2008-10-02 00:14:56]

lgtm

[jar (doing other things), 2008-10-03 00:26:45]

Specific feedback comments on several of the suggestions (most of which are
"done").  Reviewers need not respond.

The patch is already approved, but these comments never got pushed... so I'm
just publishing them.

thanks again for the review,

Jim

http://codereview.chromium.org/5630/diff/4/203
File base/lock.cc (right):

http://codereview.chromium.org/5630/diff/4/203#newcode20
Line 20: , recursion_count_shadow_(0),
On 2008/10/01 17:49:45, Dean McNamee wrote:
> This is not really chill with the style guide.  Just have only lock_() in the
> initializer, and do all these as they were done before?

Seemed pretty clear.... but I went ahead and put two separate constructors (on
DEBUG, and one NDEBUG).

http://codereview.chromium.org/5630/diff/4/203#newcode28
Line 28: Lock::~Lock() {
On 2008/10/01 18:32:44, cpu wrote:
> Couple of comments here. What you want is a memory barrier not a lock so lets
> use a memory barrier, we have atomicops.h now.

Done.

http://codereview.chromium.org/5630/diff/4/203#newcode55
Line 55: DCHECK(false);  // Outlaw recursive locking.
On 2008/10/01 17:49:45, Dean McNamee wrote:
> I would make the comment better, "Catch accidental double locking" or
something

Done.

http://codereview.chromium.org/5630/diff/4/204
File base/lock.h (right):

http://codereview.chromium.org/5630/diff/4/204#newcode67
Line 67: // that is analogous to AutoLock.
On 2008/10/01 17:49:45, Dean McNamee wrote:
> I didn't understand this without reading the code.  You shouldn't say it's
> analogous, just say that it will unlock the lock in the ctor, and lock it in
the
> dtor.

Done.

http://codereview.chromium.org/5630/diff/4/204#newcode77
Line 77: explicit AutoUnlock(Lock& lock) : lock_(&lock) {
On 2008/10/01 17:49:45, Dean McNamee wrote:
> I would move AutoUnlock into conditional_variable_win, we will not need it on
Posix.

As per earlier comment: This is probably the right place for AutoUnlock for
encapsulation, and whole program optimization (and analagous link optimization
under gcc) should avoid pulling the (Posix dead) code into a binary

http://codereview.chromium.org/5630/diff/4/204#newcode78
Line 78: // We require our caller have the lock.
On 2008/10/01 17:49:45, Dean McNamee wrote:
> english

Done.

[jar (doing other things), 2008-10-08 23:48:25]

I removed the controversial DEBUG only lock or barrier from the destructor,
along with the DCHECK.  As pointed out by cpu, the value of the assertion was
less than the controversy it was generating.

The landing appears to break a unit test code that sky can resolve, and so I'm
going to land with the DCHECK commented out for him to fix and finalize on.  

The code appeared to otherwise pass base, net, and unit_test suites (in debug
and release) on my machine with the DCHECK enabled.