SessionStorage: Deleting database on correct thread.

The SessionStorageDatabase is reference counted, and it is possible
for the last reference to be removed on a thread that disallows use
of sync primitives. This change ensures this its leveldb::DB is
deleted in the correct sequence.

Bug: 789040
Change-Id: I7b47b8104459003fc9836a350b0e13f9ed8628c1
Reviewed-on: https://chromium-review.googlesource.com/816038
Commit-Queue: Chris Mumford <cmumford@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527145}

content/browser/dom_storage/dom_storage_area_unittest.cc

@@ -241,8 +241,8 @@ TEST_P(DOMStorageAreaParamTest, ShallowCopyWithBacking) {
   base::ScopedTempDir temp_dir;
   const std::string kNamespaceId = "id1";
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
-  scoped_refptr<SessionStorageDatabase> db =
-      new SessionStorageDatabase(temp_dir.GetPath());
+  scoped_refptr<SessionStorageDatabase> db = new SessionStorageDatabase(
+      temp_dir.GetPath(), base::ThreadTaskRunnerHandle::Get());
   scoped_refptr<DOMStorageArea> area(new DOMStorageArea(
       1, kNamespaceId, nullptr, kOrigin, db.get(),
       new MockDOMStorageTaskRunner(base::ThreadTaskRunnerHandle::Get().get())));

content/browser/dom_storage/dom_storage_context_impl.cc

@@ -461,7 +461,8 @@ void DOMStorageContextImpl::SetSaveSessionStorageOnDisk() {
   DCHECK(namespaces_.empty());
   if (!sessionstorage_directory_.empty()) {
     session_storage_database_ = new SessionStorageDatabase(
-        sessionstorage_directory_);
+        sessionstorage_directory_, task_runner_->GetSequencedTaskRunner(
+                                       DOMStorageTaskRunner::COMMIT_SEQUENCE));
   }
 }
 

content/browser/dom_storage/session_storage_database.cc

@@ -100,16 +100,21 @@ class SessionStorageDatabase::DBOperation {
   SessionStorageDatabase* session_storage_database_;
 };
 
-
-SessionStorageDatabase::SessionStorageDatabase(const base::FilePath& file_path)
-    : file_path_(file_path),
+SessionStorageDatabase::SessionStorageDatabase(
+    const base::FilePath& file_path,
+    scoped_refptr<base::SequencedTaskRunner> commit_task_runner)
+    : RefCountedDeleteOnSequence<SessionStorageDatabase>(
+          std::move(commit_task_runner)),
+      file_path_(file_path),
       db_error_(false),
       is_inconsistent_(false),
       invalid_db_deleted_(false),
       operation_count_(0) {
+  DETACH_FROM_SEQUENCE(sequence_checker_);
 }
 
 SessionStorageDatabase::~SessionStorageDatabase() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 }
 
 void SessionStorageDatabase::ReadAreaValues(
@@ -163,6 +168,7 @@ bool SessionStorageDatabase::CommitAreaChanges(
     const GURL& origin,
     bool clear_all_first,
     const DOMStorageValuesMap& changes) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   // Even if |changes| is empty, we need to write the appropriate placeholders
   // in the database, so that it can be later shallow-copied successfully.
   if (!LazyOpen(true))
@@ -230,6 +236,7 @@ bool SessionStorageDatabase::CloneNamespace(
   // | namespace-2-                   | dummy               |
   // | namespace-2-origin1            | 1 (mapid) << references the same map
 
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (!LazyOpen(true))
     return false;
   DBOperation operation(this);
@@ -257,6 +264,7 @@ bool SessionStorageDatabase::CloneNamespace(
 
 bool SessionStorageDatabase::DeleteArea(const std::string& namespace_id,
                                         const GURL& origin) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (!LazyOpen(false)) {
     // No need to create the database if it doesn't exist.
     return true;
@@ -270,6 +278,7 @@ bool SessionStorageDatabase::DeleteArea(const std::string& namespace_id,
 }
 
 bool SessionStorageDatabase::DeleteNamespace(const std::string& namespace_id) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   {
     // The caller should have called other methods to open the DB before this
     // function. Otherwise, DB stores nothing interesting related to the

content/browser/dom_storage/session_storage_database.h

@@ -17,6 +17,9 @@
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/ref_counted_delete_on_sequence.h"
+#include "base/sequence_checker.h"
+#include "base/sequenced_task_runner.h"
 #include "base/synchronization/lock.h"
 #include "content/common/content_export.h"
 #include "content/common/dom_storage/dom_storage_types.h"
@@ -42,13 +45,18 @@ namespace content {
 // origins. All DOMStorageAreas for session storage share the same
 // SessionStorageDatabase.
 
-// Only one thread is allowed to call the public functions other than
-// ReadAreaValues and ReadNamespacesAndOrigins. Other threads are allowed to
-// call ReadAreaValues and ReadNamespacesAndOrigins.
-class CONTENT_EXPORT SessionStorageDatabase :
-    public base::RefCountedThreadSafe<SessionStorageDatabase> {
+// This class is not thread safe. Read-only methods (ReadAreaValues,
+// ReadNamespacesAndOrigins, and OnMemoryDump) may be called on any thread.
+// Methods that modify the database must be called on the same thread.
+class CONTENT_EXPORT SessionStorageDatabase
+    : public base::RefCountedDeleteOnSequence<SessionStorageDatabase> {
  public:
-  explicit SessionStorageDatabase(const base::FilePath& file_path);
+  // |file_path| is the path to the directory where the database will be
+  // created. |commit_task_runner| is the runner on which methods which modify
+  // the database must be run and where this object will be deleted.
+  SessionStorageDatabase(
+      const base::FilePath& file_path,
+      scoped_refptr<base::SequencedTaskRunner> commit_task_runner);
 
   // Reads the (key, value) pairs for |namespace_id| and |origin|. |result| is
   // assumed to be empty and any duplicate keys will be overwritten. If the
@@ -90,10 +98,11 @@ class CONTENT_EXPORT SessionStorageDatabase :
   void OnMemoryDump(base::trace_event::ProcessMemoryDump* pmd);
 
  private:
-  friend class base::RefCountedThreadSafe<SessionStorageDatabase>;
   class DBOperation;
   friend class SessionStorageDatabase::DBOperation;
   friend class SessionStorageDatabaseTest;
+  friend class base::RefCountedDeleteOnSequence<SessionStorageDatabase>;
+  friend class base::DeleteHelper<SessionStorageDatabase>;
   FRIEND_TEST_ALL_PREFIXES(DOMStorageAreaParamTest, ShallowCopyWithBacking);
 
   ~SessionStorageDatabase();
@@ -224,6 +233,9 @@ class CONTENT_EXPORT SessionStorageDatabase :
   // delete an inconsistent database at the right moment.
   int operation_count_;
 
+  // Used to check methods that run on the commit sequence.
+  SEQUENCE_CHECKER(sequence_checker_);
+
   DISALLOW_COPY_AND_ASSIGN(SessionStorageDatabase);
 };
 

content/browser/dom_storage/session_storage_database_unittest.cc

@@ -19,6 +19,8 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_task_environment.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "content/common/dom_storage/dom_storage_types.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/leveldatabase/src/include/leveldb/db.h"
@@ -63,6 +65,7 @@ class SessionStorageDatabaseTest : public testing::Test {
                             const GURL& origin) const;
   int64_t GetMapRefCount(const std::string& map_id) const;
 
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
   base::ScopedTempDir temp_dir_;
   scoped_refptr<SessionStorageDatabase> db_;
 
@@ -106,7 +109,8 @@ void SessionStorageDatabaseTest::SetUp() {
 }
 
 void SessionStorageDatabaseTest::ResetDatabase() {
-  db_ = new SessionStorageDatabase(temp_dir_.GetPath());
+  db_ = new SessionStorageDatabase(temp_dir_.GetPath(),
+                                   base::ThreadTaskRunnerHandle::Get());
   ASSERT_TRUE(db_->LazyOpen(true));
 }