base: Enable lock dchecks with dchecks_always_on

base: Enable lock dchecks with dchecks_always_on

Enable DCHECKs in Lock and ConditionVariable when compiled
with dchecks_always_on.

Note the correctness of DCHECKs relies on DCHECKs enabled
in both Lock and ConditionVariable. This CL keeps the
condition to enable DCHECKs in ConditionVariable the
same as Lock, to avoid any issues here.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284534

[boliu, 2014-07-21 16:33:33]

ptal, thanks!

[willchan no longer on Chromium, 2014-07-21 18:13:27]

The change LGTM, although the changelist description confuses me. Can you point
out what internal debug counters are being accessed and what the problem is?
Better yet, can you remove it from the changelist description and file a bug
instead? It's confusing to read it in the changelist description when this CL
gets committed, since readers won't have the context about what's happening 1
year from now.

[boliu, 2014-07-21 18:23:11]

Updated the CL description. Hope it's clearer?

https://codereview.chromium.org/403803004/diff/80001/base/synchronization/lock.h
File base/synchronization/lock.h (right):

https://codereview.chromium.org/403803004/diff/80001/base/synchronization/loc...
base/synchronization/lock.h:65: friend class ConditionVariable;
See comments on these friend statements about the "internal debug counter"
thing.

ConditionVariable need to update owning_thread_ref_ to make the DCHECKs on
owning_thread_ref_ hold, so the debug condition between Lock and
ConditionVariable must match.

Took me awhile to figure this out when I did not update ConditionVariable and
DCHECKs started failing that seemed to indidated lock stopped working...

[willchan no longer on Chromium, 2014-07-21 18:51:43]

OIC. OK, since you're not fixing it yet, can you clarify in the CL description
that it'll be fixed in a separate CL? It's loosely implied here, but it'd be
great to be explicit about it.

[willchan no longer on Chromium, 2014-07-21 18:51:53]

And yeah, LGTM, so feel free to commit. Cheers.

[boliu, 2014-07-21 18:55:52]

On 2014/07/21 18:51:43, willchan wrote:
> OIC. OK, since you're not fixing it yet, can you clarify in the CL description
> that it'll be fixed in a separate CL? It's loosely implied here, but it'd be
> great to be explicit about it.

Slightly confused. You are saying ConditionVariable shouldn't be accessing Lock
private members and that should be fixed?

Also s/fixed/updated/ in description.

[willchan no longer on Chromium, 2014-07-21 18:59:47]

On 2014/07/21 18:55:52, boliu wrote:
> On 2014/07/21 18:51:43, willchan wrote:
> > OIC. OK, since you're not fixing it yet, can you clarify in the CL
description
> > that it'll be fixed in a separate CL? It's loosely implied here, but it'd be
> > great to be explicit about it.
> 
> Slightly confused. You are saying ConditionVariable shouldn't be accessing
Lock
> private members and that should be fixed?
> 
> Also s/fixed/updated/ in description.

Sorry. I misunderstood. I see that you actually fixed it here. This is the
confusing thing about your changelist description. You write "ConditionVariable
also needs to be updated as well". That implies that you have not *yet* fixed
it. But I see now that your description of updating the |owning_thread_ref_|
actually already happens, because you're calling CheckHeldAndUnmark(). I didn't
notice that that function is what updates that internal debug variable.

Please update your changelist description to make it explicit that you have
actually already fixed the bug that you described. The use of future tense is
problematic here.

[boliu, 2014-07-21 20:28:44]

On 2014/07/21 18:59:47, willchan wrote:
> On 2014/07/21 18:55:52, boliu wrote:
> > On 2014/07/21 18:51:43, willchan wrote:
> > > OIC. OK, since you're not fixing it yet, can you clarify in the CL
> description
> > > that it'll be fixed in a separate CL? It's loosely implied here, but it'd
be
> > > great to be explicit about it.
> > 
> > Slightly confused. You are saying ConditionVariable shouldn't be accessing
> Lock
> > private members and that should be fixed?
> > 
> > Also s/fixed/updated/ in description.
> 
> Sorry. I misunderstood. I see that you actually fixed it here. This is the
> confusing thing about your changelist description. You write
"ConditionVariable
> also needs to be updated as well". That implies that you have not *yet* fixed
> it. But I see now that your description of updating the |owning_thread_ref_|
> actually already happens, because you're calling CheckHeldAndUnmark(). I
didn't
> notice that that function is what updates that internal debug variable.
> 
> Please update your changelist description to make it explicit that you have
> actually already fixed the bug that you described. The use of future tense is
> problematic here.

Oh I see. Updated description and should be clearer now.

[willchan no longer on Chromium, 2014-07-21 20:32:26]

lgtm

[boliu, 2014-07-21 20:32:53]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-07-21 20:33:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/boliu@chromium.org/403803004/80001

[commit-bot: I haz the power, 2014-07-21 21:34:42]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)

[commit-bot: I haz the power, 2014-07-21 23:11:29]

Change committed as 284534