Make members of Singleton<T> private and only visible to the singleton type. This enforces that the Singleton<T> pattern can only be used within classes which want singleton-ness.

As part of this CL I have also fixed up files which got missed in my previous CLs to use a GetInstance() method and use Singleton<T> from the source file.

There are a small number of places where I have also switched to LazyInstance as that was more appropriate for types used in a single source file.

BUG=65298
TEST=all existing tests should continue to pass.

Review URL: http://codereview.chromium.org/5682008

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@69107 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 179 insertions, 137 deletions

diff --git a/base/i18n/number_formatting.cc b/base/i18n/number_formatting.cc
index 7a69294..df6af14 100644
--- a/base/i18n/number_formatting.cc
+++ b/base/i18n/number_formatting.cc
@@ -6,7 +6,8 @@
 
 #include "base/format_macros.h"
 #include "base/logging.h"
-#include "base/singleton.h"
+#include "base/lazy_instance.h"
+#include "base/scoped_ptr.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #include "unicode/numfmt.h"
@@ -16,25 +17,26 @@ namespace base {
 
 namespace {
 
-struct NumberFormatSingletonTraits
-    : public DefaultSingletonTraits<icu::NumberFormat> {
-  static icu::NumberFormat* New() {
+struct NumberFormatWrapper {
+  NumberFormatWrapper() {
+    // There's no ICU call to destroy a NumberFormat object other than
+    // operator delete, so use the default Delete, which calls operator delete.
+    // This can cause problems if a different allocator is used by this file
+    // than by ICU.
     UErrorCode status = U_ZERO_ERROR;
-    icu::NumberFormat* formatter = icu::NumberFormat::createInstance(status);
+    number_format.reset(icu::NumberFormat::createInstance(status));
     DCHECK(U_SUCCESS(status));
-    return formatter;
   }
-  // There's no ICU call to destroy a NumberFormat object other than
-  // operator delete, so use the default Delete, which calls operator delete.
-  // This can cause problems if a different allocator is used by this file than
-  // by ICU.
+
+  scoped_ptr<icu::NumberFormat> number_format;
 };
 
 }  // namespace
 
+static LazyInstance<NumberFormatWrapper> g_number_format(LINKER_INITIALIZED);
+
 string16 FormatNumber(int64 number) {
-  icu::NumberFormat* number_format =
-      Singleton<icu::NumberFormat, NumberFormatSingletonTraits>::get();
+  icu::NumberFormat* number_format = g_number_format.Get().number_format.get();
 
   if (!number_format) {
     // As a fallback, just return the raw number in a string.
diff --git a/base/lazy_instance.h b/base/lazy_instance.h
index f8d5987..874e87e 100644
--- a/base/lazy_instance.h
+++ b/base/lazy_instance.h
@@ -127,7 +127,8 @@ class LazyInstance : public LazyInstanceHelper {
         NeedsInstance()) {
       // Create the instance in the space provided by |buf_|.
       instance_ = Traits::New(buf_);
-      CompleteInstance(instance_, Traits::Delete);
+      // Traits::Delete will be null for LeakyLazyInstannceTraits
+      CompleteInstance(this, (Traits::Delete == NULL) ? NULL : OnExit);
     }
 
     // This annotation helps race detectors recognize correct lock-less
@@ -140,6 +141,17 @@ class LazyInstance : public LazyInstanceHelper {
   }
 
  private:
+  // Adapter function for use with AtExit.  This should be called single
+  // threaded, so don't use atomic operations.
+  // Calling OnExit while the instance is in use by other threads is a mistake.
+  static void OnExit(void* lazy_instance) {
+    LazyInstance<Type, Traits>* me =
+        reinterpret_cast<LazyInstance<Type, Traits>*>(lazy_instance);
+    Traits::Delete(me->instance_);
+    me->instance_ = NULL;
+    base::subtle::Release_Store(&me->state_, STATE_EMPTY);
+  }
+
   int8 buf_[sizeof(Type)];  // Preallocate the space for the Type instance.
   Type *instance_;
 
diff --git a/base/path_service.cc b/base/path_service.cc
index 4381998..56ce5fa 100644
--- a/base/path_service.cc
+++ b/base/path_service.cc
@@ -13,9 +13,9 @@
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/hash_tables.h"
+#include "base/lazy_instance.h"
 #include "base/lock.h"
 #include "base/logging.h"
-#include "base/singleton.h"
 
 namespace base {
   bool PathProvider(int key, FilePath* result);
@@ -118,8 +118,10 @@ struct PathData {
   }
 };
 
+static base::LazyInstance<PathData> g_path_data(base::LINKER_INITIALIZED);
+
 static PathData* GetPathData() {
-  return Singleton<PathData>::get();
+  return g_path_data.Pointer();
 }
 
 }  // namespace
diff --git a/base/singleton.h b/base/singleton.h
index 8435c43..e5713c4 100644
--- a/base/singleton.h
+++ b/base/singleton.h
@@ -114,7 +114,6 @@ template <typename Type> intptr_t
 template <typename Type> base::subtle::Atomic32
     StaticMemorySingletonTraits<Type>::dead_ = 0;
 
-
 // The Singleton<Type, Traits, DifferentiatingType> class manages a single
 // instance of Type which will be created on first use and will be destroyed at
 // normal process exit). The Trait::Delete function will not be called on
@@ -124,15 +123,37 @@ template <typename Type> base::subtle::Atomic32
 // singletons having the same memory allocation functions but serving a
 // different purpose. This is mainly used for Locks serving different purposes.
 //
-// Example usages: (none are preferred, they all result in the same code)
-//   1. FooClass* ptr = Singleton<FooClass>::get();
-//      ptr->Bar();
-//   2. Singleton<FooClass>()->Bar();
-//   3. Singleton<FooClass>::get()->Bar();
+// Example usage:
+//
+// In your header:
+//   #include "base/singleton.h"
+//   class FooClass {
+//    public:
+//     static FooClass* GetInstance();  <-- See comment below on this.
+//     void Bar() { ... }
+//    private:
+//     FooClass() { ... }
+//     friend struct DefaultSingletonTraits<FooClass>;
+//
+//     DISALLOW_COPY_AND_ASSIGN(FooClass);
+//   };
+//
+// In your source file:
+//  FooClass* FooClass::GetInstance() {
+//    return Singleton<FooClass>::get();
+//  }
+//
+// And to call methods on FooClass:
+//   FooClass::GetInstance()->Bar();
+//
+// NOTE: The method accessing Singleton<T>::get() has to be named as GetInstance
+// and it is important that FooClass::GetInstance() is not inlined in the
+// header. This makes sure that when source files from multiple targets include
+// this header they don't end up with different copies of the inlined code
+// creating multiple copies of the singleton.
 //
 // Singleton<> has no non-static members and doesn't need to actually be
-// instantiated. It does no harm to instantiate it and use it as a class member
-// or at global level since it is acting as a POD type.
+// instantiated.
 //
 // This class is itself thread-safe. The underlying Type must of course be
 // thread-safe if you want to use it concurrently. Two parameters may be tuned
@@ -152,20 +173,6 @@ template <typename Type> base::subtle::Atomic32
 // shouldn't be false unless absolutely necessary. Remember that the heap where
 // the object is allocated may be destroyed by the CRT anyway.
 //
-// If you want to ensure that your class can only exist as a singleton, make
-// its constructors private, and make DefaultSingletonTraits<> a friend:
-//
-//   #include "base/singleton.h"
-//   class FooClass {
-//    public:
-//     void Bar() { ... }
-//    private:
-//     FooClass() { ... }
-//     friend struct DefaultSingletonTraits<FooClass>;
-//
-//     DISALLOW_COPY_AND_ASSIGN(FooClass);
-//   };
-//
 // Caveats:
 // (a) Every call to get(), operator->() and operator*() incurs some overhead
 //     (16ns on my P4/2.8GHz) to check whether the object has already been
@@ -179,7 +186,11 @@ template <typename Type,
           typename Traits = DefaultSingletonTraits<Type>,
           typename DifferentiatingType = Type>
 class Singleton {
- public:
+ private:
+  // Classes using the Singleton<T> pattern should declare a GetInstance()
+  // method and call Singleton::get() from within that.
+  friend Type* Type::GetInstance();
+
   // This class is safe to be constructed and copy-constructed since it has no
   // member.
 
@@ -240,16 +251,6 @@ class Singleton {
     return reinterpret_cast<Type*>(value);
   }
 
-  // Shortcuts.
-  Type& operator*() {
-    return *get();
-  }
-
-  Type* operator->() {
-    return get();
-  }
-
- private:
   // Adapter function for use with AtExit().  This should be called single
   // threaded, so don't use atomic operations.
   // Calling OnExit while singleton is in use by other threads is a mistake.
diff --git a/base/singleton_unittest.cc b/base/singleton_unittest.cc
index acb1247..3d7e7e6 100644
--- a/base/singleton_unittest.cc
+++ b/base/singleton_unittest.cc
@@ -12,87 +12,131 @@ namespace {
 
 COMPILE_ASSERT(DefaultSingletonTraits<int>::kRegisterAtExit == true, a);
 
-template<typename Type>
-struct LockTrait : public DefaultSingletonTraits<Type> {
-};
+typedef void (*CallbackFunc)();
 
-struct Init5Trait : public DefaultSingletonTraits<int> {
-  static int* New() {
-    return new int(5);
+class IntSingleton {
+ public:
+  static IntSingleton* GetInstance() {
+    return Singleton<IntSingleton>::get();
   }
+
+  int value_;
 };
 
-typedef void (*CallbackFunc)();
+class Init5Singleton {
+ public:
+  struct Trait;
 
-struct CallbackTrait : public DefaultSingletonTraits<CallbackFunc> {
-  static void Delete(CallbackFunc* p) {
-    if (*p)
-      (*p)();
-    DefaultSingletonTraits<CallbackFunc>::Delete(p);
+  static Init5Singleton* GetInstance() {
+    return Singleton<Init5Singleton, Trait>::get();
   }
+
+  int value_;
 };
 
-struct StaticCallbackTrait : public StaticMemorySingletonTraits<CallbackFunc> {
-  static void Delete(CallbackFunc* p) {
-    if (*p)
-      (*p)();
-    StaticMemorySingletonTraits<CallbackFunc>::Delete(p);
+struct Init5Singleton::Trait : public DefaultSingletonTraits<Init5Singleton> {
+  static Init5Singleton* New() {
+    Init5Singleton* instance = new Init5Singleton();
+    instance->value_ = 5;
+    return instance;
   }
 };
 
+int* SingletonInt() {
+  return &IntSingleton::GetInstance()->value_;
+}
+
+int* SingletonInt5() {
+  return &Init5Singleton::GetInstance()->value_;
+}
 
-struct NoLeakTrait : public CallbackTrait {
+template <typename Type>
+struct CallbackTrait : public DefaultSingletonTraits<Type> {
+  static void Delete(Type* instance) {
+    if (instance->callback_)
+      (instance->callback_)();
+    DefaultSingletonTraits<Type>::Delete(instance);
+  }
 };
 
-struct LeakTrait : public CallbackTrait {
-  static const bool kRegisterAtExit = false;
+class CallbackSingleton {
+ public:
+  CallbackSingleton() : callback_(NULL) { }
+  CallbackFunc callback_;
 };
 
-int* SingletonInt1() {
-  return Singleton<int>::get();
-}
+class CallbackSingletonWithNoLeakTrait : public CallbackSingleton {
+ public:
+  struct Trait : public CallbackTrait<CallbackSingletonWithNoLeakTrait> { };
 
-int* SingletonInt2() {
-  // Force to use a different singleton than SingletonInt1.
-  return Singleton<int, DefaultSingletonTraits<int> >::get();
-}
+  CallbackSingletonWithNoLeakTrait() : CallbackSingleton() { }
 
-class DummyDifferentiatingClass {
+  static CallbackSingletonWithNoLeakTrait* GetInstance() {
+    return Singleton<CallbackSingletonWithNoLeakTrait, Trait>::get();
+  }
 };
 
-int* SingletonInt3() {
-  // Force to use a different singleton than SingletonInt1 and SingletonInt2.
-  // Note that any type can be used; int, float, std::wstring...
-  return Singleton<int, DefaultSingletonTraits<int>,
-                   DummyDifferentiatingClass>::get();
-}
+class CallbackSingletonWithLeakTrait : public CallbackSingleton {
+ public:
+  struct Trait : public CallbackTrait<CallbackSingletonWithLeakTrait> {
+    static const bool kRegisterAtExit = false;
+  };
 
-int* SingletonInt4() {
-  return Singleton<int, LockTrait<int> >::get();
-}
+  CallbackSingletonWithLeakTrait() : CallbackSingleton() { }
+
+  static CallbackSingletonWithLeakTrait* GetInstance() {
+    return Singleton<CallbackSingletonWithLeakTrait, Trait>::get();
+  }
+};
+
+class CallbackSingletonWithStaticTrait : public CallbackSingleton {
+ public:
+  struct Trait;
+
+  CallbackSingletonWithStaticTrait() : CallbackSingleton() { }
+
+  static CallbackSingletonWithStaticTrait* GetInstance() {
+    return Singleton<CallbackSingletonWithStaticTrait, Trait>::get();
+  }
+};
+
+struct CallbackSingletonWithStaticTrait::Trait
+    : public StaticMemorySingletonTraits<CallbackSingletonWithStaticTrait> {
+  static void Delete(CallbackSingletonWithStaticTrait* instance) {
+    if (instance->callback_)
+      (instance->callback_)();
+    StaticMemorySingletonTraits<CallbackSingletonWithStaticTrait>::Delete(
+        instance);
+  }
+};
 
-int* SingletonInt5() {
-  return Singleton<int, Init5Trait>::get();
-}
 
 void SingletonNoLeak(CallbackFunc CallOnQuit) {
-  *Singleton<CallbackFunc, NoLeakTrait>::get() = CallOnQuit;
+  CallbackSingletonWithNoLeakTrait::GetInstance()->callback_ = CallOnQuit;
 }
 
 void SingletonLeak(CallbackFunc CallOnQuit) {
-  *Singleton<CallbackFunc, LeakTrait>::get() = CallOnQuit;
+  CallbackSingletonWithLeakTrait::GetInstance()->callback_ = CallOnQuit;
 }
 
 CallbackFunc* GetLeakySingleton() {
-  return Singleton<CallbackFunc, LeakTrait>::get();
+  return &CallbackSingletonWithLeakTrait::GetInstance()->callback_;
+}
+
+void DeleteLeakySingleton() {
+  DefaultSingletonTraits<CallbackSingletonWithLeakTrait>::Delete(
+      CallbackSingletonWithLeakTrait::GetInstance());
 }
 
 void SingletonStatic(CallbackFunc CallOnQuit) {
-  *Singleton<CallbackFunc, StaticCallbackTrait>::get() = CallOnQuit;
+  CallbackSingletonWithStaticTrait::GetInstance()->callback_ = CallOnQuit;
 }
 
 CallbackFunc* GetStaticSingleton() {
-  return Singleton<CallbackFunc, StaticCallbackTrait>::get();
+  return &CallbackSingletonWithStaticTrait::GetInstance()->callback_;
+}
+
+void ResurrectStaticSingleton() {
 }
 
 }  // namespace
@@ -149,10 +193,7 @@ bool SingletonTest::leaky_called_ = false;
 bool SingletonTest::static_called_ = false;
 
 TEST_F(SingletonTest, Basic) {
-  int* singleton_int_1;
-  int* singleton_int_2;
-  int* singleton_int_3;
-  int* singleton_int_4;
+  int* singleton_int;
   int* singleton_int_5;
   CallbackFunc* leaky_singleton;
   CallbackFunc* static_singleton;
@@ -160,49 +201,20 @@ TEST_F(SingletonTest, Basic) {
   {
     base::ShadowingAtExitManager sem;
     {
-      singleton_int_1 = SingletonInt1();
+      singleton_int = SingletonInt();
     }
     // Ensure POD type initialization.
-    EXPECT_EQ(*singleton_int_1, 0);
-    *singleton_int_1 = 1;
-
-    EXPECT_EQ(singleton_int_1, SingletonInt1());
-    EXPECT_EQ(*singleton_int_1, 1);
-
-    {
-      singleton_int_2 = SingletonInt2();
-    }
-    // Same instance that 1.
-    EXPECT_EQ(*singleton_int_2, 1);
-    EXPECT_EQ(singleton_int_1, singleton_int_2);
+    EXPECT_EQ(*singleton_int, 0);
+    *singleton_int = 1;
 
-    {
-      singleton_int_3 = SingletonInt3();
-    }
-    // Different instance than 1 and 2.
-    EXPECT_EQ(*singleton_int_3, 0);
-    EXPECT_NE(singleton_int_1, singleton_int_3);
-    *singleton_int_3 = 3;
-    EXPECT_EQ(*singleton_int_1, 1);
-    EXPECT_EQ(*singleton_int_2, 1);
-
-    {
-      singleton_int_4 = SingletonInt4();
-    }
-    // Use a lock for creation. Not really tested at length.
-    EXPECT_EQ(*singleton_int_4, 0);
-    *singleton_int_4 = 4;
-    EXPECT_NE(singleton_int_1, singleton_int_4);
-    EXPECT_NE(singleton_int_3, singleton_int_4);
+    EXPECT_EQ(singleton_int, SingletonInt());
+    EXPECT_EQ(*singleton_int, 1);
 
     {
       singleton_int_5 = SingletonInt5();
     }
     // Is default initialized to 5.
     EXPECT_EQ(*singleton_int_5, 5);
-    EXPECT_NE(singleton_int_1, singleton_int_5);
-    EXPECT_NE(singleton_int_3, singleton_int_5);
-    EXPECT_NE(singleton_int_4, singleton_int_5);
 
     SingletonNoLeak(&CallbackNoLeak);
     SingletonLeak(&CallbackLeak);
@@ -216,7 +228,7 @@ TEST_F(SingletonTest, Basic) {
   VerifiesCallbacks();
   // Delete the leaky singleton. It is interesting to note that Purify does
   // *not* detect the leak when this call is commented out. :(
-  DefaultSingletonTraits<CallbackFunc>::Delete(leaky_singleton);
+  DeleteLeakySingleton();
 
   // The static singleton can't be acquired post-atexit.
   EXPECT_EQ(NULL, GetStaticSingleton());
@@ -225,8 +237,8 @@ TEST_F(SingletonTest, Basic) {
     base::ShadowingAtExitManager sem;
     // Verifiy that the variables were reset.
     {
-      singleton_int_1 = SingletonInt1();
-      EXPECT_EQ(*singleton_int_1, 0);
+      singleton_int = SingletonInt();
+      EXPECT_EQ(*singleton_int, 0);
     }
     {
       singleton_int_5 = SingletonInt5();
@@ -235,7 +247,7 @@ TEST_F(SingletonTest, Basic) {
     {
       // Resurrect the static singleton, and assert that it
       // still points to the same (static) memory.
-      StaticMemorySingletonTraits<CallbackFunc>::Resurrect();
+      CallbackSingletonWithStaticTrait::Trait::Resurrect();
       EXPECT_EQ(GetStaticSingleton(), static_singleton);
     }
   }
diff --git a/base/worker_pool_mac.mm b/base/worker_pool_mac.mm
index bbc7892..956cfb4 100644
--- a/base/worker_pool_mac.mm
+++ b/base/worker_pool_mac.mm
@@ -4,11 +4,12 @@
 
 #include "base/worker_pool_mac.h"
 
+#include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/mac/scoped_nsautorelease_pool.h"
 #include "base/metrics/histogram.h"
+#include "base/scoped_nsobject.h"
 #include "base/scoped_ptr.h"
-#import "base/singleton_objc.h"
 #include "base/task.h"
 #include "base/third_party/dynamic_annotations/dynamic_annotations.h"
 #include "base/worker_pool_linux.h"
@@ -34,6 +35,18 @@ std::vector<id> outstanding_ops_;  // Outstanding operations at last check.
 size_t running_ = 0;               // Operations in |Run()|.
 size_t outstanding_ = 0;           // Operations posted but not completed.
 
+// We use a wrapper struct here for the NSOperationQueue so that the object
+// can be released when LazyInstance calls our destructor.
+struct NSOperationQueueWrapper {
+  NSOperationQueueWrapper() {
+    operation_queue.reset([[NSOperationQueue alloc] init]);
+  }
+  scoped_nsobject<NSOperationQueue> operation_queue;
+};
+
+static base::LazyInstance<NSOperationQueueWrapper> g_nsoperation_queue(
+    base::LINKER_INITIALIZED);
+
 }  // namespace
 
 namespace worker_pool_mac {
@@ -47,7 +60,7 @@ void SetUseLinuxWorkerPool(bool flag) {
 @implementation WorkerPoolObjC
 
 + (NSOperationQueue*)sharedOperationQueue {
-  return SingletonObjC<NSOperationQueue>::get();
+  return g_nsoperation_queue.Get().operation_queue.get();
 }
 
 @end  // @implementation WorkerPoolObjC