This CL is due the thread I have made on chromium-dev:...

This CL is due the thread I have post on chromium-dev:
http://groups.google.com/group/chromium-dev/browse_frm/thread/30af0b63b6adb245.

As suggested by Adam Barth I have insert the check on RefCountedBase class; 
Tests made on base_unittests, as foreseen by Dean McNamee, are all passing;

I have tried, just for test, to use RefCounted instead of the actual RefCountedThreadSafe
on MessagePump and as expected the tests are not passing anymore.

[mendola, 2008-10-04 13:48:18]

Adapted the original code to be compliant with google coding rules.
Renamed the macros in the hope that now the usage is clear.
Added example of usage on the header.

The macros are only defined if the build is for OS_POSIX and in debug mode, I
need to use the operations defined on atomicops.h in order to take rid of:

    __sync_bool_compare_and_swap
    __sync_fetch_and_xor

The class was used successfully on ref_counted.h.

[M-A Ruel, 2008-10-04 18:31:12]

I like the idea. I included a few style nits but I really want Adam to do the
actual "logic" review.

http://codereview.chromium.org/6258/diff/12/207
File base/ref_counted.cc (right):

http://codereview.chromium.org/6258/diff/12/207#newcode28
Line 28: BOOK_CRITICAL_SECTION(AddRelease); //Current thread books the critical
section "AddRelease" without release it
Spacing is as follow:
...);  // Current ...
But anyway, the line is >80 chars so you're better just to put the comment on
its own line.

http://codereview.chromium.org/6258/diff/12/208
File base/threadassert.h (right):

http://codereview.chromium.org/6258/diff/12/208#newcode5
Line 5: #ifndef THREADASSERT_H
We usually name the source name against the main class defined in the file.
Also, it'd probably be:
#ifndef THREAD_ASSERT_H_
...

http://codereview.chromium.org/6258/diff/12/208#newcode9
Line 9: //#include "base/logging.h"
I recommend against including this file in .h anyway.

http://codereview.chromium.org/6258/diff/12/208#newcode16
Line 16: // Example: Queue implementation non thread-safe but still usabe if
clients
usable

http://codereview.chromium.org/6258/diff/12/208#newcode56
Line 56: //          a "sharable" method (with external syncronization) and a
not
shareable

http://codereview.chromium.org/6258/diff/12/208#newcode59
Line 59: //          In this case 3 Critical sections has to be defined
In this case 3 critical sections have to be defined.

http://codereview.chromium.org/6258/diff/12/208#newcode84
Line 84: mutable base::ThreadCollisionWarner _##obj;
Remove the ; from the define so the user has to add it. It makes source parsing
simpler (like doxygen for instance).

http://codereview.chromium.org/6258/diff/12/208#newcode101
Line 101: public:
alignment is +1 for public/private and +2 for the members

http://codereview.chromium.org/6258/diff/12/208#newcode103
Line 103: :valid_thread_id_(0)
alignment for each contructors members is +4 and 1 space between the semi-column
and the member.
Also the opening bracket is beside, e.g.:
     : valid_thread_id_(0) {

http://codereview.chromium.org/6258/diff/12/208#newcode134
Line 134: void enter_self() {
I'd prefer to move the 3 private member functions in a .cc file. That would
simplify the include hierarchy.

http://codereview.chromium.org/6258/diff/12/208#newcode172
Line 172: 
remove one line

[mendola, 2008-10-04 22:00:14]

http://codereview.chromium.org/6258/diff/12/207
File base/ref_counted.cc (right):

http://codereview.chromium.org/6258/diff/12/207#newcode28
Line 28: BOOK_CRITICAL_SECTION(AddRelease); //Current thread books the critical
section "AddRelease" without release it
On 2008/10/04 18:31:13, M-A wrote:
> Spacing is as follow:
> ...);  // Current ...
> But anyway, the line is >80 chars so you're better just to put the comment on
> its own line.

Done.

http://codereview.chromium.org/6258/diff/12/208
File base/threadassert.h (right):

http://codereview.chromium.org/6258/diff/12/208#newcode5
Line 5: #ifndef THREADASSERT_H
On 2008/10/04 18:31:13, M-A wrote:
> We usually name the source name against the main class defined in the file.
> Also, it'd probably be:
> #ifndef THREAD_ASSERT_H_
> ...

Done.

http://codereview.chromium.org/6258/diff/12/208#newcode9
Line 9: //#include "base/logging.h"
On 2008/10/04 18:31:13, M-A wrote:
> I recommend against including this file in .h anyway.

Moved this on the .cc file

http://codereview.chromium.org/6258/diff/12/208#newcode16
Line 16: // Example: Queue implementation non thread-safe but still usabe if
clients
On 2008/10/04 18:31:13, M-A wrote:
> usable

Done.

http://codereview.chromium.org/6258/diff/12/208#newcode56
Line 56: //          a "sharable" method (with external syncronization) and a
not
On 2008/10/04 18:31:13, M-A wrote:
> shareable

Done.

http://codereview.chromium.org/6258/diff/12/208#newcode59
Line 59: //          In this case 3 Critical sections has to be defined
On 2008/10/04 18:31:13, M-A wrote:
> In this case 3 critical sections have to be defined.

Done.

http://codereview.chromium.org/6258/diff/12/208#newcode84
Line 84: mutable base::ThreadCollisionWarner _##obj;
On 2008/10/04 18:31:13, M-A wrote:
> Remove the ; from the define so the user has to add it. It makes source
parsing
> simpler (like doxygen for instance).

Done.

http://codereview.chromium.org/6258/diff/12/208#newcode101
Line 101: public:
On 2008/10/04 18:31:13, M-A wrote:
> alignment is +1 for public/private and +2 for the members

Done.

http://codereview.chromium.org/6258/diff/12/208#newcode103
Line 103: :valid_thread_id_(0)
On 2008/10/04 18:31:13, M-A wrote:
> alignment for each contructors members is +4 and 1 space between the
semi-column
> and the member.
> Also the opening bracket is beside, e.g.:
>      : valid_thread_id_(0) {

Done.

http://codereview.chromium.org/6258/diff/12/208#newcode134
Line 134: void enter_self() {
On 2008/10/04 18:31:13, M-A wrote:
> I'd prefer to move the 3 private member functions in a .cc file. That would
> simplify the include hierarchy.

Done.

http://codereview.chromium.org/6258/diff/12/208#newcode172
Line 172: 
On 2008/10/04 18:31:13, M-A wrote:
> remove one line

Done.

[abarth-chromium, 2008-10-05 18:39:56]

This looks promising.  There two things I'd like to see happen before we check
this in:

1) We should find at least one actual race condition to prove the concept.
2) We need unit tests for ThreadCollisionWarner.

http://codereview.chromium.org/6258/diff/215/15
File base/ref_counted.cc (right):

http://codereview.chromium.org/6258/diff/215/15#newcode8
Line 8: 
No space here.

http://codereview.chromium.org/6258/diff/215/15#newcode29
Line 29: BOOK_CRITICAL_SECTION(AddRelease);
I'm surprised this works.  I'm pretty sure we have examples where the first
AddRef is on one thread by the eventual Release is on another thread.  Did you
test this?  Seems like we'd need the SCOPED version....

http://codereview.chromium.org/6258/diff/215/14
File base/ref_counted.h (right):

http://codereview.chromium.org/6258/diff/215/14#newcode34
Line 34: DEFINE_CRITICAL_SECTION(AddRelease);
This should be above DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/6258/diff/215/16
File base/thread_collision_warner.cc (right):

http://codereview.chromium.org/6258/diff/215/16#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
Probably just (c) 2008, unless you've been working in this file for two years.

http://codereview.chromium.org/6258/diff/215/16#newcode5
Line 5: #include "base/thread_collision_warner.h"
Space below this line.

http://codereview.chromium.org/6258/diff/215/16#newcode10
Line 10: void ThreadCollisionWarner::enter_self() {
Non-trivial methods should be in camel case, as in EnterSelf()

http://codereview.chromium.org/6258/diff/215/17
File base/thread_collision_warner.h (right):

http://codereview.chromium.org/6258/diff/215/17#newcode73
Line 73: // TODO[gmendola] provide an implementation for OS_WIN
Use parenthesis instead of square brackets here.  For example, TODO(gmendola). 
Also you should file a bug for every TODO and reference the bug number in the
comment.

http://codereview.chromium.org/6258/diff/215/17#newcode79
Line 79: 
Do we have to use macros here?  Why can't we just declare this as a member
variable of the class?

Also, if we have to use macros, please start the macro with a "D" so we know
that it only does anything in DEBUG builds.

http://codereview.chromium.org/6258/diff/215/17#newcode81
Line 81: mutable base::ThreadCollisionWarner _##obj
Private member variables have a trailing underscore, not a leading underscore. 
Also, it would be cleaner if the clients just passed in the name of the member
variable they wanted to use so we're not randomly polluting the name space.  For
example, DEFINE_CRITICAL_SECTION(add_release_section_);

http://codereview.chromium.org/6258/diff/215/17#newcode101
Line 101: }
should be all on one line.

http://codereview.chromium.org/6258/diff/215/17#newcode104
Line 104: }
Should be all on one line:
~ThreadCollisionWarner() {}

http://codereview.chromium.org/6258/diff/215/17#newcode105
Line 105: 
please add a description of what this class does and why it never leaves the
critical section

http://codereview.chromium.org/6258/diff/215/17#newcode106
Line 106: class Check {
I don't understand this class at all.  Why does it hold a pointer to the
ThreadCollisionWarner?  It seems like you should just write:

DCHECK(add_release_section_.EnterSelf());

instead of this complicated MACRO / class business.

http://codereview.chromium.org/6258/diff/215/17#newcode108
Line 108: Check(ThreadCollisionWarner& a_warner)
We don't allow pass-by-non-const-ref.  Please use a pointer here.  Also, you can
just call the parameter "warner"

http://codereview.chromium.org/6258/diff/215/17#newcode117
Line 117: ThreadCollisionWarner& warner_;
presumably you want to DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/6258/diff/215/17#newcode128
Line 128: warner_.leave();
How do you know warner_ hasn't been deallocated at this point?  Seems like this
class needs to hold a reference to the warner_ somehow...

http://codereview.chromium.org/6258/diff/215/17#newcode132
Line 132: ThreadCollisionWarner& warner_;
Presumably you want to DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/6258/diff/215/17#newcode136
Line 136: 
Please document these methods.  Also, it's unclear how EnterSelf differs from
Enter from their names.  Maybe EnterRecursive?  That's not a great name either.

Why are these methods private?  Seems like they could be public.

http://codereview.chromium.org/6258/diff/215/17#newcode143
Line 143: volatile int valid_thread_id_;
Presumably you want to DISALLOW_COPY_AND_ASSIGN

[mendola, 2008-10-06 10:32:16]

I still haven't find any existing critical race, at least running
base_unittests, I'm able to have a DCHECK failing if I replace MessagePump base
class with RefCounted.

I will submit tests for ThreadCollisionWarner

http://codereview.chromium.org/6258/diff/215/15
File base/ref_counted.cc (right):

http://codereview.chromium.org/6258/diff/215/15#newcode8
Line 8: 
On 2008/10/05 18:39:56, abarth wrote:
> No space here.

Done.

http://codereview.chromium.org/6258/diff/215/15#newcode29
Line 29: BOOK_CRITICAL_SECTION(AddRelease);
On 2008/10/05 18:39:56, abarth wrote:
> I'm surprised this works.  I'm pretty sure we have examples where the first
> AddRef is on one thread by the eventual Release is on another thread.  Did you
> test this?  Seems like we'd need the SCOPED version....

All unit tests are passing, can you point me where the execution pattern you are
talking is in place?

http://codereview.chromium.org/6258/diff/215/14
File base/ref_counted.h (right):

http://codereview.chromium.org/6258/diff/215/14#newcode34
Line 34: DEFINE_CRITICAL_SECTION(AddRelease);
On 2008/10/05 18:39:56, abarth wrote:
> This should be above DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/6258/diff/215/16
File base/thread_collision_warner.cc (right):

http://codereview.chromium.org/6258/diff/215/16#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2008/10/05 18:39:56, abarth wrote:
> Probably just (c) 2008, unless you've been working in this file for two years.

Done.

http://codereview.chromium.org/6258/diff/215/16#newcode5
Line 5: #include "base/thread_collision_warner.h"
On 2008/10/05 18:39:56, abarth wrote:
> Space below this line.

Done.

http://codereview.chromium.org/6258/diff/215/16#newcode10
Line 10: void ThreadCollisionWarner::enter_self() {
On 2008/10/05 18:39:56, abarth wrote:
> Non-trivial methods should be in camel case, as in EnterSelf()

Done.

http://codereview.chromium.org/6258/diff/215/17
File base/thread_collision_warner.h (right):

http://codereview.chromium.org/6258/diff/215/17#newcode73
Line 73: // TODO[gmendola] provide an implementation for OS_WIN
On 2008/10/05 18:39:56, abarth wrote:
> Use parenthesis instead of square brackets here.  For example, TODO(gmendola).

> Also you should file a bug for every TODO and reference the bug number in the
> comment.

Done.

http://codereview.chromium.org/6258/diff/215/17#newcode79
Line 79: 
On 2008/10/05 18:39:56, abarth wrote:
> Do we have to use macros here?  Why can't we just declare this as a member
> variable of the class?
> 
> Also, if we have to use macros, please start the macro with a "D" so we know
> that it only does anything in DEBUG builds.

If we agree to use macros you mean something like:

DDEFINE_CRITICAL_SECTION
DBOOK_CRITICAL_SECTION
DSCOPED_BOOK_CRITICAL_SECTION 

?

http://codereview.chromium.org/6258/diff/215/17#newcode81
Line 81: mutable base::ThreadCollisionWarner _##obj
On 2008/10/05 18:39:56, abarth wrote:
> Private member variables have a trailing underscore, not a leading underscore.

> Also, it would be cleaner if the clients just passed in the name of the member
> variable they wanted to use so we're not randomly polluting the name space. 
For
> example, DEFINE_CRITICAL_SECTION(add_release_section_);
> 

Done.

http://codereview.chromium.org/6258/diff/215/17#newcode101
Line 101: }
On 2008/10/05 18:39:56, abarth wrote:
> should be all on one line.

Done.

http://codereview.chromium.org/6258/diff/215/17#newcode104
Line 104: }
On 2008/10/05 18:39:56, abarth wrote:
> Should be all on one line:
> ~ThreadCollisionWarner() {}

Done.

http://codereview.chromium.org/6258/diff/215/17#newcode105
Line 105: 
On 2008/10/05 18:39:56, abarth wrote:
> please add a description of what this class does and why it never leaves the
> critical section

Done.

http://codereview.chromium.org/6258/diff/215/17#newcode106
Line 106: class Check {
On 2008/10/05 18:39:56, abarth wrote:
> I don't understand this class at all.  Why does it hold a pointer to the
> ThreadCollisionWarner?  It seems like you should just write:
> 
> DCHECK(add_release_section_.EnterSelf());
> 
> instead of this complicated MACRO / class business.

We can take rid of Check put EnterSelf (or whatever name) public and let the
user to directly use it. See my reply to your comment for line 136

http://codereview.chromium.org/6258/diff/215/17#newcode108
Line 108: Check(ThreadCollisionWarner& a_warner)
On 2008/10/05 18:39:56, abarth wrote:
> We don't allow pass-by-non-const-ref.  Please use a pointer here.  Also, you
can
> just call the parameter "warner"

Done.

http://codereview.chromium.org/6258/diff/215/17#newcode117
Line 117: ThreadCollisionWarner& warner_;
On 2008/10/05 18:39:56, abarth wrote:
> presumably you want to DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/6258/diff/215/17#newcode128
Line 128: warner_.leave();
On 2008/10/05 18:39:56, abarth wrote:
> How do you know warner_ hasn't been deallocated at this point?  Seems like
this
> class needs to hold a reference to the warner_ somehow...

Sure the user at the moment is able to shot his own feet, the ScopedCheck is
meant to be used inside a method of the class holding a warner_, if a user calls
a method of a destroyed class (like in Phoenix Singleton) then yes we have a
problem. I can explore the reference somehow but I have the feeling that to warm
our hands we are going to fire our own feet.

http://codereview.chromium.org/6258/diff/215/17#newcode132
Line 132: ThreadCollisionWarner& warner_;
On 2008/10/05 18:39:56, abarth wrote:
> Presumably you want to DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/6258/diff/215/17#newcode136
Line 136: 
On 2008/10/05 18:39:56, abarth wrote:
> Please document these methods.  Also, it's unclear how EnterSelf differs from
> Enter from their names.  Maybe EnterRecursive?  That's not a great name
either.
> 
> Why are these methods private?  Seems like they could be public.

The idea was to use this methods only through the public Check and ScopedCheck
(via macros). If we get rid of this two macros then these methods can be made
public.

The usage will be something like:

class ExoticClass {
 public:
  ExoticClass() {
    ctor_dtor_.BookRecursionAllowed();
    ...
  }
  ~ExoticClass() { 
    ctor_dtor_.BookRecursionAllowed();
    ... 
  }
  
  void Shareable() { 
      ThreadCollisionWarner::ScopedBook book(shareable_section_);
      ... 
  }
	
  void NotSharable() { 
     not_shareable_section_.BookRecursionAllowed();
     ... 
  }
  ...
 private:
  DEFINE_CRITICAL_SECTION(ctor_dtor_);
  DEFINE_CRITICAL_SECTION(shareable_section_);
  DEFINE_CRITICAL_SECTION(not_shareable_section_);
};

is BookRecursionAllowed a good name ?

http://codereview.chromium.org/6258/diff/215/17#newcode143
Line 143: volatile int valid_thread_id_;
On 2008/10/05 18:39:56, abarth wrote:
> Presumably you want to DISALLOW_COPY_AND_ASSIGN

If I disallow copy and assign then this class can not be used on a copyable
class, may be is a good idea to implement a copy constructor that doesn't copy
the value in valid_thread_id_ but initializes it with 0 (a sort of reverse "Move
Constructor").

[mendola, 2008-10-06 16:10:53]

Added the concept of SCOPED_RECURSIVE_BOOK_CRITICAL_SECTION to cover this use
case:

class ClassNonThreadSafe {
public:
   void foo() { 
      SCOPED_RECURSIVE_BOOK_CRITICAL_SECTION(test_);
      ...
      bar(); 
      ...
   }
   void bar() { 
      SCOPED_RECURSIVE_BOOK_CRITICAL_SECTION(test_);
      ...
   } 
private:
   DEFINE_CRITICAL_SECTION(test_);
};

I used as proof of concept this new check to class WorkQueue inside
condition_variable_unitest.cc in where I saw the comment:
" // Worker threads only call the following methods.
  // They should use the lock to get exclusive access."

[M-A Ruel, 2008-10-06 16:24:58]

Adding jar for the condition variable unit test.

http://codereview.chromium.org/6258/diff/32/36
File base/thread_collision_warner.h (right):

http://codereview.chromium.org/6258/diff/32/36#newcode169
Line 169: // Counter to trace how many time a critical section was "pinned"
(when allowed)
80 chars

http://codereview.chromium.org/6258/diff/32/36#newcode171
Line 171: int counter_;
I think you should use Atomic32 here.

[mendola, 2008-10-06 16:51:55]

http://codereview.chromium.org/6258/diff/32/38
File base/condition_variable_unittest.cc (right):

http://codereview.chromium.org/6258/diff/32/38#newcode69
Line 69: // They should use the lock to get exclusive access.
use lock for all methods between line 70 -> 85 or only the next 4? task_count
returns the value of a variable changed in GetAnAssignment so it seems all
methods has to be synced. If that is the case (for all has to be used the lock)
then better move the method lock() in another section.

[cpu_(ooo_6.6-7.5), 2008-10-06 17:25:07]

http://codereview.chromium.org/6258/diff/32/36
File base/thread_collision_warner.h (right):

http://codereview.chromium.org/6258/diff/32/36#newcode26
Line 26: // private:
This macro has a terrible name, for casual reader it will seem that the class
has a critical section. I looked on the web and I am not the only one:

http://www.dylan-user.org/cgi-bin/viewvc.cgi/trunk/fundev/sources/lib/run-tim...

http://codereview.chromium.org/6258/diff/32/36#newcode30
Line 30: //
Actually, same comment for all the macro names :)

[abarth-chromium, 2008-10-07 08:20:34]

This patch is definitely an improvement, but it hasn't meet the requirements
outlined in the last review:

1) Unit tests for ThreadCollisionWarner.
2) Discovery of an actual race condition in Chromium.

http://codereview.chromium.org/6258/diff/215/15
File base/ref_counted.cc (right):

http://codereview.chromium.org/6258/diff/215/15#newcode29
Line 29: BOOK_CRITICAL_SECTION(AddRelease);
> All unit tests are passing, can you point me where the
> execution pattern you are talking is in place?

Have you tried surfing the web for a few minutes?  As I recall, the condition
fired all the time.

http://codereview.chromium.org/6258/diff/215/17
File base/thread_collision_warner.h (right):

http://codereview.chromium.org/6258/diff/215/17#newcode136
Line 136: 
We still need to document what these methods do and how they work.  Someone
modifying the code in the future will need to understand what they are supposed
to do.

http://codereview.chromium.org/6258/diff/32/38
File base/condition_variable_unittest.cc (right):

http://codereview.chromium.org/6258/diff/32/38#newcode19
Line 19: #include "base/thread_collision_warner.h"
This should be added to the above includes alphabetically.

http://codereview.chromium.org/6258/diff/32/38#newcode123
Line 123: DEFINE_CRITICAL_SECTION(WorkQueue);
This is a member variable.  It should be in unix_hacker_style_ like the above
member variables.

http://codereview.chromium.org/6258/diff/32/36
File base/thread_collision_warner.h (right):

http://codereview.chromium.org/6258/diff/32/36#newcode81
Line 81: mutable base::ThreadCollisionWarner obj##_
why even append the _ here?  Seems like we should just use the symbol the client
passed in.  Also, as cpu requested, we need something in the name to tell us
this is a debug-only feature.

http://codereview.chromium.org/6258/diff/32/36#newcode175
Line 175: // "bound" to any thread (to allow his usage on copyable classes at
least)?
You should just disable the copy constructor.  Its too confusing to think about
what happens when you make a copy of a critical section.

[mendola, 2008-10-07 15:47:01]

http://codereview.chromium.org/6258/diff/32/38
File base/condition_variable_unittest.cc (right):

http://codereview.chromium.org/6258/diff/32/38#newcode19
Line 19: #include "base/thread_collision_warner.h"
On 2008/10/07 08:20:35, abarth wrote:
> This should be added to the above includes alphabetically.

Done.

http://codereview.chromium.org/6258/diff/32/38#newcode123
Line 123: DEFINE_CRITICAL_SECTION(WorkQueue);
On 2008/10/07 08:20:35, abarth wrote:
> This is a member variable.  It should be in unix_hacker_style_ like the above
> member variables.

Done.

http://codereview.chromium.org/6258/diff/32/36
File base/thread_collision_warner.h (right):

http://codereview.chromium.org/6258/diff/32/36#newcode26
Line 26: // private:
On 2008/10/06 17:25:07, cpu wrote:
> This macro has a terrible name, for casual reader it will seem that the class
> has a critical section. I looked on the web and I am not the only one:
> 
>
http://www.dylan-user.org/cgi-bin/viewvc.cgi/trunk/fundev/sources/lib/run-tim...
> 

I will make a post on chromium-dev to discuss about the right name.

http://codereview.chromium.org/6258/diff/32/36#newcode30
Line 30: //
On 2008/10/06 17:25:07, cpu wrote:
> Actually, same comment for all the macro names :)

I will make a post on chromium-dev to discuss about the right name.

http://codereview.chromium.org/6258/diff/32/36#newcode81
Line 81: mutable base::ThreadCollisionWarner obj##_
On 2008/10/07 08:20:35, abarth wrote:
> why even append the _ here?  Seems like we should just use the symbol the
client
> passed in.  Also, as cpu requested, we need something in the name to tell us
> this is a debug-only feature.

Removed the "_" and now macros are D_XXXXXXXX to point the fact that is defined
only in debug

http://codereview.chromium.org/6258/diff/32/36#newcode169
Line 169: // Counter to trace how many time a critical section was "pinned"
(when allowed)
On 2008/10/06 16:24:58, M-A wrote:
> 80 chars

Done.

http://codereview.chromium.org/6258/diff/32/36#newcode171
Line 171: int counter_;
On 2008/10/06 16:24:58, M-A wrote:
> I think you should use Atomic32 here.

Done.

http://codereview.chromium.org/6258/diff/32/36#newcode175
Line 175: // "bound" to any thread (to allow his usage on copyable classes at
least)?
On 2008/10/07 08:20:35, abarth wrote:
> You should just disable the copy constructor.  Its too confusing to think
about
> what happens when you make a copy of a critical section.

Done.

[mendola, 2008-10-07 15:49:31]

* Renamed macros in D_XXXXXXXXXX
* Used Atomic32 instead of int
* New example in thread_collision_warner.h
* Find a bug in the actual code discovered using
  ThreadCollisionWarner, I think I found one.
  In unit_test condition_variable_unitttest.cc, at line
  640 is missing a lock before to call work_is_available()

Open issues:
  * It seems that macros name are not the best choice ever,
    I would like some proposal about those names :D
  * I'm still using gcc internal functions as atomic 
    operations, need to figure out the two equivalent on 
    atomicops.h
  * Unit test is missing. Is there a way to check on a test
    that a DCHECK "fired"?

[jar (doing other things), 2008-10-08 21:53:57]

Corrections and changes below.

I'm not (personally) endorsing the macro, but simply correcting its use, and the
conclusion in this one file. I don't believe there was a bug detected via the
macro in the code.

If you relocate the declarations (you realized the comment did not apply to
lock(), but it wasn't clear that it didn't apply to getting the CV), please
reorder any constructor initializers appropriately.

Thanks, and sorry to have caused confusion,

Jim

http://codereview.chromium.org/6258/diff/52/58
File base/condition_variable_unittest.cc (right):

http://codereview.chromium.org/6258/diff/52/58#newcode82
Line 82: ConditionVariable* work_is_available();
Since the comment on line 68 was confusing, you could relocate lock() and
work_is_available() outside (below) this block, and indicate that:

"Worker threads can call them, but don't need to have a lock."

http://codereview.chromium.org/6258/diff/52/58#newcode533
Line 533: D_SCOPED_RECURSIVE_BOOK_CRITICAL_SECTION(locked_methods_);
This accesses an effectively constant value (not variable across the lifetimes
of the test threads).  As a result, it is not necessary to lock anything, or be
on a specific thread.  

Please remove the macro call (check).

http://codereview.chromium.org/6258/diff/52/58#newcode655
Line 655: AutoLock auto_lock(lock_);
Locking is not needed in front of the call to signal.

Please revert changes to these 3 lines.

[mendola, 2008-10-09 15:50:34]

* Unittest empty added, I'm still thinking about how to test it, I have to try
out a few ideas
* Still unting for an existing bug, I extended the check to scoped_ptr and
stack_container, "fortunately" all tests are still passing.
* Reverted the lock I have add to conditional_variable_unittest.cc and fixed the
comment about to the methods to call in exclusive access

http://codereview.chromium.org/6258/diff/52/58
File base/condition_variable_unittest.cc (right):

http://codereview.chromium.org/6258/diff/52/58#newcode82
Line 82: ConditionVariable* work_is_available();
On 2008/10/08 21:53:57, jar wrote:
> Since the comment on line 68 was confusing, you could relocate lock() and
> work_is_available() outside (below) this block, and indicate that:
> 
> "Worker threads can call them, but don't need to have a lock."

Done.

http://codereview.chromium.org/6258/diff/52/58#newcode533
Line 533: D_SCOPED_RECURSIVE_BOOK_CRITICAL_SECTION(locked_methods_);
On 2008/10/08 21:53:57, jar wrote:
> This accesses an effectively constant value (not variable across the lifetimes
> of the test threads).  As a result, it is not necessary to lock anything, or
be
> on a specific thread.  
> 
> Please remove the macro call (check).

For same reason I'll do the same to all_threads_have_ids and to no_more_tasks,
all those methods return a ConditionVariable that is meant to be used from
multiple threads.

http://codereview.chromium.org/6258/diff/52/58#newcode655
Line 655: AutoLock auto_lock(lock_);
On 2008/10/08 21:53:57, jar wrote:
> Locking is not needed in front of the call to signal.
> 
> Please revert changes to these 3 lines.
> 
> 
> 

Done.

[M-A Ruel, 2008-10-14 19:36:20]

http://codereview.chromium.org/6258/diff/62/248
File base/thread_collision_warner.cc (right):

http://codereview.chromium.org/6258/diff/62/248#newcode15
Line 15: if (! __sync_bool_compare_and_swap(&valid_thread_id_,
This still can't be checked-in as is. :/

http://codereview.chromium.org/6258/diff/62/248#newcode50
Line 50: } //namespace base
}  // namespace base

http://codereview.chromium.org/6258/diff/62/248#newcode52
Line 52: 
too many empty lines

http://codereview.chromium.org/6258/diff/62/249
File base/thread_collision_warner.h (right):

http://codereview.chromium.org/6258/diff/62/249#newcode29
Line 29: //    D_DEFINE_CRITICAL_SECTION(push_pop_)
Forgot ';' ?
Also for the followings.

http://codereview.chromium.org/6258/diff/62/247
File base/thread_collision_warner_unittest.cc (right):

http://codereview.chromium.org/6258/diff/62/247#newcode14
Line 14: EXPECT_TRUE(true);
eh :)

[mendola, 2008-10-15 09:21:33]

Removed the usage of GCC builtin, now it should work on windows too. 

Still missing:
* TestUnit, I need to not fire the DCHECK but something else during the test
* Bug spotted using this class

http://codereview.chromium.org/6258/diff/62/248
File base/thread_collision_warner.cc (right):

http://codereview.chromium.org/6258/diff/62/248#newcode15
Line 15: if (! __sync_bool_compare_and_swap(&valid_thread_id_,
On 2008/10/14 19:36:20, M-A wrote:
> This still can't be checked-in as is. :/

Done.

http://codereview.chromium.org/6258/diff/62/248#newcode50
Line 50: } //namespace base
On 2008/10/14 19:36:20, M-A wrote:
> }  // namespace base

Done.

http://codereview.chromium.org/6258/diff/62/248#newcode52
Line 52: 
On 2008/10/14 19:36:20, M-A wrote:
> too many empty lines

Done.

http://codereview.chromium.org/6258/diff/62/249
File base/thread_collision_warner.h (right):

http://codereview.chromium.org/6258/diff/62/249#newcode29
Line 29: //    D_DEFINE_CRITICAL_SECTION(push_pop_)
On 2008/10/14 19:36:20, M-A wrote:
> Forgot ';' ?
> Also for the followings.

Done.

http://codereview.chromium.org/6258/diff/62/247
File base/thread_collision_warner_unittest.cc (right):

http://codereview.chromium.org/6258/diff/62/247#newcode14
Line 14: EXPECT_TRUE(true);
On 2008/10/14 19:36:20, M-A wrote:
> eh :)

Solid test :)

[M-A Ruel, 2008-10-15 13:45:23]

http://codereview.chromium.org/6258/diff/401/79
File base/thread_collision_warner.cc (right):

http://codereview.chromium.org/6258/diff/401/79#newcode28
Line 28: current_thread_id,
alignment

http://codereview.chromium.org/6258/diff/401/79#newcode59
Line 59: } //                          namespace base
too many spaces.

http://codereview.chromium.org/6258/diff/401/80
File base/thread_collision_warner.h (right):

http://codereview.chromium.org/6258/diff/401/80#newcode23
Line 23: // public:
alignment, throughout the file.

http://codereview.chromium.org/6258/diff/401/80#newcode207
Line 207: } //namespace base
spaces

http://codereview.chromium.org/6258/diff/401/78
File base/thread_collision_warner_unittest.cc (right):

http://codereview.chromium.org/6258/diff/401/78#newcode12
Line 12: TEST(ThreadCollisionTest, Test1) {
you'll still have to instantiate something.

[M-A Ruel, 2008-10-15 16:15:01]

First, you need to modify the xcodeproj and the vcproj. The patch is at
http://pastebin.com/m354cfbd1 so you should add theses changes to your patch.


Second, during the release windows build, compilation errors occurred:
11>os_file_win.cc
11>C:\b\slave\try-win32-1\build\src\base/atomicops_internals_x86_msvc.h(18) :
error C2065: 'LONG' : undeclared identifier
11>C:\b\slave\try-win32-1\build\src\base/atomicops_internals_x86_msvc.h(18) :
error C2146: syntax error : missing ';' before identifier 'result'
11>C:\b\slave\try-win32-1\build\src\base/atomicops_internals_x86_msvc.h(18) :
error C2065: 'result' : undeclared identifier
11>C:\b\slave\try-win32-1\build\src\base/atomicops_internals_x86_msvc.h(19) :
error C4430: missing type specifier - int assumed. Note: C++ does not support
default-int
11>C:\b\slave\try-win32-1\build\src\base/atomicops_internals_x86_msvc.h(19) :
error C2061: syntax error : identifier 'LONG'
11>C:\b\slave\try-win32-1\build\src\base/atomicops_internals_x86_msvc.h(18) :
error C3861: 'InterlockedCompareExchange': identifier not found
11>C:\b\slave\try-win32-1\build\src\base/atomicops_internals_x86_msvc.h(27) :
error C2146: syntax error : missing ';' before identifier 'result'
11>C:\b\slave\try-win32-1\build\src\base/atomicops_internals_x86_msvc.h(28) :
error C4430: missing type specifier - int assumed. Note: C++ does not support
default-int
11>C:\b\slave\try-win32-1\build\src\base/atomicops_internals_x86_msvc.h(28) :
error C2061: syntax error : identifier 'LONG'
11>C:\b\slave\try-win32-1\build\src\base/atomicops_internals_x86_msvc.h(27) :
error C3861: 'InterlockedExchange': identifier not found
11>C:\b\slave\try-win32-1\build\src\base/atomicops_internals_x86_msvc.h(36) :
error C4430: missing type specifier - int assumed. Note: C++ does not support
default-int
11>C:\b\slave\try-win32-1\build\src\base/atomicops_internals_x86_msvc.h(36) :
error C2061: syntax error : identifier 'LONG'
11>C:\b\slave\try-win32-1\build\src\base/atomicops_internals_x86_msvc.h(42) :
fatal error C1903: unable to recover from previous error(s); stopping
compilation

so it seems like an include is missing somewhere. The build is not finished so
I'll post another reply if there is any other compilation failure.


Finally, I did a try run. Running the test did fire the assert a *lot of times*.
This needs to be investigated.
ipc_tests: http://pastebin.com/m6c5390b9
unit_tests: http://pastebin.com/m1956170d
ui_tests: http://pastebin.com/m8ac7a30

So it seems like this is useful!

[M-A Ruel, 2008-10-15 17:35:21]

35>condition_variable_unittest.cc
35>..\condition_variable_unittest.cc(481) : error C2065: 'locked_methods_' :
undeclared identifier
35>..\condition_variable_unittest.cc(481) : error C3861:
'D_SCOPED_RECURSIVE_BOOK_CRITICAL_SECTION': identifier not found
35>..\condition_variable_unittest.cc(487) : error C3861:
'D_SCOPED_RECURSIVE_BOOK_CRITICAL_SECTION': identifier not found
35>..\condition_variable_unittest.cc(492) : error C3861:
'D_SCOPED_RECURSIVE_BOOK_CRITICAL_SECTION': identifier not found
35>..\condition_variable_unittest.cc(502) : error C3861:
'D_SCOPED_RECURSIVE_BOOK_CRITICAL_SECTION': identifier not found
35>..\condition_variable_unittest.cc(507) : error C3861:
'D_SCOPED_RECURSIVE_BOOK_CRITICAL_SECTION': identifier not found
35>..\condition_variable_unittest.cc(512) : error C3861:
'D_SCOPED_RECURSIVE_BOOK_CRITICAL_SECTION': identifier not found
35>..\condition_variable_unittest.cc(517) : error C3861:
'D_SCOPED_RECURSIVE_BOOK_CRITICAL_SECTION': identifier not found
35>..\condition_variable_unittest.cc(522) : error C3861:
'D_SCOPED_RECURSIVE_BOOK_CRITICAL_SECTION': identifier not found
35>..\condition_variable_unittest.cc(527) : error C3861:
'D_SCOPED_RECURSIVE_BOOK_CRITICAL_SECTION': identifier not found

[mendola, 2008-10-17 09:59:30]

http://codereview.chromium.org/6258/diff/401/79
File base/thread_collision_warner.cc (right):

http://codereview.chromium.org/6258/diff/401/79#newcode28
Line 28: current_thread_id,
On 2008/10/15 13:45:23, M-A wrote:
> alignment

Done.

http://codereview.chromium.org/6258/diff/401/80
File base/thread_collision_warner.h (right):

http://codereview.chromium.org/6258/diff/401/80#newcode23
Line 23: // public:
On 2008/10/15 13:45:23, M-A wrote:
> alignment, throughout the file.

Done.

http://codereview.chromium.org/6258/diff/401/80#newcode207
Line 207: } //namespace base
On 2008/10/15 13:45:23, M-A wrote:
> spaces

Done.

http://codereview.chromium.org/6258/diff/401/78
File base/thread_collision_warner_unittest.cc (right):

http://codereview.chromium.org/6258/diff/401/78#newcode12
Line 12: TEST(ThreadCollisionTest, Test1) {
On 2008/10/15 13:45:23, M-A wrote:
> you'll still have to instantiate something.

I will as soon I find a way to test it.

[mendola, 2008-10-18 01:27:47]

Applied patch for win and mac suggested by maruel on:
http://pastebin.com/m354cfbd1. 
Rework on maruel comments

[mendola, 2008-10-18 17:58:03]

First test implementation. Still missing tests with multiple threads.

[M-A Ruel, 2008-10-18 19:11:16]

90% of the comments are style nits. As soon as I can (within the next 3 days),
I'll debug the failure on Windows to confirm if they spurious or real.

http://codereview.chromium.org/6258/diff/105/442
File base/thread_collision_warner.h (right):

http://codereview.chromium.org/6258/diff/105/442#newcode125
Line 125: struct AsserterBase {
virtual ~AssertBase() { }
! :)

http://codereview.chromium.org/6258/diff/105/442#newcode135
Line 135: 
no empty line

http://codereview.chromium.org/6258/diff/105/442#newcode199
Line 199: private:
alignment

http://codereview.chromium.org/6258/diff/105/442#newcode200
Line 200: 
no empty line

http://codereview.chromium.org/6258/diff/105/440
File base/thread_collision_warner_unittest.cc (right):

http://codereview.chromium.org/6258/diff/105/440#newcode13
Line 13: class AssertReporter : public base::AsserterBase {
Don't indent in namespace

http://codereview.chromium.org/6258/diff/105/440#newcode28
Line 28: }
}  // namespace

http://codereview.chromium.org/6258/diff/105/440#newcode52
Line 52: { // Pin section
{  // Pin section.

http://codereview.chromium.org/6258/diff/105/440#newcode58
Line 58: } //Unpin section
}  // Unpin section.

[mendola, 2008-10-19 02:22:42]

Now Multi Thread tests are provided.

TODO
* Work on comments
* Choose a better name for macros, I feel almost noone like the actual names
* Spot a bug thanks the use of this class, it seems Maruel has something to
investigate

http://codereview.chromium.org/6258/diff/105/442
File base/thread_collision_warner.h (right):

http://codereview.chromium.org/6258/diff/105/442#newcode125
Line 125: struct AsserterBase {
On 2008/10/18 19:11:16, M-A wrote:
> virtual ~AssertBase() { }
> ! :)

ops!

http://codereview.chromium.org/6258/diff/105/442#newcode135
Line 135: 
On 2008/10/18 19:11:16, M-A wrote:
> no empty line

Done.

http://codereview.chromium.org/6258/diff/105/442#newcode199
Line 199: private:
On 2008/10/18 19:11:16, M-A wrote:
> alignment

Done.

http://codereview.chromium.org/6258/diff/105/442#newcode200
Line 200: 
On 2008/10/18 19:11:16, M-A wrote:
> no empty line

Done.

http://codereview.chromium.org/6258/diff/105/440
File base/thread_collision_warner_unittest.cc (right):

http://codereview.chromium.org/6258/diff/105/440#newcode13
Line 13: class AssertReporter : public base::AsserterBase {
On 2008/10/18 19:11:16, M-A wrote:
> Don't indent in namespace

Done.

http://codereview.chromium.org/6258/diff/105/440#newcode28
Line 28: }
On 2008/10/18 19:11:16, M-A wrote:
> }  // namespace

Done.

http://codereview.chromium.org/6258/diff/105/440#newcode52
Line 52: { // Pin section
On 2008/10/18 19:11:16, M-A wrote:
> {  // Pin section.

Done.

http://codereview.chromium.org/6258/diff/105/440#newcode58
Line 58: } //Unpin section
On 2008/10/18 19:11:16, M-A wrote:
> }  // Unpin section.

Done.

[M-A Ruel, 2008-10-20 19:54:21]

The patch looks good to me but since I'm getting a lot of asserts, I'd check-in
everything except ref_counted.h, ref_counted.cc, scoped_ptr.h and
stack_container.h.

The rationale for ref_counter.* is:
- There's a lot of classes to fix. I'd rather have the first part checked-in and
then have an "enable and fix" secondary patch.
- The scoped_ptr will probably never get in due to this example use: one thread
creates a thread aware object and sends it to another thread. If the class has a
scoped_ptr<> member, it'll break on first use.
- I'm just being precautious about stack_container.h due to the 2 first issues.

Are you fine with this, e.g. checking-in everything except the four files and
then do a second round up with the related fixes.

http://codereview.chromium.org/6258/diff/113/118
File base/thread_collision_warner_unittest.cc (right):

http://codereview.chromium.org/6258/diff/113/118#newcode18
Line 18: public:
Alignment.
There's a few other ones.

http://codereview.chromium.org/6258/diff/113/118#newcode57
Line 57: { // Pin section.
{  // Pin section.
There's a few other one.

[mendola, 2008-10-24 17:02:30]

Removed spaces on comments, I hope those were last.

[cpu_(ooo_6.6-7.5), 2009-02-24 20:13:51]

Can we delete this issue?

On 2008/10/24 17:02:30, mendola wrote:
> Removed spaces on comments, I hope those were last.

[mendola, 2009-03-01 11:07:28]

On 2009/02/24 20:13:51, cpu wrote:
> Can we delete this issue?
> 
> On 2008/10/24 17:02:30, mendola wrote:
> > Removed spaces on comments, I hope those were last.

Sure, I think maruel already created another issue with those classes.