Fix AtExitManager recursive lock acquisition.

base/at_exit.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/at_exit.h"
 
 #include <stddef.h>
 #include <ostream>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/logging.h"
 
 namespace base {
 
 // Keep a stack of registered AtExitManagers.  We always operate on the most
 // recent, and we should never have more than one outside of testing (for a
 // statically linked version of this library).  Testing may use the shadow
 // version of the constructor, and if we are building a dynamic library we may
 // end up with multiple AtExitManagers on the same process.  We don't protect
 // this for thread-safe access, since it will only be modified in testing.
 static AtExitManager* g_top_manager = NULL;
 
-AtExitManager::AtExitManager() : next_manager_(g_top_manager) {
+AtExitManager::AtExitManager()
+    : processing_callbacks_(false), next_manager_(g_top_manager) {
 // If multiple modules instantiate AtExitManagers they'll end up living in this
 // module... they have to coexist.
 #if !defined(COMPONENT_BUILD)
   DCHECK(!g_top_manager);
 #endif
   g_top_manager = this;
 }
 
 AtExitManager::~AtExitManager() {
   if (!g_top_manager) {
@@ skipping 13 matching lines @@
 }
 
 // static
 void AtExitManager::RegisterTask(base::Closure task) {
   if (!g_top_manager) {
     NOTREACHED() << "Tried to RegisterCallback without an AtExitManager";
     return;
   }
 
   AutoLock lock(g_top_manager->lock_);
+  DCHECK(!g_top_manager->processing_callbacks_);

[Mark Mentovai, 2016/02/08 15:15:45]

Maybe this should be a CHECK.

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

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

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

Anand Mistry wrote:
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.

   g_top_manager->stack_.push(task);
 }
 
 // static
 void AtExitManager::ProcessCallbacksNow() {
   if (!g_top_manager) {
     NOTREACHED() << "Tried to ProcessCallbacksNow without an AtExitManager";
     return;
   }
 
-  AutoLock lock(g_top_manager->lock_);
+  // Callbacks may add new callbacks, so run them without holding |lock_|. This

[Mark Mentovai, 2016/02/08 15:15:45]

may add→may try to add

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

Done.

+  // is an error and caught by the DCHECK in RegisterTask(), but handle it
+  // gracefully in release builds so we don't deadlock.
+  std::stack<base::Closure> tasks;
+  {
+    AutoLock lock(g_top_manager->lock_);
+    tasks.swap(g_top_manager->stack_);
+    g_top_manager->processing_callbacks_ = true;
+  }
 
-  while (!g_top_manager->stack_.empty()) {
-    base::Closure task = g_top_manager->stack_.top();
+  while (!tasks.empty()) {
+    base::Closure task = tasks.top();
     task.Run();
-    g_top_manager->stack_.pop();
+    tasks.pop();
   }
+
+  // Expect that all callbacks have been run.
+  DCHECK(g_top_manager->stack_.empty());
 }
 
-AtExitManager::AtExitManager(bool shadow) : next_manager_(g_top_manager) {
+AtExitManager::AtExitManager(bool shadow)
+    : processing_callbacks_(false), next_manager_(g_top_manager) {
   DCHECK(shadow || !g_top_manager);
   g_top_manager = this;
 }
 
 }  // namespace base