[Sync] ModelTypeStore factory shouldn't require valid PSS to function correctly

components/sync/model_impl/model_type_store_backend.cc

Index: components/sync/model_impl/model_type_store_backend.cc
diff --git a/components/sync/model_impl/model_type_store_backend.cc b/components/sync/model_impl/model_type_store_backend.cc
index afc6fe8f3d7c7f0c37eb2e3ff2ce29b38625a2f9..6a19fd1075dbb12c0ee5a67886d8e36053e6e978 100644
--- a/components/sync/model_impl/model_type_store_backend.cc
+++ b/components/sync/model_impl/model_type_store_backend.cc
@@ -7,8 +7,10 @@
 #include <utility>
 
 #include "base/files/file_path.h"
+#include "base/lazy_instance.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/synchronization/lock.h"
 #include "components/sync/protocol/model_type_store_schema_descriptor.pb.h"
 #include "third_party/leveldatabase/env_chromium.h"
 #include "third_party/leveldatabase/src/helpers/memenv/memenv.h"
@@ -31,10 +33,6 @@ const char ModelTypeStoreBackend::kDBSchemaDescriptorRecordId[] =
 const char ModelTypeStoreBackend::kStoreInitResultHistogramName[] =
     "Sync.ModelTypeStoreInitResult";
 
-// static
-base::LazyInstance<ModelTypeStoreBackend::BackendMap>::DestructorAtExit
-    ModelTypeStoreBackend::backend_map_ = LAZY_INSTANCE_INITIALIZER;
-
 namespace {
 
 StoreInitResultForHistogram LevelDbStatusToStoreInitResult(
@@ -54,13 +52,61 @@ StoreInitResultForHistogram LevelDbStatusToStoreInitResult(
   return STORE_INIT_RESULT_UNKNOWN;
 }
 
+// BackendMap tracks created ModelTypeStoreBackends ensuring at most one backend
+// exists for a given path. BackendMap keeps non-owning pointer to backend
+// allowing backend lifetime to be controlled by consumers. Since backends can
+// run concurrently on different threads all map operations are guarded by lock.
+class BackendMap {
+ public:
+  BackendMap() = default;
+
+  // Returns backend reference or nullptr if backend for |path| is not in the
+  // map.
+  scoped_refptr<ModelTypeStoreBackend> GetBackend(
+      const std::string& path) const;
+  // Adds backend into the map ensuring it wasn't added before.
+  void SetBackend(const std::string& path, ModelTypeStoreBackend* backend);
+  void EraseBackend(const std::string& path);
+
+ private:
+  mutable base::Lock lock_;
+
+  std::unordered_map<std::string, ModelTypeStoreBackend*> backends_;
+
+  DISALLOW_COPY_AND_ASSIGN(BackendMap);
+};
+
+base::LazyInstance<BackendMap>::DestructorAtExit backend_map =
+    LAZY_INSTANCE_INITIALIZER;
+
+scoped_refptr<ModelTypeStoreBackend> BackendMap::GetBackend(
+    const std::string& path) const {
+  base::AutoLock scoped_lock(lock_);
+  auto it = backends_.find(path);
+  if (it == backends_.end())

[skym, 2017/03/08 19:28:15]

a ternary would probably still be a single line, just saying :)

[pavely, 2017/03/08 20:41:12]

Done.

+    return nullptr;
+  return it->second;
+}
+
+void BackendMap::SetBackend(const std::string& path,
+                            ModelTypeStoreBackend* backend) {
+  base::AutoLock scoped_lock(lock_);
+  DCHECK(backends_.find(path) == backends_.end());
+  backends_[path] = backend;
+}
+
+void BackendMap::EraseBackend(const std::string& path) {
+  base::AutoLock scoped_lock(lock_);
+  backends_.erase(path);
+}
+
 }  // namespace
 
 ModelTypeStoreBackend::ModelTypeStoreBackend(const std::string& path)
     : path_(path) {}
 
 ModelTypeStoreBackend::~ModelTypeStoreBackend() {
-  backend_map_.Get().erase(path_);
+  backend_map.Get().EraseBackend(path_);
 }
 
 std::unique_ptr<leveldb::Env> ModelTypeStoreBackend::CreateInMemoryEnv() {
@@ -72,18 +118,19 @@ scoped_refptr<ModelTypeStoreBackend> ModelTypeStoreBackend::GetOrCreateBackend(
     const std::string& path,
     std::unique_ptr<leveldb::Env> env,
     ModelTypeStore::Result* result) {
-  if (backend_map_.Get().find(path) != backend_map_.Get().end()) {
+  scoped_refptr<ModelTypeStoreBackend> backend =
+      backend_map.Get().GetBackend(path);
+  if (backend) {
     *result = ModelTypeStore::Result::SUCCESS;
-    return make_scoped_refptr(backend_map_.Get()[path]);
+    return backend;
   }
 
-  scoped_refptr<ModelTypeStoreBackend> backend =
-      new ModelTypeStoreBackend(path);
+  backend = new ModelTypeStoreBackend(path);
 
   *result = backend->Init(path, std::move(env));
 
   if (*result == ModelTypeStore::Result::SUCCESS) {
-    backend_map_.Get()[path] = backend.get();
+    backend_map.Get().SetBackend(path, backend.get());
   } else {
     backend = nullptr;
   }
@@ -94,7 +141,7 @@ scoped_refptr<ModelTypeStoreBackend> ModelTypeStoreBackend::GetOrCreateBackend(
 ModelTypeStore::Result ModelTypeStoreBackend::Init(
     const std::string& path,
     std::unique_ptr<leveldb::Env> env) {
-  DFAKE_SCOPED_LOCK(push_pop_);
+  DCHECK(sequence_checker_.CalledOnValidSequence());
 
   env_ = std::move(env);
 
@@ -162,7 +209,7 @@ ModelTypeStore::Result ModelTypeStoreBackend::ReadRecordsWithPrefix(
     const ModelTypeStore::IdList& id_list,
     ModelTypeStore::RecordList* record_list,
     ModelTypeStore::IdList* missing_id_list) {
-  DFAKE_SCOPED_LOCK(push_pop_);
+  DCHECK(sequence_checker_.CalledOnValidSequence());
   DCHECK(db_);
   record_list->reserve(id_list.size());
   leveldb::ReadOptions read_options;
@@ -186,7 +233,7 @@ ModelTypeStore::Result ModelTypeStoreBackend::ReadRecordsWithPrefix(
 ModelTypeStore::Result ModelTypeStoreBackend::ReadAllRecordsWithPrefix(
     const std::string& prefix,
     ModelTypeStore::RecordList* record_list) {
-  DFAKE_SCOPED_LOCK(push_pop_);
+  DCHECK(sequence_checker_.CalledOnValidSequence());
   DCHECK(db_);
   leveldb::ReadOptions read_options;
   read_options.verify_checksums = true;
@@ -205,7 +252,7 @@ ModelTypeStore::Result ModelTypeStoreBackend::ReadAllRecordsWithPrefix(
 
 ModelTypeStore::Result ModelTypeStoreBackend::WriteModifications(
     std::unique_ptr<leveldb::WriteBatch> write_batch) {
-  DFAKE_SCOPED_LOCK(push_pop_);
+  DCHECK(sequence_checker_.CalledOnValidSequence());
   DCHECK(db_);
   leveldb::Status status =
       db_->Write(leveldb::WriteOptions(), write_batch.get());
@@ -263,4 +310,9 @@ void ModelTypeStoreBackend::RecordStoreInitResultHistogram(
                             STORE_INIT_RESULT_COUNT);
 }
 
+// static
+bool ModelTypeStoreBackend::BackendExistsForTest(const std::string& path) {
+  return backend_map.Get().GetBackend(path) != nullptr;
+}
+
 }  // namespace syncer