Let IndexedDBContextImpl create its own task runner

content/browser/indexed_db/indexed_db_unittest.cc

 // Copyright (c) 2012 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 <stdint.h>
 #include <utility>
 
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
-#include "base/test/test_simple_task_runner.h"
-#include "base/threading/thread.h"
-#include "base/threading/thread_task_runner_handle.h"
+#include "base/test/scoped_task_environment.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "content/browser/indexed_db/indexed_db_connection.h"
 #include "content/browser/indexed_db/indexed_db_context_impl.h"
 #include "content/browser/indexed_db/indexed_db_factory_impl.h"
 #include "content/browser/indexed_db/mock_indexed_db_callbacks.h"
 #include "content/browser/indexed_db/mock_indexed_db_database_callbacks.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/common/url_constants.h"
 #include "content/public/test/test_browser_context.h"
 #include "content/public/test/test_browser_thread_bundle.h"
+#include "content/public/test/test_utils.h"
 #include "storage/browser/quota/quota_manager.h"
 #include "storage/browser/quota/special_storage_policy.h"
 #include "storage/browser/test/mock_quota_manager_proxy.h"
 #include "storage/browser/test/mock_special_storage_policy.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/origin.h"
 
 using url::Origin;
 
 namespace content {
 
 class IndexedDBTest : public testing::Test {
  public:
   const Origin kNormalOrigin;
   const Origin kSessionOnlyOrigin;
 
   IndexedDBTest()
       : kNormalOrigin(GURL("http://normal/")),
         kSessionOnlyOrigin(GURL("http://session-only/")),
-        task_runner_(new base::TestSimpleTaskRunner),
+        scoped_task_environment_(
+            base::test::ScopedTaskEnvironment::MainThreadType::UI),
         special_storage_policy_(new MockSpecialStoragePolicy),
         quota_manager_proxy_(new MockQuotaManagerProxy(nullptr, nullptr)) {
     special_storage_policy_->AddSessionOnly(kSessionOnlyOrigin.GetURL());
   }
   ~IndexedDBTest() override {
     quota_manager_proxy_->SimulateQuotaManagerDestroyed();
   }
 
  protected:
-  void FlushIndexedDBTaskRunner() { task_runner_->RunUntilIdle(); }
-
-  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
   scoped_refptr<MockSpecialStoragePolicy> special_storage_policy_;
   scoped_refptr<MockQuotaManagerProxy> quota_manager_proxy_;
 
  private:
   TestBrowserThreadBundle thread_bundle_;

[gab, 2017/06/13 15:31:23]

TestBrowserThreadBundle includes a base::test::ScopedTaskEnvironment, you don't
need to mix both.

(and TestBrowserThreadBundle provides a main thread of type UI by default as
well)

TestBrowserThreadBundle/ScopedTaskEnvironment typically need to be the first
member though so that might be why you were hitting issues before, just move it
to 55 instead of ScopedTaskEnvironment and it should work.

Regarding running-until-idle. 

ScopedTaskEnvironment -> ScopedTaskEnvironment::RunUntilIdle()
TestBrowserThreadBundle -> RunAllBlockingPoolTasksUntilIdle()

you shouldn't need to mix both within a given file, they're equivalent (and we
should eventually just have TestBrowserThreadBundle::RunUntilIdle() to coalesce
the APIs but RunAllBlockingPoolTasksUntilIdle() is old stuff so need to live
with it for now...)

 
   DISALLOW_COPY_AND_ASSIGN(IndexedDBTest);
 };
 
 TEST_F(IndexedDBTest, ClearSessionOnlyDatabases) {
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
 
   base::FilePath normal_path;
   base::FilePath session_only_path;
 
   // Create the scope which will ensure we run the destructor of the context
   // which should trigger the clean up.
   {
     scoped_refptr<IndexedDBContextImpl> idb_context = new IndexedDBContextImpl(
         temp_dir.GetPath(), special_storage_policy_.get(),
-        quota_manager_proxy_.get(), task_runner_.get());
+        quota_manager_proxy_.get());
 
     normal_path = idb_context->GetFilePathForTesting(kNormalOrigin);
     session_only_path = idb_context->GetFilePathForTesting(kSessionOnlyOrigin);
     ASSERT_TRUE(base::CreateDirectory(normal_path));
     ASSERT_TRUE(base::CreateDirectory(session_only_path));
-    FlushIndexedDBTaskRunner();
-    base::RunLoop().RunUntilIdle();
+    RunAllBlockingPoolTasksUntilIdle();
     quota_manager_proxy_->SimulateQuotaManagerDestroyed();
   }
 
-  FlushIndexedDBTaskRunner();
-  base::RunLoop().RunUntilIdle();
+  RunAllBlockingPoolTasksUntilIdle();
 
   EXPECT_TRUE(base::DirectoryExists(normal_path));
   EXPECT_FALSE(base::DirectoryExists(session_only_path));
 }
 
 TEST_F(IndexedDBTest, SetForceKeepSessionState) {
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
 
   base::FilePath normal_path;
   base::FilePath session_only_path;
 
   // Create the scope which will ensure we run the destructor of the context.
   {
     // Create some indexedDB paths.
     // With the levelDB backend, these are directories.
     scoped_refptr<IndexedDBContextImpl> idb_context = new IndexedDBContextImpl(
         temp_dir.GetPath(), special_storage_policy_.get(),
-        quota_manager_proxy_.get(), task_runner_.get());
+        quota_manager_proxy_.get());
 
     // Save session state. This should bypass the destruction-time deletion.
     idb_context->SetForceKeepSessionState();
 
     normal_path = idb_context->GetFilePathForTesting(kNormalOrigin);
     session_only_path = idb_context->GetFilePathForTesting(kSessionOnlyOrigin);
     ASSERT_TRUE(base::CreateDirectory(normal_path));
     ASSERT_TRUE(base::CreateDirectory(session_only_path));
     base::RunLoop().RunUntilIdle();
   }
 
   // Make sure we wait until the destructor has run.
   base::RunLoop().RunUntilIdle();
 
   // No data was cleared because of SetForceKeepSessionState.
   EXPECT_TRUE(base::DirectoryExists(normal_path));
   EXPECT_TRUE(base::DirectoryExists(session_only_path));
 }
 
 class ForceCloseDBCallbacks : public IndexedDBCallbacks {
  public:
   ForceCloseDBCallbacks(scoped_refptr<IndexedDBContextImpl> idb_context,
                         const Origin& origin)
-      : IndexedDBCallbacks(nullptr,
-                           origin,
-                           nullptr,
-                           base::ThreadTaskRunnerHandle::Get()),
+      : IndexedDBCallbacks(nullptr, origin, nullptr, idb_context->TaskRunner()),
         idb_context_(idb_context),
         origin_(origin) {}
 
   void OnSuccess() override {}
   void OnSuccess(const std::vector<base::string16>&) override {}
   void OnSuccess(std::unique_ptr<IndexedDBConnection> connection,
                  const IndexedDBDatabaseMetadata& metadata) override {
     connection_ = std::move(connection);
     idb_context_->ConnectionOpened(origin_, connection_.get());
   }
@@ skipping 22 matching lines @@
   base::FilePath test_path;
 
   // Create the scope which will ensure we run the destructor of the context.
   {
     TestBrowserContext browser_context;
 
     const Origin kTestOrigin(GURL("http://test/"));
 
     scoped_refptr<IndexedDBContextImpl> idb_context = new IndexedDBContextImpl(
         temp_dir.GetPath(), special_storage_policy_.get(),
-        quota_manager_proxy_.get(), task_runner_.get());
+        quota_manager_proxy_.get());
+
+    // Assign current task runner so that methods on context can be invoked
+    // directly from test body.
+    idb_context->SetTaskRunnerForTesting(
+        base::SequencedTaskRunnerHandle::Get());

[gab, 2017/06/13 15:31:23]

A cleaner alternative to doing this would be to post your entire test body which
needs to run on |task_runner| as a lambda and then run until idle.

That way you properly create a multi-threaded test where things run as expected
(and highlight races for TSAN if there are any in the product).

i.e.:

instead of 

TEST(...) {
  idb_context->Foo();
  idb_context->Bar();
}

you could:

TEST(...) {
  idb_context->TaskRunner()->PostTask(
      base::Bind([](IndexedDBContextImpl* idb_context) {
          idb_context->Foo();
          idb_context->Bar();
      }, base::Unretained(idb_context)));

  run until idle...
}

 
     scoped_refptr<ForceCloseDBCallbacks> open_callbacks =
         new ForceCloseDBCallbacks(idb_context, kTestOrigin);
 
     scoped_refptr<ForceCloseDBCallbacks> closed_callbacks =
         new ForceCloseDBCallbacks(idb_context, kTestOrigin);
 
     IndexedDBFactory* factory = idb_context->GetIDBFactory();
 
     test_path = idb_context->GetFilePathForTesting(kTestOrigin);
@@ skipping 14 matching lines @@
                   idb_context->data_path());
 
     closed_callbacks->connection()->Close();
 
     // TODO(jsbell): Remove static_cast<> when overloads are eliminated.
     idb_context->TaskRunner()->PostTask(
         FROM_HERE,
         base::Bind(static_cast<void (IndexedDBContextImpl::*)(const Origin&)>(
                        &IndexedDBContextImpl::DeleteForOrigin),
                    idb_context, kTestOrigin));
-    FlushIndexedDBTaskRunner();
-    base::RunLoop().RunUntilIdle();
+
+    scoped_task_environment_.RunUntilIdle();
   }
 
   // Make sure we wait until the destructor has run.
-  base::RunLoop().RunUntilIdle();
+  scoped_task_environment_.RunUntilIdle();
 
   EXPECT_TRUE(open_db_callbacks->forced_close_called());
   EXPECT_FALSE(closed_db_callbacks->forced_close_called());
   EXPECT_FALSE(base::DirectoryExists(test_path));
 }
 
 TEST_F(IndexedDBTest, DeleteFailsIfDirectoryLocked) {
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
   const Origin kTestOrigin(GURL("http://test/"));
 
   scoped_refptr<IndexedDBContextImpl> idb_context = new IndexedDBContextImpl(
       temp_dir.GetPath(), special_storage_policy_.get(),
-      quota_manager_proxy_.get(), task_runner_.get());
+      quota_manager_proxy_.get());
 
   base::FilePath test_path = idb_context->GetFilePathForTesting(kTestOrigin);
   ASSERT_TRUE(base::CreateDirectory(test_path));
 
   std::unique_ptr<LevelDBLock> lock =
       LevelDBDatabase::LockForTesting(test_path);
   ASSERT_TRUE(lock);
 
   // TODO(jsbell): Remove static_cast<> when overloads are eliminated.
   void (IndexedDBContextImpl::* delete_for_origin)(const Origin&) =
       &IndexedDBContextImpl::DeleteForOrigin;
   idb_context->TaskRunner()->PostTask(
       FROM_HERE,
       base::Bind(delete_for_origin, idb_context, kTestOrigin));
-  FlushIndexedDBTaskRunner();
+  RunAllBlockingPoolTasksUntilIdle();
 
   EXPECT_TRUE(base::DirectoryExists(test_path));
 }
 
 TEST_F(IndexedDBTest, ForceCloseOpenDatabasesOnCommitFailure) {
   const Origin kTestOrigin(GURL("http://test/"));
 
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
 
   scoped_refptr<IndexedDBContextImpl> context = new IndexedDBContextImpl(
       temp_dir.GetPath(), special_storage_policy_.get(),
-      quota_manager_proxy_.get(), task_runner_.get());
+      quota_manager_proxy_.get());
+
+  // Assign current task runner so that methods on context can be invoked
+  // directly from test body.
+  context->SetTaskRunnerForTesting(base::SequencedTaskRunnerHandle::Get());
 
   scoped_refptr<IndexedDBFactoryImpl> factory =
       static_cast<IndexedDBFactoryImpl*>(context->GetIDBFactory());
 
   scoped_refptr<MockIndexedDBCallbacks> callbacks(new MockIndexedDBCallbacks());
   scoped_refptr<MockIndexedDBDatabaseCallbacks> db_callbacks(
       new MockIndexedDBDatabaseCallbacks());
   const int64_t transaction_id = 1;
   std::unique_ptr<IndexedDBPendingConnection> connection(
       base::MakeUnique<IndexedDBPendingConnection>(
@@ skipping 12 matching lines @@
 
   // Simulate the write failure.
   leveldb::Status status = leveldb::Status::IOError("Simulated failure");
   context->GetIDBFactory()->HandleBackingStoreFailure(kTestOrigin);
 
   EXPECT_TRUE(db_callbacks->forced_close_called());
   EXPECT_FALSE(factory->IsBackingStoreOpen(kTestOrigin));
 }
 
 }  // namespace content