[Backport][Scheduler]: Revamp SequenceLocalStorage.

This CL fixes a few issues with SequenceLocalStorage:
- Supports holding T with disabled copy/move constructor
  by adding emplace().
- Prevents auto default construction with GetValuePointer.
- Query if SequenceLocalStorageSlot currently holds a value
  with operator bool().
- Update doc to use NoDestructor.

This makes it possible to replace in many cases:
SequenceLocalStorageSlot<unique_ptr<T>> with
SequenceLocalStorageSlot<T>

and
auto ptr = sls.Get();
if (!ptr) {
  ptr = make_unique<T>();
  sls.Set(ptr);
}
return *ptr;

with
return sls.GetOrCreateValue();

TBR=fdoray@chromium.org

Change-Id: I3ebb4201fa6d0df296d4a4eaa9b90a1692f3a772
Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664708}
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>

8 files changed, 78 insertions, 84 deletions

diff --git a/chromium/base/threading/sequence_local_storage_slot.cc b/chromium/base/threading/sequence_local_storage_slot.cc
index b7db40bb4fe..5ae1d9f47ae 100644
--- a/chromium/base/threading/sequence_local_storage_slot.cc
+++ b/chromium/base/threading/sequence_local_storage_slot.cc
@@ -23,4 +23,5 @@ int GetNextSequenceLocalStorageSlotNumber() {
 }
 
 }  // namespace internal
+
 }  // namespace base
diff --git a/chromium/base/threading/sequence_local_storage_slot.h b/chromium/base/threading/sequence_local_storage_slot.h
index 1d6dd7c606b..b8a8496e2f3 100644
--- a/chromium/base/threading/sequence_local_storage_slot.h
+++ b/chromium/base/threading/sequence_local_storage_slot.h
@@ -9,6 +9,7 @@
 #include <utility>
 
 #include "base/base_export.h"
+#include "base/template_util.h"
 #include "base/threading/sequence_local_storage_map.h"
 
 namespace base {
@@ -22,17 +23,18 @@ BASE_EXPORT int GetNextSequenceLocalStorageSlotNumber();
 //
 // Example usage:
 //
-// namespace {
-// base::LazyInstance<SequenceLocalStorageSlot<int>> sls_value;
+// int& GetSequenceLocalStorage()
+//     static base::NoDestructor<SequenceLocalStorageSlot<int>> sls_value;
+//     return sls_value->GetOrCreateValue();
 // }
 //
 // void Read() {
-//   int value = sls_value.Get().Get();
+//   int value = GetSequenceLocalStorage();
 //   ...
 // }
 //
 // void Write() {
-//   sls_value.Get().Set(42);
+//   GetSequenceLocalStorage() = 42;
 // }
 //
 // void PostTasks() {
@@ -55,27 +57,48 @@ class SequenceLocalStorageSlot {
       : slot_id_(internal::GetNextSequenceLocalStorageSlotNumber()) {}
   ~SequenceLocalStorageSlot() = default;
 
-  // Get the sequence-local value stored in this slot. Returns a
-  // default-constructed value if no value was previously set.
-  T& Get() {
-    void* value =
+  operator bool() const { return GetValuePointer() != nullptr; }
+
+  // Default-constructs the value for the current sequence if not
+  // already constructed. Then, returns the value.
+  T& GetOrCreateValue() {
+    T* ptr = GetValuePointer();
+    if (!ptr)
+      ptr = emplace();
+    return *ptr;
+  }
+
+  // Returns a pointer to the value for the current sequence. May be
+  // nullptr if the value was not constructed on the current sequence.
+  T* GetValuePointer() {
+    void* ptr =
         internal::SequenceLocalStorageMap::GetForCurrentThread().Get(slot_id_);
+    return static_cast<T*>(ptr);
+  }
+  const T* GetValuePointer() const {
+    return const_cast<SequenceLocalStorageSlot*>(this)->GetValuePointer();
+  }
+
+  T* operator->() { return GetValuePointer(); }
+  const T* operator->() const { return GetValuePointer(); }
 
-    // Sets and returns a default-constructed value if no value was previously
-    // set.
-    if (!value) {
-      Set(T());
-      return Get();
-    }
-    return *(static_cast<T*>(value));
+  T& operator*() { return *GetValuePointer(); }
+  const T& operator*() const { return *GetValuePointer(); }
+
+  void reset() { Adopt(nullptr); }
+
+  // Constructs this slot's sequence-local value with |args...| and returns a
+  // pointer to the created object.
+  template <class... Args>
+  T* emplace(Args&&... args) {
+    T* value_ptr = new T(std::forward<Args>(args)...);
+    Adopt(value_ptr);
+    return value_ptr;
   }
 
-  // Set this slot's sequence-local value to |value|.
-  // Note that if T is expensive to copy, it may be more appropriate to instead
-  // store a std::unique_ptr<T>. This is enforced by the
-  // DISALLOW_COPY_AND_ASSIGN style rather than directly by this class however.
-  void Set(T value) {
-    // Allocates the |value| with new rather than std::make_unique.
+ private:
+  // Takes ownership of |value_ptr|.
+  void Adopt(T* value_ptr) {
     // Since SequenceLocalStorageMap needs to store values of various types
     // within the same map, the type of value_destructor_pair.value is void*
     // (std::unique_ptr<void> is invalid). Memory is freed by calling
@@ -83,8 +106,6 @@ class SequenceLocalStorageSlot {
     // ValueDestructorPair which is invoked when the value is overwritten by
     // another call to SequenceLocalStorageMap::Set or when the
     // SequenceLocalStorageMap is deleted.
-    T* value_ptr = new T(std::move(value));
-
     internal::SequenceLocalStorageMap::ValueDestructorPair::DestructorFunc*
         destructor = [](void* ptr) { Deleter()(static_cast<T*>(ptr)); };
 
@@ -95,7 +116,6 @@ class SequenceLocalStorageSlot {
         slot_id_, std::move(value_destructor_pair));
   }
 
- private:
   // |slot_id_| is used as a key in SequenceLocalStorageMap
   const int slot_id_;
   DISALLOW_COPY_AND_ASSIGN(SequenceLocalStorageSlot);
diff --git a/chromium/components/tracing/common/tracing_sampler_profiler.cc b/chromium/components/tracing/common/tracing_sampler_profiler.cc
index 347cd783481..f295b55d16b 100644
--- a/chromium/components/tracing/common/tracing_sampler_profiler.cc
+++ b/chromium/components/tracing/common/tracing_sampler_profiler.cc
@@ -143,9 +143,9 @@ void TracingSamplerProfiler::CreateOnChildThread() {
   using ProfilerSlot =
       base::SequenceLocalStorageSlot<std::unique_ptr<TracingSamplerProfiler>>;
   static base::NoDestructor<ProfilerSlot> slot;
-  if (!slot.get()->Get()) {
-    slot.get()->Set(base::WrapUnique(
-        new TracingSamplerProfiler(base::PlatformThread::CurrentId())));
+  if (!*slot) {
+    slot->emplace(
+        new TracingSamplerProfiler(base::PlatformThread::CurrentId()));
   }
 }
 
diff --git a/chromium/mojo/public/cpp/bindings/lib/connector.cc b/chromium/mojo/public/cpp/bindings/lib/connector.cc
index 4f73c98ebdc..01c5c3b21b6 100644
--- a/chromium/mojo/public/cpp/bindings/lib/connector.cc
+++ b/chromium/mojo/public/cpp/bindings/lib/connector.cc
@@ -7,12 +7,12 @@
 #include <stdint.h>
 
 #include "base/bind.h"
-#include "base/lazy_instance.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/message_loop/message_loop_current.h"
+#include "base/no_destructor.h"
 #include "base/run_loop.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/sequence_local_storage_slot.h"
@@ -32,13 +32,6 @@ namespace mojo {
 
 namespace {
 
-// The NestingObserver for each thread. Note that this is always a
-// Connector::RunLoopNestingObserver; we use the base type here because that
-// subclass is private to Connector.
-base::LazyInstance<
-    base::SequenceLocalStorageSlot<base::RunLoop::NestingObserver*>>::Leaky
-    g_sls_nesting_observer = LAZY_INSTANCE_INITIALIZER;
-
 // The default outgoing serialization mode for new Connectors.
 Connector::OutgoingSerializationMode g_default_outgoing_serialization_mode =
     Connector::OutgoingSerializationMode::kLazy;
@@ -80,15 +73,15 @@ class Connector::ActiveDispatchTracker {
 // Watches the MessageLoop on the current thread. Notifies the current chain of
 // ActiveDispatchTrackers when a nested run loop is started.
 class Connector::RunLoopNestingObserver
-    : public base::RunLoop::NestingObserver,
-      public base::MessageLoopCurrent::DestructionObserver {
+    : public base::RunLoop::NestingObserver {
  public:
   RunLoopNestingObserver() {
     base::RunLoop::AddNestingObserverOnCurrentThread(this);
-    base::MessageLoopCurrent::Get()->AddDestructionObserver(this);
   }
 
-  ~RunLoopNestingObserver() override {}
+  ~RunLoopNestingObserver() override {
+    base::RunLoop::RemoveNestingObserverOnCurrentThread(this);
+  }
 
   // base::RunLoop::NestingObserver:
   void OnBeginNestedRunLoop() override {
@@ -96,25 +89,16 @@ class Connector::RunLoopNestingObserver
       top_tracker_->NotifyBeginNesting();
   }
 
-  // base::MessageLoopCurrent::DestructionObserver:
-  void WillDestroyCurrentMessageLoop() override {
-    base::RunLoop::RemoveNestingObserverOnCurrentThread(this);
-    base::MessageLoopCurrent::Get()->RemoveDestructionObserver(this);
-    DCHECK_EQ(this, g_sls_nesting_observer.Get().Get());
-    g_sls_nesting_observer.Get().Set(nullptr);
-    delete this;
-  }
-
   static RunLoopNestingObserver* GetForThread() {
     if (!base::MessageLoopCurrent::Get())
       return nullptr;
-    auto* observer = static_cast<RunLoopNestingObserver*>(
-        g_sls_nesting_observer.Get().Get());
-    if (!observer) {
-      observer = new RunLoopNestingObserver;
-      g_sls_nesting_observer.Get().Set(observer);
-    }
-    return observer;
+    // The NestingObserver for each thread. Note that this is always a
+    // Connector::RunLoopNestingObserver; we use the base type here because that
+    // subclass is private to Connector.
+    static base::NoDestructor<
+        base::SequenceLocalStorageSlot<RunLoopNestingObserver>>
+        sls_nesting_observer;
+    return &sls_nesting_observer->GetOrCreateValue();
   }
 
  private:
diff --git a/chromium/mojo/public/cpp/bindings/lib/message.cc b/chromium/mojo/public/cpp/bindings/lib/message.cc
index fa28aa4e51e..1019088895d 100644
--- a/chromium/mojo/public/cpp/bindings/lib/message.cc
+++ b/chromium/mojo/public/cpp/bindings/lib/message.cc
@@ -516,17 +516,17 @@ bool PassThroughFilter::Accept(Message* message) {
 
 SyncMessageResponseContext::SyncMessageResponseContext()
     : outer_context_(current()) {
-  g_sls_sync_response_context.Get().Set(this);
+  g_sls_sync_response_context.Get().emplace(this);
 }
 
 SyncMessageResponseContext::~SyncMessageResponseContext() {
   DCHECK_EQ(current(), this);
-  g_sls_sync_response_context.Get().Set(outer_context_);
+  g_sls_sync_response_context.Get().emplace(outer_context_);
 }
 
 // static
 SyncMessageResponseContext* SyncMessageResponseContext::current() {
-  return g_sls_sync_response_context.Get().Get();
+  return g_sls_sync_response_context.Get().GetOrCreateValue();
 }
 
 void SyncMessageResponseContext::ReportBadMessage(const std::string& error) {
@@ -558,17 +558,17 @@ MessageHeaderV2::MessageHeaderV2() = default;
 
 MessageDispatchContext::MessageDispatchContext(Message* message)
     : outer_context_(current()), message_(message) {
-  g_sls_message_dispatch_context.Get().Set(this);
+  g_sls_message_dispatch_context.Get().emplace(this);
 }
 
 MessageDispatchContext::~MessageDispatchContext() {
   DCHECK_EQ(current(), this);
-  g_sls_message_dispatch_context.Get().Set(outer_context_);
+  g_sls_message_dispatch_context.Get().emplace(outer_context_);
 }
 
 // static
 MessageDispatchContext* MessageDispatchContext::current() {
-  return g_sls_message_dispatch_context.Get().Get();
+  return g_sls_message_dispatch_context.Get().GetOrCreateValue();
 }
 
 ReportBadMessageCallback MessageDispatchContext::GetBadMessageCallback() {
diff --git a/chromium/mojo/public/cpp/bindings/lib/sequence_local_sync_event_watcher.cc b/chromium/mojo/public/cpp/bindings/lib/sequence_local_sync_event_watcher.cc
index f4618ffbe80..6543802d2e1 100644
--- a/chromium/mojo/public/cpp/bindings/lib/sequence_local_sync_event_watcher.cc
+++ b/chromium/mojo/public/cpp/bindings/lib/sequence_local_sync_event_watcher.cc
@@ -73,10 +73,7 @@ class SequenceLocalSyncEventWatcher::SequenceLocalState {
   // Initializes a SequenceLocalState instance in sequence-local storage if
   // not already initialized. Returns a WeakPtr to the stored state object.
   static base::WeakPtr<SequenceLocalState> GetOrCreate() {
-    auto& state_ptr = GetStorageSlot().Get();
-    if (!state_ptr)
-      state_ptr = std::make_unique<SequenceLocalState>();
-    return state_ptr->weak_ptr_factory_.GetWeakPtr();
+    return GetStorageSlot().GetOrCreateValue().weak_ptr_factory_.GetWeakPtr();
   }
 
   // Registers a new watcher and returns an iterator into the WatcherStateMap to
@@ -108,7 +105,7 @@ class SequenceLocalSyncEventWatcher::SequenceLocalState {
     if (registered_watchers_.empty()) {
       // If no more watchers are registered, clear our sequence-local storage.
       // Deletes |this|.
-      GetStorageSlot().Get().reset();
+      GetStorageSlot().reset();
     }
   }
 
@@ -168,8 +165,7 @@ class SequenceLocalSyncEventWatcher::SequenceLocalState {
   }
 
  private:
-  using StorageSlotType =
-      base::SequenceLocalStorageSlot<std::unique_ptr<SequenceLocalState>>;
+  using StorageSlotType = base::SequenceLocalStorageSlot<SequenceLocalState>;
   static StorageSlotType& GetStorageSlot() {
     static base::NoDestructor<StorageSlotType> storage;
     return *storage;
diff --git a/chromium/mojo/public/cpp/bindings/lib/sync_call_restrictions.cc b/chromium/mojo/public/cpp/bindings/lib/sync_call_restrictions.cc
index 2b359861d72..0188933a95a 100644
--- a/chromium/mojo/public/cpp/bindings/lib/sync_call_restrictions.cc
+++ b/chromium/mojo/public/cpp/bindings/lib/sync_call_restrictions.cc
@@ -47,7 +47,7 @@ GlobalSyncCallSettings& GetGlobalSettings() {
 
 size_t& GetSequenceLocalScopedAllowCount() {
   static base::NoDestructor<base::SequenceLocalStorageSlot<size_t>> count;
-  return count->Get();
+  return count->GetOrCreateValue();
 }
 
 }  // namespace
diff --git a/chromium/mojo/public/cpp/bindings/lib/sync_handle_registry.cc b/chromium/mojo/public/cpp/bindings/lib/sync_handle_registry.cc
index 2ac48334450..3278a76a676 100644
--- a/chromium/mojo/public/cpp/bindings/lib/sync_handle_registry.cc
+++ b/chromium/mojo/public/cpp/bindings/lib/sync_handle_registry.cc
@@ -6,36 +6,29 @@
 
 #include <algorithm>
 
-#include "base/lazy_instance.h"
 #include "base/logging.h"
+#include "base/no_destructor.h"
 #include "base/stl_util.h"
 #include "base/threading/sequence_local_storage_slot.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "mojo/public/c/system/core.h"
 
 namespace mojo {
-namespace {
-
-base::LazyInstance<
-    base::SequenceLocalStorageSlot<scoped_refptr<SyncHandleRegistry>>>::Leaky
-    g_current_sync_handle_watcher = LAZY_INSTANCE_INITIALIZER;
-
-}  // namespace
 
 // static
 scoped_refptr<SyncHandleRegistry> SyncHandleRegistry::current() {
+  static base::NoDestructor<
+      base::SequenceLocalStorageSlot<scoped_refptr<SyncHandleRegistry>>>
+      g_current_sync_handle_watcher;
+
   // SyncMessageFilter can be used on threads without sequence-local storage
   // being available. Those receive a unique, standalone SyncHandleRegistry.
   if (!base::SequencedTaskRunnerHandle::IsSet())
     return new SyncHandleRegistry();
 
-  scoped_refptr<SyncHandleRegistry> result =
-      g_current_sync_handle_watcher.Get().Get();
-  if (!result) {
-    result = new SyncHandleRegistry();
-    g_current_sync_handle_watcher.Get().Set(result);
-  }
-  return result;
+  if (!*g_current_sync_handle_watcher)
+    g_current_sync_handle_watcher->emplace(new SyncHandleRegistry());
+  return *g_current_sync_handle_watcher->GetValuePointer();
 }
 
 bool SyncHandleRegistry::RegisterHandle(const Handle& handle,