[leveldb] Remove ChromiumEnv::BGThread

This is used to satisfy Env::Schedule calls. The implementation lazily
spins a new PlatformThread and maintains a queue of tasks for it to
run. LevelDB uses this to schedule work which may do file I/O. For the
sandboxed Storage Service, this means doing sync Mojo IPCs.

Mojo IPC in general requires some basic task scheduling state to be set
up on the calling thread though, so the Storage Service will DCHECK
when it tries to do compaction within a sandbox.

Fortunately this thread doesn't seem to be necessary anymore, and it can
be replaced with a much simpler dispatch to the ThreadPool. Env contract
does not require scheduled tasks to be serialized, so they can be thrown
at the ThreadPool without a sequenced task runner.

A test is added to the sandboxed Storage Service in order to provide
some (albeit roundabout) coverage of this code path.

Incidentally the test also revealed a bug in FilesystemProxy which
caused it to completely ignore its given IPC task runner, which can
cause deadlocks in certain situations. This is also fixed and
effectively covered by the new test.

Bug: 1052045
Change-Id: Ic299a4050240c1b52d3669964ff6b65f74de8179
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2341415Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#796599}

chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc

@@ -625,7 +625,7 @@ class MetadataDatabaseTest : public testing::TestWithParam<bool> {
 
  private:
   base::ScopedTempDir database_dir_;
-  base::test::SingleThreadTaskEnvironment task_environment_;
+  base::test::TaskEnvironment task_environment_;
 
   std::unique_ptr<leveldb::Env> in_memory_env_;
   std::unique_ptr<MetadataDatabase> metadata_database_;

components/leveldb_proto/internal/proto_database_perftest.cc

@@ -23,6 +23,7 @@
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
+#include "base/test/task_environment.h"
 #include "base/threading/thread.h"
 #include "build/build_config.h"
 #include "components/leveldb_proto/internal/leveldb_database.h"
@@ -558,7 +559,7 @@ class ProtoDBPerfTest : public testing::Test {
 
   std::map<std::string, std::unique_ptr<ScopedTempDir>> temp_dirs_;
   std::map<std::string, std::unique_ptr<TestDatabase>> dbs_;
-  base::test::SingleThreadTaskEnvironment task_environment_;
+  base::test::TaskEnvironment task_environment_;
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
 };
 

components/services/storage/BUILD.gn

@@ -28,8 +28,6 @@ source_set("storage") {
     "dom_storage/session_storage_namespace_impl.h",
     "dom_storage/storage_area_impl.cc",
     "dom_storage/storage_area_impl.h",
-    "filesystem_proxy_factory.cc",
-    "filesystem_proxy_factory.h",
     "indexed_db/leveldb/leveldb_factory.cc",
     "indexed_db/leveldb/leveldb_factory.h",
     "indexed_db/leveldb/leveldb_state.cc",
@@ -88,11 +86,26 @@ source_set("storage") {
   ]
 
   deps = [
+    ":filesystem_proxy_factory",
     "//components/services/storage/public/cpp",
     "//storage/common",
   ]
 }
 
+# This is its own component target because it exposes global state to multiple
+# independent libraries in component builds.
+component("filesystem_proxy_factory") {
+  sources = [
+    "filesystem_proxy_factory.cc",
+    "filesystem_proxy_factory.h",
+  ]
+  public_deps = [
+    "//base",
+    "//components/services/storage/public/cpp/filesystem",
+  ]
+  defines = [ "IS_FILESYSTEM_PROXY_FACTORY_IMPL" ]
+}
+
 # A separate component target which merely defines storage for a global testing
 # API binder implementations to be injected. No actual testing logic should be
 # included in this target.

components/services/storage/filesystem_proxy_factory.h

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "base/callback.h"
+#include "base/component_export.h"
 #include "components/services/storage/public/cpp/filesystem/filesystem_proxy.h"
 
 namespace storage {
@@ -16,6 +17,7 @@ namespace storage {
 // must be safe to call from any thread.
 using FilesystemProxyFactory =
     base::RepeatingCallback<std::unique_ptr<FilesystemProxy>()>;
+COMPONENT_EXPORT(FILESYSTEM_PROXY_FACTORY)
 void SetFilesystemProxyFactory(FilesystemProxyFactory factory);
 
 // Produces a new FilesystemProxy suitable for use in the service's current
@@ -24,6 +26,7 @@ void SetFilesystemProxyFactory(FilesystemProxyFactory factory);
 // access any path. If the service is sandboxed, this will produce a restricted
 // FilesystemProxy which can only traverse a limited scope of filesystem, and
 // only through IPC to a more privileged process.
+COMPONENT_EXPORT(FILESYSTEM_PROXY_FACTORY)
 std::unique_ptr<FilesystemProxy> CreateFilesystemProxy();
 
 }  // namespace storage

components/services/storage/public/cpp/filesystem/filesystem_proxy.cc

@@ -89,7 +89,7 @@ FilesystemProxy::FilesystemProxy(
     scoped_refptr<base::SequencedTaskRunner> ipc_task_runner)
     : root_(root),
       num_root_components_(GetNumPathComponents(root_)),
-      remote_directory_(std::move(directory)) {
+      remote_directory_(std::move(directory), ipc_task_runner) {
   DCHECK(root_.IsAbsolute());
 }
 

components/services/storage/public/mojom/test_api.test-mojom

@@ -9,4 +9,9 @@ module storage.mojom;
 interface TestApi {
   // Tells the service to crash its process immediately.
   CrashNow();
+
+  // Tells the service to create a new LevelDB database and force a compaction.
+  // This exercises an interesting code path in the sandboxed service which we
+  // want tests to cover.
+  [Sync] ForceLeveldbDatabaseCompaction(string name) => ();
 };

components/services/storage/test_api/BUILD.gn

@@ -12,8 +12,10 @@ source_set("test_api") {
 
   deps = [
     "//base",
+    "//components/services/storage:filesystem_proxy_factory",
     "//components/services/storage:test_api_stubs",
     "//components/services/storage/public/mojom:test_api",
     "//mojo/public/cpp/bindings",
+    "//third_party/leveldatabase",
   ]
 }

components/services/storage/test_api/test_api.cc

@@ -5,16 +5,51 @@
 #include "components/services/storage/test_api/test_api.h"
 
 #include "base/bind.h"
+#include "base/check.h"
 #include "base/immediate_crash.h"
 #include "base/no_destructor.h"
+#include "base/task/task_traits.h"
+#include "base/task/thread_pool.h"
+#include "base/threading/sequenced_task_runner_handle.h"
+#include "components/services/storage/filesystem_proxy_factory.h"
 #include "components/services/storage/public/mojom/test_api.test-mojom.h"
 #include "components/services/storage/test_api_stubs.h"
 #include "mojo/public/cpp/bindings/receiver_set.h"
+#include "third_party/leveldatabase/env_chromium.h"
+#include "third_party/leveldatabase/src/include/leveldb/db.h"
 
 namespace storage {
 
 namespace {
 
+class TestApiDatabaseEnv : public leveldb_env::ChromiumEnv {
+ public:
+  TestApiDatabaseEnv()
+      : ChromiumEnv("ChromiumEnv.TestApi", CreateFilesystemProxy()) {}
+  TestApiDatabaseEnv(const TestApiDatabaseEnv&) = delete;
+  TestApiDatabaseEnv& operator=(const TestApiDatabaseEnv&) = delete;
+};
+
+TestApiDatabaseEnv* GetTestApiDatabaseEnv() {
+  static base::NoDestructor<TestApiDatabaseEnv> env;
+  return env.get();
+}
+
+void CreateAndCompactDatabase(
+    const std::string& name,
+    scoped_refptr<base::SequencedTaskRunner> callback_task_runner,
+    mojom::TestApi::ForceLeveldbDatabaseCompactionCallback callback) {
+  leveldb_env::Options options;
+  options.create_if_missing = true;
+  options.env = GetTestApiDatabaseEnv();
+  std::unique_ptr<leveldb::DB> db;
+  leveldb::Status status = leveldb_env::OpenDB(options, name, &db);
+  CHECK(status.ok()) << status.ToString();
+  db->CompactRange(nullptr, nullptr);
+  db.reset();
+  callback_task_runner->PostTask(FROM_HERE, std::move(callback));
+}
+
 class TestApiImpl : public mojom::TestApi {
  public:
   TestApiImpl() = default;
@@ -27,6 +62,20 @@ class TestApiImpl : public mojom::TestApi {
   // mojom::TestApi implementation:
   void CrashNow() override { IMMEDIATE_CRASH(); }
 
+  void ForceLeveldbDatabaseCompaction(
+      const std::string& name,
+      ForceLeveldbDatabaseCompactionCallback callback) override {
+    // Note that we post to a SequencedTaskRunner because the task will use Mojo
+    // bindings, and by default Mojo bindings assume there is a current
+    // SequencedTaskRunnerHandle they can use for scheduling.
+    base::ThreadPool::CreateSequencedTaskRunner(
+        {base::MayBlock(), base::WithBaseSyncPrimitives()})
+        ->PostTask(FROM_HERE,
+                   base::BindOnce(&CreateAndCompactDatabase, name,
+                                  base::SequencedTaskRunnerHandle::Get(),
+                                  std::move(callback)));
+  }
+
  private:
   mojo::ReceiverSet<mojom::TestApi> receivers_;
 };

content/browser/BUILD.gn

@@ -77,6 +77,7 @@ source_set("browser") {
     "//components/services/filesystem:lib",
     "//components/services/quarantine:quarantine",
     "//components/services/storage",
+    "//components/services/storage:filesystem_proxy_factory",
     "//components/services/storage/dom_storage:local_storage_proto",
     "//components/services/storage/public/cpp",
     "//components/services/storage/public/mojom",

content/browser/storage_service_sandbox_browsertest.cc

@@ -19,6 +19,7 @@
 #include "content/public/test/content_browser_test.h"
 #include "content/public/test/content_browser_test_utils.h"
 #include "content/shell/browser/shell.h"
+#include "mojo/public/cpp/bindings/sync_call_restrictions.h"
 
 namespace content {
 namespace {
@@ -108,5 +109,14 @@ IN_PROC_BROWSER_TEST_F(StorageServiceSandboxBrowserTest, DomStorage) {
             EvalJs(shell()->web_contents(), R"(window.localStorage.yeet)"));
 }
 
+IN_PROC_BROWSER_TEST_F(StorageServiceSandboxBrowserTest, CompactDatabase) {
+  // Tests that the sandboxed service can execute a LevelDB database compaction
+  // operation without crashing. If the service crashes, the sync call below
+  // will return false.
+  mojo::ScopedAllowSyncCallForTesting allow_sync_calls;
+  EXPECT_TRUE(
+      GetTestApi()->ForceLeveldbDatabaseCompaction("CompactDatabaseTestDb"));
+}
+
 }  // namespace
 }  // namespace content

third_party/leveldatabase/env_chromium.cc

@@ -24,6 +24,8 @@
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/system/sys_info.h"
+#include "base/task/task_traits.h"
+#include "base/task/thread_pool.h"
 #include "base/threading/scoped_blocking_call.h"
 #include "base/time/time_override.h"
 #include "base/trace_event/memory_dump_manager.h"
@@ -700,14 +702,9 @@ ChromiumEnv::ChromiumEnv(const std::string& name)
 
 ChromiumEnv::ChromiumEnv(const std::string& name,
                          std::unique_ptr<storage::FilesystemProxy> filesystem)
-    : filesystem_(std::move(filesystem)),
-      name_(name),
-      bgsignal_(&mu_),
-      started_bgthread_(false) {
+    : filesystem_(std::move(filesystem)), name_(name) {
   DCHECK(filesystem_);
 
-  bgsignal_.declare_only_used_while_idle();
-
   size_t max_open_files = base::GetMaxFds();
   if (base::FeatureList::IsEnabled(kLevelDBFileHandleEviction) &&
       max_open_files < kFileLimitToDisableEviction) {
@@ -1041,46 +1038,9 @@ class Thread : public base::PlatformThread::Delegate {
 };
 
 void ChromiumEnv::Schedule(ScheduleFunc* function, void* arg) {
-  mu_.Acquire();
-
-  // Start background thread if necessary
-  if (!started_bgthread_) {
-    started_bgthread_ = true;
-    StartThread(&ChromiumEnv::BGThreadWrapper, this);
-  }
-
-  // If the queue is currently empty, the background thread may currently be
-  // waiting.
-  if (queue_.empty()) {
-    bgsignal_.Signal();
-  }
-
-  // Add to priority queue
-  queue_.push_back(BGItem());
-  queue_.back().function = function;
-  queue_.back().arg = arg;
-
-  mu_.Release();
-}
-
-void ChromiumEnv::BGThread() {
-  base::PlatformThread::SetName(name_.c_str());
-
-  while (true) {
-    // Wait until there is an item that is ready to run
-    mu_.Acquire();
-    while (queue_.empty()) {
-      bgsignal_.Wait();
-    }
-
-    void (*function)(void*) = queue_.front().function;
-    void* arg = queue_.front().arg;
-    queue_.pop_front();
-
-    mu_.Release();
-    TRACE_EVENT0("leveldb", "ChromiumEnv::BGThread-Task");
-    (*function)(arg);
-  }
+  base::ThreadPool::PostTask(FROM_HERE,
+                             {base::MayBlock(), base::WithBaseSyncPrimitives()},
+                             base::BindOnce(function, arg));
 }
 
 void ChromiumEnv::StartThread(void (*function)(void* arg), void* arg) {

third_party/leveldatabase/env_chromium.h

@@ -197,30 +197,12 @@ class LEVELDB_EXPORT ChromiumEnv : public leveldb::Env {
  private:
   void RemoveBackupFiles(const base::FilePath& dir);
 
-  // BGThread() is the body of the background thread
-  void BGThread();
-  static void BGThreadWrapper(void* arg) {
-    reinterpret_cast<ChromiumEnv*>(arg)->BGThread();
-  }
-
-
   const std::unique_ptr<storage::FilesystemProxy> filesystem_;
 
-  base::FilePath test_directory_;
-
-  std::string name_;
-
   base::Lock mu_;
-  base::ConditionVariable bgsignal_;
-  bool started_bgthread_;
+  base::FilePath test_directory_ GUARDED_BY(mu_);
 
-  // Entry per Schedule() call
-  struct BGItem {
-    void* arg;
-    void (*function)(void*);
-  };
-  using BGQueue = base::circular_deque<BGItem>;
-  BGQueue queue_;
+  std::string name_;
   std::unique_ptr<leveldb::Cache> file_cache_;
 };