Fix lifetime of leveldb::MojoEnv instances.

components/leveldb/leveldb_service_impl.cc

Index: components/leveldb/leveldb_service_impl.cc
diff --git a/components/leveldb/leveldb_service_impl.cc b/components/leveldb/leveldb_service_impl.cc
index e3fd368af1777e58c2940ed07b90d3e322ff5090..c23f91ae2b38eb60f730c118f6c3bcbbff33b9d8 100644
--- a/components/leveldb/leveldb_service_impl.cc
+++ b/components/leveldb/leveldb_service_impl.cc
@@ -22,15 +22,10 @@ namespace leveldb {
 
 LevelDBServiceImpl::LevelDBServiceImpl(
     scoped_refptr<base::SingleThreadTaskRunner> file_task_runner)
-    : thread_(new LevelDBMojoProxy(std::move(file_task_runner))),
-      environment_name_("LevelDBEnv") {}
+    : env_(MojoEnv::Get(std::move(file_task_runner))) {}
 
 LevelDBServiceImpl::~LevelDBServiceImpl() {}

[michaeln, 2017/03/27 20:24:11]

should UnregisterDirectory happen in this class?

[Marijn Kruisselbrink, 2017/03/31 23:26:24]

All the open directories should be owned by some LevelDBDatabaseImpl instance.
Destroying the ServiceImpl will close the connection, and thus also close all
the associated database connections which will delete all the
LevelDBDatabaseImpl instances, so there shouldn't be any need for extra
UnregisterDirectory calls.

 
-void LevelDBServiceImpl::SetEnvironmentName(const std::string& name) {
-  environment_name_ = name;
-}
-
 void LevelDBServiceImpl::Open(
     filesystem::mojom::DirectoryPtr directory,
     const std::string& dbname,
@@ -56,20 +51,15 @@ void LevelDBServiceImpl::OpenWithOptions(
   options.reuse_logs = leveldb_env::kDefaultLogReuseOptionValue;
   options.compression = leveldb::kSnappyCompression;
 
-  // Register our directory with the file thread.
-  LevelDBMojoProxy::OpaqueDir* dir =
-      thread_->RegisterDirectory(std::move(directory));
-
-  std::unique_ptr<MojoEnv> env_mojo(
-      new MojoEnv(environment_name_, thread_, dir));
-  options.env = env_mojo.get();
+  std::string prefix = env_->RegisterDirectory(std::move(directory));
+  options.env = env_;
 
   leveldb::DB* db = nullptr;
-  leveldb::Status s = leveldb::DB::Open(options, dbname, &db);
+  leveldb::Status s = leveldb::DB::Open(options, prefix + dbname, &db);
 
   if (s.ok()) {
     mojo::MakeStrongAssociatedBinding(
-        base::MakeUnique<LevelDBDatabaseImpl>(std::move(env_mojo),
+        base::MakeUnique<LevelDBDatabaseImpl>(env_, std::move(prefix),
                                               base::WrapUnique(db)),
         std::move(database));
   }
@@ -104,13 +94,10 @@ void LevelDBServiceImpl::Destroy(filesystem::mojom::DirectoryPtr directory,
                                  const std::string& dbname,
                                  const DestroyCallback& callback) {
   leveldb::Options options;
-  // Register our directory with the file thread.
-  LevelDBMojoProxy::OpaqueDir* dir =
-      thread_->RegisterDirectory(std::move(directory));
-  std::unique_ptr<MojoEnv> env_mojo(
-      new MojoEnv(environment_name_, thread_, dir));
-  options.env = env_mojo.get();
-  callback.Run(LeveldbStatusToError(leveldb::DestroyDB(dbname, options)));
+  std::string prefix = env_->RegisterDirectory(std::move(directory));

[michaeln, 2017/03/27 20:24:10]

Does this leak a directory registration? Wdyt about an object that scopes the
lifetime of a DirRegistration and provides storage for the prefix_ and env_ ptr?

Having the Reg and Unreg calls in different files makes it a little harder to
follow the lifetime of a dir registration.

[Marijn Kruisselbrink, 2017/03/31 23:26:24]

Yeah, definitely agree that it would be nice to not have to worry about matching
reg and unreg calls. (in the before code the Reg call was also here, and the
Unreg call was in the destructor of MojoEnv). I'll see if I can come up with
some sensible way to simplify this... I don't really want to end up with
DirRegistration wrapping OpaqueDir wrapping mojom::DirectoryPtr, but they do all
serve different enough purposes that merging any of them might not be the
entirely right way either...

+  options.env = env_;
+  callback.Run(
+      LeveldbStatusToError(leveldb::DestroyDB(prefix + dbname, options)));
 }
 
 }  // namespace leveldb