Fix AtExitManager recursive lock acquisition.

Fix AtExitManager recursive lock acquisition.

Also detect when new callbacks are being added while callbacks are being processed.

Committed: https://crrev.com/f35088d4ad8a6628d2f50806b84596a647e59a6b
Cr-Commit-Position: refs/heads/master@{#374575}

[Anand Mistry (off Chromium), 2016-02-03 02:39:07]

amistry@chromium.org changed reviewers:
+ mark@chromium.org

[Anand Mistry (off Chromium), 2016-02-03 02:39:07]

I'm not sure about this change. Although it fixes the bug, I wonder if it's
better to instead detect that this is happening and CHECK fail instead. I think
this situation may be the result of a programming error rather than a normal
condition. WDYT?

[Mark Mentovai, 2016-02-03 15:14:45]

I think I’d prefer to investigate the “you can’t add new callbacks while
callbacks are being run” approach. If that proves unworkable for some reason, we
can come back here.

[Anand Mistry (off Chromium), 2016-02-08 03:05:06]

Description was changed from

==========
Fix AtExitManager recursive lock acquisition.
==========

to

==========
Fix AtExitManager recursive lock acquisition.

Also detect when new callbacks are being added while callbacks are being
processed.
==========

[Anand Mistry (off Chromium), 2016-02-08 03:07:20]

On 2016/02/03 15:14:45, Mark Mentovai wrote:
> I think I’d prefer to investigate the “you can’t add new callbacks while
> callbacks are being run” approach. If that proves unworkable for some
reason, we
> can come back here.

PTAL.

[Mark Mentovai, 2016-02-08 15:15:46]

https://codereview.chromium.org/1661673003/diff/40001/base/at_exit.cc
File base/at_exit.cc (right):

https://codereview.chromium.org/1661673003/diff/40001/base/at_exit.cc#newcode59
base/at_exit.cc:59: DCHECK(!g_top_manager->processing_callbacks_);
Maybe this should be a CHECK.

https://codereview.chromium.org/1661673003/diff/40001/base/at_exit.cc#newcode70
base/at_exit.cc:70: // Callbacks may add new callbacks, so run them without
holding |lock_|. This
may add→may try to add

[Anand Mistry (off Chromium), 2016-02-08 23:12:25]

https://codereview.chromium.org/1661673003/diff/40001/base/at_exit.cc
File base/at_exit.cc (right):

https://codereview.chromium.org/1661673003/diff/40001/base/at_exit.cc#newcode59
base/at_exit.cc:59: DCHECK(!g_top_manager->processing_callbacks_);
On 2016/02/08 15:15:45, Mark Mentovai wrote:
> Maybe this should be a CHECK.

I didn't want to break release builds, only catch this in tests which have
dcheck_always_on set.

https://codereview.chromium.org/1661673003/diff/40001/base/at_exit.cc#newcode70
base/at_exit.cc:70: // Callbacks may add new callbacks, so run them without
holding |lock_|. This
On 2016/02/08 15:15:45, Mark Mentovai wrote:
> may add→may try to add

Done.

[Mark Mentovai, 2016-02-09 15:37:46]

LGTM

https://codereview.chromium.org/1661673003/diff/40001/base/at_exit.cc
File base/at_exit.cc (right):

https://codereview.chromium.org/1661673003/diff/40001/base/at_exit.cc#newcode59
base/at_exit.cc:59: DCHECK(!g_top_manager->processing_callbacks_);
Anand Mistry wrote:
> On 2016/02/08 15:15:45, Mark Mentovai wrote:
> > Maybe this should be a CHECK.
> 
> I didn't want to break release builds, only catch this in tests which have
> dcheck_always_on set.

I suspect that the real usage of this is different enough outside of tests that
we won’t really catch violations unless this is a CHECK, and that we should
consider doing that at some point.

[Anand Mistry (off Chromium), 2016-02-10 00:27:34]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2016-02-10 00:30:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1661673003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1661673003/60001

[commit-bot: I haz the power, 2016-02-10 02:18:42]

Description was changed from

==========
Fix AtExitManager recursive lock acquisition.

Also detect when new callbacks are being added while callbacks are being
processed.
==========

to

==========
Fix AtExitManager recursive lock acquisition.

Also detect when new callbacks are being added while callbacks are being
processed.
==========

[commit-bot: I haz the power, 2016-02-10 02:18:42]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-10 02:20:02]

Description was changed from

==========
Fix AtExitManager recursive lock acquisition.

Also detect when new callbacks are being added while callbacks are being
processed.
==========

to

==========
Fix AtExitManager recursive lock acquisition.

Also detect when new callbacks are being added while callbacks are being
processed.

Committed: https://crrev.com/f35088d4ad8a6628d2f50806b84596a647e59a6b
Cr-Commit-Position: refs/heads/master@{#374575}
==========

[commit-bot: I haz the power, 2016-02-10 02:20:03]

Patchset 4 (id:??) landed as
https://crrev.com/f35088d4ad8a6628d2f50806b84596a647e59a6b
Cr-Commit-Position: refs/heads/master@{#374575}