Fix lifetime of leveldb::MojoEnv instances.

components/leveldb/leveldb_service_impl.cc

 // Copyright 2016 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.
 
 #include "components/leveldb/leveldb_service_impl.h"
 
 #include <memory>
 
 #include "base/memory/ptr_util.h"
 #include "components/leveldb/env_mojo.h"
 #include "components/leveldb/leveldb_database_impl.h"
 #include "components/leveldb/public/cpp/util.h"
 #include "mojo/public/cpp/bindings/strong_associated_binding.h"
 #include "third_party/leveldatabase/env_chromium.h"
 #include "third_party/leveldatabase/src/helpers/memenv/memenv.h"
 #include "third_party/leveldatabase/src/include/leveldb/db.h"
 #include "third_party/leveldatabase/src/include/leveldb/env.h"
 #include "third_party/leveldatabase/src/include/leveldb/filter_policy.h"
 #include "third_party/leveldatabase/src/include/leveldb/slice.h"
 
 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,
     leveldb::mojom::LevelDBDatabaseAssociatedRequest database,
     const OpenCallback& callback) {
   OpenWithOptions(leveldb::mojom::OpenOptions::New(), std::move(directory),
                   dbname, std::move(database), callback);
 }
 
 void LevelDBServiceImpl::OpenWithOptions(
     leveldb::mojom::OpenOptionsPtr open_options,
     filesystem::mojom::DirectoryPtr directory,
     const std::string& dbname,
     leveldb::mojom::LevelDBDatabaseAssociatedRequest database,
     const OpenCallback& callback) {
   leveldb::Options options;
   options.create_if_missing = open_options->create_if_missing;
   options.error_if_exists = open_options->error_if_exists;
   options.paranoid_checks = open_options->paranoid_checks;
   options.write_buffer_size = open_options->write_buffer_size;
   options.max_open_files = open_options->max_open_files;
 
   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));
   }
 
   callback.Run(LeveldbStatusToError(s));
 }
 
 void LevelDBServiceImpl::OpenInMemory(
     leveldb::mojom::LevelDBDatabaseAssociatedRequest database,
     const OpenCallback& callback) {
@@ skipping 14 matching lines @@
                                       std::move(database));
   }
 
   callback.Run(LeveldbStatusToError(s));
 }
 
 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