Fix lifetime of leveldb::MojoEnv instances.

components/leveldb/env_mojo.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/env_mojo.h"
 
 #include <errno.h>
 
 #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"
 
 namespace leveldb {
 
 namespace {
 
 const base::FilePath::CharType table_extension[] = FILE_PATH_LITERAL(".ldb");
 
@@ skipping 186 matching lines @@
   std::string filename_;
   base::File file_;
   Type file_type_;
   LevelDBMojoProxy::OpaqueDir* dir_;
   std::string parent_dir_;
   scoped_refptr<LevelDBMojoProxy> thread_;
 
   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",
                        leveldb_env::kNewSequentialFile, f.error_details());
   }
 
   *result = new MojoSequentialFile(fname, std::move(f));
   return Status::OK();
 }
 
 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) {
   MojoFileLock* my_lock = reinterpret_cast<MojoFileLock*>(lock);
 
   std::string fname = my_lock ? my_lock->name() : "(invalid)";
   TRACE_EVENT1("leveldb", "MojoEnv::UnlockFile", "fname", fname);
 
   filesystem::mojom::FileError err = thread_->UnlockFile(my_lock->TakeLock());
   delete my_lock;
   return FilesystemErrorToStatus(err, fname, leveldb_env::kUnlockFile);
 }
 
 Status MojoEnv::GetTestDirectory(std::string* path) {
   // TODO(erg): This method is actually only used from the test harness in
   // leveldb. And when we go and port that test stuff to a
   // service_manager::ServiceTest,
   // we probably won't use it since the mojo filesystem actually handles
   // temporary filesystems just fine.
   NOTREACHED();
   return Status::OK();
 }
 
 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));
     return Status::OK();
   }
 }
 
+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