Assert on deleting a held lock.

Assert on deleting a held lock.


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

[Sigurður Ásgeirsson, 2013-04-08 12:39:51]

I had a stupid problem in some Syzygy code that this check would have flagged.

[willchan no longer on Chromium, 2013-04-08 15:28:13]

https://codereview.chromium.org/13728003/diff/2001/base/synchronization/lock.cc
File base/synchronization/lock.cc (right):

https://codereview.chromium.org/13728003/diff/2001/base/synchronization/lock....
base/synchronization/lock.cc:27: // This may trip the assert on unlock in the
owning thread.
I don't understand this one. We know that it's false already due to the new
DCHECK on line 24, right?

[Sigurður Ásgeirsson, 2013-04-08 15:32:34]

https://codereview.chromium.org/13728003/diff/2001/base/synchronization/lock.cc
File base/synchronization/lock.cc (right):

https://codereview.chromium.org/13728003/diff/2001/base/synchronization/lock....
base/synchronization/lock.cc:27: // This may trip the assert on unlock in the
owning thread.
On 2013/04/08 15:28:13, willchan wrote:
> I don't understand this one. We know that it's false already due to the new
> DCHECK on line 24, right?

Depends on whether the DCHECK is set to log fatally, it seems in some builds
and/or on some bots we just spew a stack trace and move on. In a build with
those settings, you *may* get a second stack trace as e.g. AssertAcquired and/or
CheckHeldAndUnmark are invoked on the owning thread as part of unlocking. It
depends on whether the lock is e.g. a static instance, and if not, on what
happens to the allocation afterwards.
This is definitely heuristic, though - I'll remove it if you think it has no
value?

[willchan no longer on Chromium, 2013-04-08 15:35:31]

I asked because that was very non-obvious from the code. I didn't realize we had
non-fatal assertions. I think it's better just to remove it and if it spews a
stacktrace twice, no big deal, since it shouldn't be spewing stacktraces in the
first place. It's better than having to write a long comment explaining why this
code does something that looks unnecessary.

LGTM otherwise.

[commit-bot: I haz the power, 2013-04-08 19:22:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/siggi@chromium.org/13728003/11001

[commit-bot: I haz the power, 2013-04-09 01:40:22]

Change committed as 192980