Fix lifetime of leveldb::MojoEnv instances.

components/leveldb/env_mojo.cc

Index: components/leveldb/env_mojo.cc
diff --git a/components/leveldb/env_mojo.cc b/components/leveldb/env_mojo.cc
index 82af93a997036fa73be861228083b196b93f85f9..74ec15073df424ee00ce15d78d616de2425da6d2 100644
--- a/components/leveldb/env_mojo.cc
+++ b/components/leveldb/env_mojo.cc
@@ -8,7 +8,10 @@
 
 #include <memory>
 
+#include "base/lazy_instance.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
 #include "base/trace_event/trace_event.h"
 #include "third_party/leveldatabase/chromium_logger.h"
 #include "third_party/leveldatabase/src/include/leveldb/status.h"
@@ -215,22 +218,54 @@ class MojoWritableFile : public leveldb::WritableFile {
   DISALLOW_COPY_AND_ASSIGN(MojoWritableFile);
 };
 
+base::LazyInstance<std::vector<MojoEnv*>>::Leaky g_mojo_envs =
+    LAZY_INSTANCE_INITIALIZER;
+
 }  // namespace
 
-MojoEnv::MojoEnv(const std::string& name,
-                 scoped_refptr<LevelDBMojoProxy> file_thread,
-                 LevelDBMojoProxy::OpaqueDir* dir)
-    : ChromiumEnv(name), thread_(file_thread), dir_(dir) {}
+// static
+MojoEnv* MojoEnv::Get(
+    scoped_refptr<base::SingleThreadTaskRunner> file_task_runner) {
+  auto* envs = g_mojo_envs.Pointer();
+  for (MojoEnv* env : *envs) {
+    if (env->thread_->task_runner() == file_task_runner)

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

I think this does ptr comparison, do we seen multiple task_runner instances all
pointing to the same thread? I'm wondering if the collection will grow
unexpectedly?

What about thread safety, does it matter?

The comment "but instead have a global instance" implying a single instance, but
really there's a collection here. I'm guessing the comment is stale and you
discovered the need for the collection?

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

Part of the confusion/complication is because of how generic this class is
(trying to be) vs how it is actually used in chrome. In chrome the same task
runner (the FILE BrowserThread) is always passed as file_task_runner, and
methods are always called on the same thread (the IO BrowserThread). But since
this code can't actually depend on content/, it can't really assume that the
same task runner is always passed in. I could just keep a single instance, and
assert that you're always calling this method with the same task runner, but
that might possibly make the class less useful? But on the other hand we would
really like to only have to have one MojoEnv, so maybe enforcing that wouldn't
be so bad for now...

Similar about thread safety, easiest might be to add some dchecks to make sure
this is always called from the same thread... But that would be kind of ugly
(and maybe not in the spirit of mojo servicification?). So yeah, good
questions... I'll have to think about this for a bit...

+      return env;
+  }
+  MojoEnv* env = new MojoEnv(std::move(file_task_runner));
+  envs->push_back(env);
+  return env;
+}
 
-MojoEnv::~MojoEnv() {
-  thread_->UnregisterDirectory(dir_);
+std::string MojoEnv::RegisterDirectory(

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

This function/class is not thread safe (dirs_), does it need to be?

+    filesystem::mojom::DirectoryPtr directory) {
+  LevelDBMojoProxy::OpaqueDir* dir =
+      thread_->RegisterDirectory(std::move(directory));
+  int dir_id = dirs_.Add(dir);
+  return base::StringPrintf("%d:", dir_id);
+}
+
+void MojoEnv::UnregisterDirectory(base::StringPiece prefix) {
+  DCHECK(!prefix.empty());
+  DCHECK_EQ(prefix.back(), ':');
+  prefix.remove_suffix(1);
+  int dir_id;
+  bool result = base::StringToInt(prefix, &dir_id);
+  DCHECK(result) << "Invalid directory id '" << prefix << "'";
+  LevelDBMojoProxy::OpaqueDir* dir = dirs_.Lookup(dir_id);
+  DCHECK(dir) << "Invalid directory id " << dir_id;
+  dirs_.Remove(dir_id);
+  thread_->UnregisterDirectory(dir);
 }
 
 Status MojoEnv::NewSequentialFile(const std::string& fname,
                                   SequentialFile** result) {
   TRACE_EVENT1("leveldb", "MojoEnv::NewSequentialFile", "fname", fname);
+  LevelDBMojoProxy::OpaqueDir* dir;
+  std::string path;
+  ParsePath(fname, &dir, &path);
+
   base::File f = thread_->OpenFileHandle(
-      dir_, fname, filesystem::mojom::kFlagOpen | filesystem::mojom::kFlagRead);
+      dir, path, filesystem::mojom::kFlagOpen | filesystem::mojom::kFlagRead);
   if (!f.IsValid()) {
     *result = nullptr;
     return MakeIOError(fname, "Unable to create sequential file",
@@ -244,105 +279,152 @@ Status MojoEnv::NewSequentialFile(const std::string& fname,
 Status MojoEnv::NewRandomAccessFile(const std::string& fname,
                                     RandomAccessFile** result) {
   TRACE_EVENT1("leveldb", "MojoEnv::NewRandomAccessFile", "fname", fname);
+  LevelDBMojoProxy::OpaqueDir* dir;
+  std::string path;
+  ParsePath(fname, &dir, &path);
+
   base::File f = thread_->OpenFileHandle(
-      dir_, fname, filesystem::mojom::kFlagRead | filesystem::mojom::kFlagOpen);
+      dir, path, filesystem::mojom::kFlagRead | filesystem::mojom::kFlagOpen);
   if (!f.IsValid()) {
     *result = nullptr;
     base::File::Error error_code = f.error_details();
-    return MakeIOError(fname, FileErrorString(error_code),
+    return MakeIOError(path, FileErrorString(error_code),
                        leveldb_env::kNewRandomAccessFile, error_code);
   }
 
-  *result = new MojoRandomAccessFile(fname, std::move(f));
+  *result = new MojoRandomAccessFile(path, std::move(f));
   return Status::OK();
 }
 
 Status MojoEnv::NewWritableFile(const std::string& fname,
                                 WritableFile** result) {
   TRACE_EVENT1("leveldb", "MojoEnv::NewWritableFile", "fname", fname);
-  base::File f =
-      thread_->OpenFileHandle(dir_, fname, filesystem::mojom::kCreateAlways |
-                                               filesystem::mojom::kFlagWrite);
+  LevelDBMojoProxy::OpaqueDir* dir;
+  std::string path;
+  ParsePath(fname, &dir, &path);
+
+  base::File f = thread_->OpenFileHandle(
+      dir, path,
+      filesystem::mojom::kCreateAlways | filesystem::mojom::kFlagWrite);
   if (!f.IsValid()) {
     *result = nullptr;
-    return MakeIOError(fname, "Unable to create writable file",
+    return MakeIOError(path, "Unable to create writable file",
                        leveldb_env::kNewWritableFile, f.error_details());
   }
 
-  *result = new MojoWritableFile(dir_, fname, std::move(f), thread_);
+  *result = new MojoWritableFile(dir, path, std::move(f), thread_);
   return Status::OK();
 }
 
 Status MojoEnv::NewAppendableFile(const std::string& fname,
                                   WritableFile** result) {
   TRACE_EVENT1("leveldb", "MojoEnv::NewAppendableFile", "fname", fname);
-  base::File f =
-      thread_->OpenFileHandle(dir_, fname, filesystem::mojom::kFlagOpenAlways |
-                                               filesystem::mojom::kFlagAppend);
+  LevelDBMojoProxy::OpaqueDir* dir;
+  std::string path;
+  ParsePath(fname, &dir, &path);
+
+  base::File f = thread_->OpenFileHandle(
+      dir, path,
+      filesystem::mojom::kFlagOpenAlways | filesystem::mojom::kFlagAppend);
   if (!f.IsValid()) {
     *result = nullptr;
-    return MakeIOError(fname, "Unable to create appendable file",
+    return MakeIOError(path, "Unable to create appendable file",
                        leveldb_env::kNewAppendableFile, f.error_details());
   }
 
-  *result = new MojoWritableFile(dir_, fname, std::move(f), thread_);
+  *result = new MojoWritableFile(dir, path, std::move(f), thread_);
   return Status::OK();
 }
 
 bool MojoEnv::FileExists(const std::string& fname) {
   TRACE_EVENT1("leveldb", "MojoEnv::FileExists", "fname", fname);
-  return thread_->FileExists(dir_, fname);
+  LevelDBMojoProxy::OpaqueDir* dir;
+  std::string path;
+  ParsePath(fname, &dir, &path);
+
+  return thread_->FileExists(dir, path);
 }
 
-Status MojoEnv::GetChildren(const std::string& path,
+Status MojoEnv::GetChildren(const std::string& dirname,
                             std::vector<std::string>* result) {
-  TRACE_EVENT1("leveldb", "MojoEnv::GetChildren", "path", path);
-  return FilesystemErrorToStatus(thread_->GetChildren(dir_, path, result), path,
+  TRACE_EVENT1("leveldb", "MojoEnv::GetChildren", "dirname", dirname);
+  LevelDBMojoProxy::OpaqueDir* dir;
+  std::string path;
+  ParsePath(dirname, &dir, &path);
+
+  return FilesystemErrorToStatus(thread_->GetChildren(dir, path, result), path,
                                  leveldb_env::kGetChildren);
 }
 
 Status MojoEnv::DeleteFile(const std::string& fname) {
   TRACE_EVENT1("leveldb", "MojoEnv::DeleteFile", "fname", fname);
-  return FilesystemErrorToStatus(thread_->Delete(dir_, fname, 0), fname,
+  LevelDBMojoProxy::OpaqueDir* dir;
+  std::string path;
+  ParsePath(fname, &dir, &path);
+
+  return FilesystemErrorToStatus(thread_->Delete(dir, path, 0), path,
                                  leveldb_env::kDeleteFile);
 }
 
 Status MojoEnv::CreateDir(const std::string& dirname) {
   TRACE_EVENT1("leveldb", "MojoEnv::CreateDir", "dirname", dirname);
-  return FilesystemErrorToStatus(thread_->CreateDir(dir_, dirname), dirname,
+  LevelDBMojoProxy::OpaqueDir* dir;
+  std::string path;
+  ParsePath(dirname, &dir, &path);
+
+  return FilesystemErrorToStatus(thread_->CreateDir(dir, path), path,
                                  leveldb_env::kCreateDir);
 }
 
 Status MojoEnv::DeleteDir(const std::string& dirname) {
   TRACE_EVENT1("leveldb", "MojoEnv::DeleteDir", "dirname", dirname);
+  LevelDBMojoProxy::OpaqueDir* dir;
+  std::string path;
+  ParsePath(dirname, &dir, &path);
+
   return FilesystemErrorToStatus(
-      thread_->Delete(dir_, dirname, filesystem::mojom::kDeleteFlagRecursive),
-      dirname, leveldb_env::kDeleteDir);
+      thread_->Delete(dir, path, filesystem::mojom::kDeleteFlagRecursive), path,
+      leveldb_env::kDeleteDir);
 }
 
 Status MojoEnv::GetFileSize(const std::string& fname, uint64_t* file_size) {
   TRACE_EVENT1("leveldb", "MojoEnv::GetFileSize", "fname", fname);
-  return FilesystemErrorToStatus(thread_->GetFileSize(dir_, fname, file_size),
-                                 fname,
-                                 leveldb_env::kGetFileSize);
+  LevelDBMojoProxy::OpaqueDir* dir;
+  std::string path;
+  ParsePath(fname, &dir, &path);
+
+  return FilesystemErrorToStatus(thread_->GetFileSize(dir, path, file_size),
+                                 path, leveldb_env::kGetFileSize);
 }
 
 Status MojoEnv::RenameFile(const std::string& src, const std::string& target) {
   TRACE_EVENT2("leveldb", "MojoEnv::RenameFile", "src", src, "target", target);
-  return FilesystemErrorToStatus(thread_->RenameFile(dir_, src, target), src,
-                                 leveldb_env::kRenameFile);
+  LevelDBMojoProxy::OpaqueDir* src_dir;
+  std::string src_path;
+  ParsePath(src, &src_dir, &src_path);
+  LevelDBMojoProxy::OpaqueDir* target_dir;
+  std::string target_path;
+  ParsePath(target, &target_dir, &target_path);
+  DCHECK_EQ(src_dir, target_dir);
+
+  return FilesystemErrorToStatus(
+      thread_->RenameFile(src_dir, src_path, target_path), src_path,
+      leveldb_env::kRenameFile);
 }
 
 Status MojoEnv::LockFile(const std::string& fname, FileLock** lock) {
   TRACE_EVENT1("leveldb", "MojoEnv::LockFile", "fname", fname);
+  LevelDBMojoProxy::OpaqueDir* dir;
+  std::string path;
+  ParsePath(fname, &dir, &path);
 
   std::pair<filesystem::mojom::FileError, LevelDBMojoProxy::OpaqueLock*> p =
-      thread_->LockFile(dir_, fname);
+      thread_->LockFile(dir, path);
 
   if (p.second)
     *lock = new MojoFileLock(p.second, fname);
 
-  return FilesystemErrorToStatus(p.first, fname, leveldb_env::kLockFile);
+  return FilesystemErrorToStatus(p.first, path, leveldb_env::kLockFile);
 }
 
 Status MojoEnv::UnlockFile(FileLock* lock) {
@@ -368,12 +450,16 @@ Status MojoEnv::GetTestDirectory(std::string* path) {
 
 Status MojoEnv::NewLogger(const std::string& fname, Logger** result) {
   TRACE_EVENT1("leveldb", "MojoEnv::NewLogger", "fname", fname);
+  LevelDBMojoProxy::OpaqueDir* dir;
+  std::string path;
+  ParsePath(fname, &dir, &path);
+
   base::File f(thread_->OpenFileHandle(
-      dir_, fname,
+      dir, path,
       filesystem::mojom::kCreateAlways | filesystem::mojom::kFlagWrite));
   if (!f.IsValid()) {
     *result = NULL;
-    return MakeIOError(fname, "Unable to create log file",
+    return MakeIOError(path, "Unable to create log file",
                        leveldb_env::kNewLogger, f.error_details());
   } else {
     *result = new leveldb::ChromiumLogger(std::move(f));
@@ -381,4 +467,26 @@ Status MojoEnv::NewLogger(const std::string& fname, Logger** result) {
   }
 }
 
+void MojoEnv::ParsePath(const std::string& path,
+                        LevelDBMojoProxy::OpaqueDir** out_dir,
+                        std::string* out_path) {
+  size_t colon_pos = path.find(':');
+  DCHECK_NE(colon_pos, std::string::npos);
+
+  int dir_id;
+  bool result = base::StringToInt(
+      base::StringPiece(path.begin(), path.begin() + colon_pos), &dir_id);
+  DCHECK(result) << "Invalid directory id in path '" << path << "'";
+  *out_dir = dirs_.Lookup(dir_id);
+  DCHECK(*out_dir) << "Invalid directory id " << dir_id;
+
+  *out_path = std::string(path.begin() + colon_pos + 1, path.end());
+}
+
+MojoEnv::MojoEnv(scoped_refptr<base::SingleThreadTaskRunner> file_task_runner)
+    : ChromiumEnv("LevelDBEnv.Mojo"),
+      thread_(new LevelDBMojoProxy(std::move(file_task_runner))) {}
+
+MojoEnv::~MojoEnv() {}
+
 }  // namespace leveldb