Start and stop crash reporting outside of the loader lock.

chrome_frame/scoped_initialization_manager.h

Index: chrome_frame/scoped_initialization_manager.h
diff --git a/chrome_frame/scoped_initialization_manager.h b/chrome_frame/scoped_initialization_manager.h
new file mode 100644
index 0000000000000000000000000000000000000000..3b57e2c1f2cba5f8751654998fda1d0656db5809
--- /dev/null
+++ b/chrome_frame/scoped_initialization_manager.h
@@ -0,0 +1,105 @@
+// Copyright 2013 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.
+
+#ifndef CHROME_FRAME_SCOPED_INITIALIZATION_MANAGER_H_
+#define CHROME_FRAME_SCOPED_INITIALIZATION_MANAGER_H_
+
+#include "base/basictypes.h"
+#include "base/lazy_instance.h"
+#include "base/synchronization/lock.h"
+
+namespace chrome_frame {
+
+// A class intended to be instantiated on the stack in a dyanmically loaded
+// shared object to initialize and shutdown the object's dependencies. |Traits|
+// must be a type with two public static void(void) functions named Initialize
+// and Shutdown. Traits::Initialize will be invoked when the first instance of
+// this class is created and Traits::Shutdown will ordinarily be invoked when
+// the last one is destroyed. Shutdown will be deferred if any instance's Commit
+// method is invoked, causing whatever was initialized to outlive all instances
+// of this class. Decommit can be invoked later to restore the original behavior
+// (i.e., Shutdown will be invoked once all instances are destroyed).

[robertshield, 2013/03/12 03:16:49]

Consider mentioning that multiple Commit() calls are idempotent.

[grt (UTC plus 2), 2013/03/12 14:45:51]

Done.

+template<class Traits>
+class ScopedInitializationManager {
+ public:
+  ScopedInitializationManager() : manager_(manager_instance_.Get()) {
+    manager_.AddRef();
+  }
+  ~ScopedInitializationManager() {
+    manager_.Release();
+  }
+
+  // Causes crash reporting to outlive this and all other instances.
+  void Commit() {
+    manager_.Commit();
+  }
+
+  // Causes crash reporting to be stopped once this and all other instances have
+  // been destroyed.
+  void Decommit() {
+    manager_.Decommit();
+  }
+
+ private:
+  // The long-lived manager class. A single instance of this is lazily created
+  // when the first scoper is created and is never destroyed. Each scoper takes
+  // a reference on the manager. The initialize function is invoked when the
+  // first reference is taken. The Shutdown method is invoked when the last
+  // reference is released if Commit has not been invoked.

[robertshield, 2013/03/12 03:16:49]

Nit: You could write this without the inner class, moving the implementation
directly into ScopedInitializationManager, having |ref_count| and |committed| be
static members. The code would be a bit shorter, but there might be something
lost that I am missing. What say you?

[grt (UTC plus 2), 2013/03/12 14:45:51]

Hmm. The base::Lock would need to be lazy-initialized, so I don't see that
there's a low-hanging fruit gain. I find the separation in the current code very
easy to understand and reason about. Do you find it overly complicated?

[robertshield, 2013/03/12 16:06:12]

Not overly complicated, just felt a little longer than it had to be, but it's
just a nit. I do not object to this implementation.

+  class CrashReportingManager {
+   public:
+    CrashReportingManager() : ref_count_(0), committed_(false) {}
+    ~CrashReportingManager() {
+      // Instances of this class are leaked.
+      NOTREACHED();
+    }
+
+    void AddRef() {
+      base::AutoLock auto_lock(lock_);
+      DCHECK_LT(ref_count_, kuint32max);
+      if (++ref_count_ == 1 && !committed_)

[robertshield, 2013/03/12 03:16:49]

nit: I mildly prefer to have the increment operation on a separate line as I
feel it is more readable. If you violently disagree, then feel free to ignore
me.

[grt (UTC plus 2), 2013/03/12 14:45:51]

I disagree just enough to want to leave it this way. :-)

[robertshield, 2013/03/12 16:06:12]

I suspected you might :-)

+        Traits::Initialize();
+    }
+
+    void Release() {
+      base::AutoLock auto_lock(lock_);
+      DCHECK_GT(ref_count_, 0U);
+      if (--ref_count_ == 0 && !committed_)
+        Traits::Shutdown();
+    }
+
+    void Commit() {
+      base::AutoLock auto_lock(lock_);
+      DCHECK_NE(ref_count_, 0U);
+      committed_ = true;
+    }
+
+    void Decommit() {
+      base::AutoLock auto_lock(lock_);
+      DCHECK_NE(ref_count_, 0U);
+      committed_ = false;
+    }
+
+    base::Lock lock_;
+    uint32 ref_count_;
+    bool committed_;
+    DISALLOW_COPY_AND_ASSIGN(CrashReportingManager);
+  };
+
+  static typename base::LazyInstance<CrashReportingManager>::Leaky
+      manager_instance_;
+
+  CrashReportingManager& manager_;
+  DISALLOW_COPY_AND_ASSIGN(ScopedInitializationManager);
+};
+
+template<class Traits>
+typename base::LazyInstance<
+    typename ScopedInitializationManager<Traits>::CrashReportingManager>::Leaky
+    ScopedInitializationManager<Traits>::manager_instance_ =
+    LAZY_INSTANCE_INITIALIZER;
+
+}  // namespace chrome_frame
+
+#endif  // CHROME_FRAME_SCOPED_INITIALIZATION_MANAGER_H_