IndexedDBFactory now ForceCloses databases.

Also, IndexedDBContextImpl used to maintain a map of open connections. This change also deletes this map, and instead relies on one which already exists in IndexedDBFactory. BUG=259564 Review URL: https://codereview.chromium.org/93873017 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@244240 0039d316-1c4b-4281-b951-d872f2087c98
author: cmumford@chromium.org <cmumford@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> 2014-01-10 21:11:10 +0000
committer: cmumford@chromium.org <cmumford@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> 2014-01-10 21:11:10 +0000
commit: 791c1228d13ebccee5d79aa5d4e96779ab1f0e91 (patch)
tree: 8012985818a878b8f5d4850ce12a769f13efd5d3 /content
parent: b2732a066c9ca0b106db440b3fd267befee2ea58 (diff)
download: chromium_src-791c1228d13ebccee5d79aa5d4e96779ab1f0e91.zip
chromium_src-791c1228d13ebccee5d79aa5d4e96779ab1f0e91.tar.gz
chromium_src-791c1228d13ebccee5d79aa5d4e96779ab1f0e91.tar.bz2
7 files changed, 133 insertions, 92 deletions
diff --git a/content/browser/indexed_db/indexed_db_context_impl.cc b/content/browser/indexed_db/indexed_db_context_impl.cc
index fc1c932..df99bf4 100644
--- a/content/browser/indexed_db/indexed_db_context_impl.cc
+++ b/content/browser/indexed_db/indexed_db_context_impl.cc
@@ -187,16 +187,17 @@ base::ListValue* IndexedDBContextImpl::GetAllOriginsDetails() {
     // origins in the outer loop.
 
     if (factory_) {
-      std::vector<IndexedDBDatabase*> databases =
+      std::pair<IndexedDBFactory::OriginDBMapIterator,
+                IndexedDBFactory::OriginDBMapIterator> range =
           factory_->GetOpenDatabasesForOrigin(origin_url);
       // TODO(jsbell): Sort by name?
       scoped_ptr<base::ListValue> database_list(new base::ListValue());
 
-      for (std::vector<IndexedDBDatabase*>::iterator it = databases.begin();
-           it != databases.end();
+      for (IndexedDBFactory::OriginDBMapIterator it = range.first;
+           it != range.second;
            ++it) {
 
-        const IndexedDBDatabase* db = *it;
+        const IndexedDBDatabase* db = it->second;
         scoped_ptr<base::DictionaryValue> db_info(new base::DictionaryValue());
 
         db_info->SetString("name", db->name());
@@ -336,21 +337,9 @@ void IndexedDBContextImpl::ForceClose(const GURL origin_url) {
   if (data_path_.empty() || !IsInOriginSet(origin_url))
     return;
 
-  if (connections_.find(origin_url) != connections_.end()) {
-    ConnectionSet& connections = connections_[origin_url];
-    ConnectionSet::iterator it = connections.begin();
-    while (it != connections.end()) {
-      // Remove before closing so callbacks don't double-erase
-      IndexedDBConnection* connection = *it;
-      DCHECK(connection->IsConnected());
-      connections.erase(it++);
-      connection->ForceClose();
-    }
-    DCHECK_EQ(connections_[origin_url].size(), 0UL);
-    connections_.erase(origin_url);
-  }
   if (factory_)
     factory_->ForceClose(origin_url);
+  DCHECK_EQ(0UL, GetConnectionCount(origin_url));
 }
 
 size_t IndexedDBContextImpl::GetConnectionCount(const GURL& origin_url) {
@@ -358,10 +347,10 @@ size_t IndexedDBContextImpl::GetConnectionCount(const GURL& origin_url) {
   if (data_path_.empty() || !IsInOriginSet(origin_url))
     return 0;
 
-  if (connections_.find(origin_url) == connections_.end())
+  if (!factory_)
     return 0;
 
-  return connections_[origin_url].size();
+  return factory_->GetConnectionCount(origin_url);
 }
 
 base::FilePath IndexedDBContextImpl::GetFilePath(const GURL& origin_url) const {
@@ -383,14 +372,12 @@ void IndexedDBContextImpl::SetTaskRunnerForTesting(
 void IndexedDBContextImpl::ConnectionOpened(const GURL& origin_url,
                                             IndexedDBConnection* connection) {
   DCHECK(TaskRunner()->RunsTasksOnCurrentThread());
-  DCHECK_EQ(connections_[origin_url].count(connection), 0UL);
   if (quota_manager_proxy()) {
     quota_manager_proxy()->NotifyStorageAccessed(
         quota::QuotaClient::kIndexedDatabase,
         origin_url,
         quota::kStorageTypeTemporary);
   }
-  connections_[origin_url].insert(connection);
   if (AddToOriginSet(origin_url)) {
     // A newly created db, notify the quota system.
     QueryDiskAndUpdateQuotaUsage(origin_url);
@@ -403,26 +390,18 @@ void IndexedDBContextImpl::ConnectionOpened(const GURL& origin_url,
 void IndexedDBContextImpl::ConnectionClosed(const GURL& origin_url,
                                             IndexedDBConnection* connection) {
   DCHECK(TaskRunner()->RunsTasksOnCurrentThread());
-  // May not be in the map if connection was forced to close
-  if (connections_.find(origin_url) == connections_.end() ||
-      connections_[origin_url].count(connection) != 1)
-    return;
   if (quota_manager_proxy()) {
     quota_manager_proxy()->NotifyStorageAccessed(
         quota::QuotaClient::kIndexedDatabase,
         origin_url,
         quota::kStorageTypeTemporary);
   }
-  connections_[origin_url].erase(connection);
-  if (connections_[origin_url].size() == 0) {
+  if (factory_ && factory_->GetConnectionCount(origin_url) == 0)
     QueryDiskAndUpdateQuotaUsage(origin_url);
-    connections_.erase(origin_url);
-  }
 }
 
 void IndexedDBContextImpl::TransactionComplete(const GURL& origin_url) {
-  DCHECK(connections_.find(origin_url) != connections_.end() &&
-         connections_[origin_url].size() > 0);
+  DCHECK(!factory_ || factory_->GetConnectionCount(origin_url) > 0);
   QueryDiskAndUpdateQuotaUsage(origin_url);
   QueryAvailableQuota(origin_url);
 }
diff --git a/content/browser/indexed_db/indexed_db_context_impl.h b/content/browser/indexed_db/indexed_db_context_impl.h
index 74e5e8c..f3b1185 100644
--- a/content/browser/indexed_db/indexed_db_context_impl.h
+++ b/content/browser/indexed_db/indexed_db_context_impl.h
@@ -137,8 +137,6 @@ class CONTENT_EXPORT IndexedDBContextImpl
   scoped_ptr<std::set<GURL> > origin_set_;
   OriginToSizeMap origin_size_map_;
   OriginToSizeMap space_available_map_;
-  typedef std::set<IndexedDBConnection*> ConnectionSet;
-  std::map<GURL, ConnectionSet> connections_;
 
   DISALLOW_COPY_AND_ASSIGN(IndexedDBContextImpl);
 };
diff --git a/content/browser/indexed_db/indexed_db_database.cc b/content/browser/indexed_db/indexed_db_database.cc
index ba53e66..3f7f98a 100644
--- a/content/browser/indexed_db/indexed_db_database.cc
+++ b/content/browser/indexed_db/indexed_db_database.cc
@@ -1589,6 +1589,17 @@ void IndexedDBDatabase::DeleteDatabaseFinal(
     factory_->DatabaseDeleted(identifier_);
 }
 
+void IndexedDBDatabase::ForceClose() {
+  // IndexedDBConnection::ForceClose() may delete this database, so hold ref.
+  scoped_refptr<IndexedDBDatabase> protect(this);
+  ConnectionSet::const_iterator it = connections_.begin();
+  while (it != connections_.end()) {
+    IndexedDBConnection* connection = *it++;
+    connection->ForceClose();
+  }
+  DCHECK(connections_.empty());
+}
+
 void IndexedDBDatabase::Close(IndexedDBConnection* connection, bool forced) {
   DCHECK(connections_.count(connection));
   DCHECK(connection->IsConnected());
diff --git a/content/browser/indexed_db/indexed_db_database.h b/content/browser/indexed_db/indexed_db_database.h
index 3a81b52..f5eb3e4 100644
--- a/content/browser/indexed_db/indexed_db_database.h
+++ b/content/browser/indexed_db/indexed_db_database.h
@@ -90,6 +90,7 @@ class CONTENT_EXPORT IndexedDBDatabase
                          const std::vector<int64>& object_store_ids,
                          uint16 mode);
   void Close(IndexedDBConnection* connection, bool forced);
+  void ForceClose();
 
   void Commit(int64 transaction_id);
   void Abort(int64 transaction_id);
diff --git a/content/browser/indexed_db/indexed_db_factory.cc b/content/browser/indexed_db/indexed_db_factory.cc
index 8cf5042..5540331 100644
--- a/content/browser/indexed_db/indexed_db_factory.cc
+++ b/content/browser/indexed_db/indexed_db_factory.cc
@@ -25,14 +25,32 @@ IndexedDBFactory::IndexedDBFactory(IndexedDBContextImpl* context)
 
 IndexedDBFactory::~IndexedDBFactory() {}
 
-void IndexedDBFactory::ReleaseDatabase(
-    const IndexedDBDatabase::Identifier& identifier,
-    bool forcedClose) {
+void IndexedDBFactory::RemoveDatabaseFromMaps(
+    const IndexedDBDatabase::Identifier& identifier) {
   IndexedDBDatabaseMap::iterator it = database_map_.find(identifier);
   DCHECK(it != database_map_.end());
-  DCHECK(!it->second->backing_store());
+  IndexedDBDatabase* database = it->second;
   database_map_.erase(it);
 
+  std::pair<OriginDBMap::iterator, OriginDBMap::iterator> range =
+      origin_dbs_.equal_range(database->identifier().first);
+  DCHECK(range.first != range.second);
+  for (OriginDBMap::iterator it2 = range.first; it2 != range.second; ++it2) {
+    if (it2->second == database) {
+      origin_dbs_.erase(it2);
+      break;
+    }
+  }
+}
+
+void IndexedDBFactory::ReleaseDatabase(
+    const IndexedDBDatabase::Identifier& identifier,
+    bool forcedClose) {
+
+  DCHECK(!database_map_.find(identifier)->second->backing_store());
+
+  RemoveDatabaseFromMaps(identifier);
+
   // No grace period on a forced-close, as the initiator is
   // assuming the backing store will be released once all
   // connections are closed.
@@ -92,6 +110,15 @@ bool IndexedDBFactory::HasLastBackingStoreReference(const GURL& origin_url)
 }
 
 void IndexedDBFactory::ForceClose(const GURL& origin_url) {
+  std::pair<OriginDBMapIterator, OriginDBMapIterator> range =
+      GetOpenDatabasesForOrigin(origin_url);
+
+  while (range.first != range.second) {
+    IndexedDBDatabase* db = range.first->second;
+    ++range.first;
+    db->ForceClose();
+  }
+
   if (backing_store_map_.find(origin_url) != backing_store_map_.end())
     ReleaseBackingStore(origin_url, true /* immediate */);
 }
@@ -183,8 +210,9 @@ void IndexedDBFactory::DeleteDatabase(
   }
 
   database_map_[unique_identifier] = database;
+  origin_dbs_.insert(std::make_pair(origin_url, database));
   database->DeleteDatabase(callbacks);
-  database_map_.erase(unique_identifier);
+  RemoveDatabaseFromMaps(unique_identifier);
   database = NULL;
   backing_store = NULL;
   ReleaseBackingStore(origin_url, false /* immediate */);
@@ -324,20 +352,27 @@ void IndexedDBFactory::Open(
   database->OpenConnection(
       callbacks, database_callbacks, transaction_id, version);
 
-  if (!was_open && database->ConnectionCount() > 0)
+  if (!was_open && database->ConnectionCount() > 0) {
     database_map_[unique_identifier] = database;
+    origin_dbs_.insert(std::make_pair(origin_url, database));
+  }
 }
 
-std::vector<IndexedDBDatabase*> IndexedDBFactory::GetOpenDatabasesForOrigin(
-    const GURL& origin_url) const {
-  std::vector<IndexedDBDatabase*> result;
-  for (IndexedDBDatabaseMap::const_iterator it = database_map_.begin();
-       it != database_map_.end();
-       ++it) {
-    if (it->first.first == origin_url)
-      result.push_back(it->second);
-  }
-  return result;
+std::pair<IndexedDBFactory::OriginDBMapIterator,
+          IndexedDBFactory::OriginDBMapIterator>
+IndexedDBFactory::GetOpenDatabasesForOrigin(const GURL& origin_url) const {
+  return origin_dbs_.equal_range(origin_url);
+}
+
+size_t IndexedDBFactory::GetConnectionCount(const GURL& origin_url) const {
+  size_t count(0);
+
+  std::pair<OriginDBMapIterator, OriginDBMapIterator> range =
+      GetOpenDatabasesForOrigin(origin_url);
+  for (OriginDBMapIterator it = range.first; it != range.second; ++it)
+    count += it->second->ConnectionCount();
+
+  return count;
 }
 
 }  // namespace content
diff --git a/content/browser/indexed_db/indexed_db_factory.h b/content/browser/indexed_db/indexed_db_factory.h
index ae0d936..e42d4fa 100644
--- a/content/browser/indexed_db/indexed_db_factory.h
+++ b/content/browser/indexed_db/indexed_db_factory.h
@@ -26,6 +26,9 @@ class IndexedDBContextImpl;
 class CONTENT_EXPORT IndexedDBFactory
     : NON_EXPORTED_BASE(public base::RefCountedThreadSafe<IndexedDBFactory>) {
  public:
+  typedef std::multimap<GURL, IndexedDBDatabase*> OriginDBMap;
+  typedef OriginDBMap::const_iterator OriginDBMapIterator;
+
   explicit IndexedDBFactory(IndexedDBContextImpl* context);
 
   // Notifications from weak pointers.
@@ -50,8 +53,7 @@ class CONTENT_EXPORT IndexedDBFactory
 
   void HandleBackingStoreFailure(const GURL& origin_url);
 
-  // Iterates over all databases; for diagnostics only.
-  std::vector<IndexedDBDatabase*> GetOpenDatabasesForOrigin(
+  std::pair<OriginDBMapIterator, OriginDBMapIterator> GetOpenDatabasesForOrigin(
       const GURL& origin_url) const;
 
   // Called by IndexedDBContext after all connections are closed, to
@@ -64,6 +66,8 @@ class CONTENT_EXPORT IndexedDBFactory
   // Called by an IndexedDBDatabase when it is actually deleted.
   void DatabaseDeleted(const IndexedDBDatabase::Identifier& identifier);
 
+  size_t GetConnectionCount(const GURL& origin_url) const;
+
  protected:
   friend class base::RefCountedThreadSafe<IndexedDBFactory>;
 
@@ -104,12 +108,14 @@ class CONTENT_EXPORT IndexedDBFactory
                       const base::string16& name) const;
   bool IsBackingStoreOpen(const GURL& origin_url) const;
   bool IsBackingStorePendingClose(const GURL& origin_url) const;
+  void RemoveDatabaseFromMaps(const IndexedDBDatabase::Identifier& identifier);
 
   IndexedDBContextImpl* context_;
 
   typedef std::map<IndexedDBDatabase::Identifier,
                    IndexedDBDatabase*> IndexedDBDatabaseMap;
   IndexedDBDatabaseMap database_map_;
+  OriginDBMap origin_dbs_;
 
   typedef std::map<GURL, scoped_refptr<IndexedDBBackingStore> >
       IndexedDBBackingStoreMap;
diff --git a/content/browser/indexed_db/indexed_db_unittest.cc b/content/browser/indexed_db/indexed_db_unittest.cc
index d0fe981..006436b 100644
--- a/content/browser/indexed_db/indexed_db_unittest.cc
+++ b/content/browser/indexed_db/indexed_db_unittest.cc
@@ -116,35 +116,44 @@ TEST_F(IndexedDBTest, SetForceKeepSessionState) {
   EXPECT_TRUE(base::DirectoryExists(session_only_path));
 }
 
-class MockConnection : public IndexedDBConnection {
+class ForceCloseDBCallbacks : public IndexedDBCallbacks {
  public:
-  explicit MockConnection(bool expect_force_close)
-      : IndexedDBConnection(NULL, NULL),
-        expect_force_close_(expect_force_close),
-        force_close_called_(false) {}
-
-  virtual ~MockConnection() {
-    EXPECT_TRUE(force_close_called_ == expect_force_close_);
+  ForceCloseDBCallbacks(scoped_refptr<IndexedDBContextImpl> idb_context,
+                        const GURL& origin_url)
+      : IndexedDBCallbacks(NULL, 0, 0),
+        idb_context_(idb_context),
+        origin_url_(origin_url),
+        connection_(NULL) {}
+
+  virtual void OnSuccess() OVERRIDE {}
+  virtual void OnSuccess(const std::vector<base::string16>&) OVERRIDE {}
+  virtual void OnSuccess(scoped_ptr<IndexedDBConnection> connection,
+                         const IndexedDBDatabaseMetadata& metadata) OVERRIDE {
+    connection_ = connection.release();
+    idb_context_->ConnectionOpened(origin_url_, connection_);
   }
 
-  virtual void ForceClose() OVERRIDE {
-    ASSERT_TRUE(expect_force_close_);
-    force_close_called_ = true;
-  }
+  IndexedDBConnection* connection() { return connection_; }
 
-  virtual bool IsConnected() OVERRIDE {
-    return !force_close_called_;
-  }
+ protected:
+  virtual ~ForceCloseDBCallbacks() {}
 
  private:
-  bool expect_force_close_;
-  bool force_close_called_;
+  scoped_refptr<IndexedDBContextImpl> idb_context_;
+  GURL origin_url_;
+  IndexedDBConnection* connection_;
+  DISALLOW_COPY_AND_ASSIGN(ForceCloseDBCallbacks);
 };
 
 TEST_F(IndexedDBTest, ForceCloseOpenDatabasesOnDelete) {
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
 
+  scoped_refptr<MockIndexedDBDatabaseCallbacks> open_db_callbacks(
+      new MockIndexedDBDatabaseCallbacks());
+  scoped_refptr<MockIndexedDBDatabaseCallbacks> closed_db_callbacks(
+      new MockIndexedDBDatabaseCallbacks());
+
   base::FilePath test_path;
 
   // Create the scope which will ensure we run the destructor of the context.
@@ -156,33 +165,33 @@ TEST_F(IndexedDBTest, ForceCloseOpenDatabasesOnDelete) {
     scoped_refptr<IndexedDBContextImpl> idb_context = new IndexedDBContextImpl(
         temp_dir.path(), special_storage_policy_, NULL, task_runner_);
 
-    test_path = idb_context->GetFilePathForTesting(
-        webkit_database::GetIdentifierFromOrigin(kTestOrigin));
-    ASSERT_TRUE(base::CreateDirectory(test_path));
+    scoped_refptr<ForceCloseDBCallbacks> open_callbacks =
+        new ForceCloseDBCallbacks(idb_context, kTestOrigin);
 
-    const bool kExpectForceClose = true;
+    scoped_refptr<ForceCloseDBCallbacks> closed_callbacks =
+        new ForceCloseDBCallbacks(idb_context, kTestOrigin);
 
-    MockConnection connection1(kExpectForceClose);
-    idb_context->TaskRunner()->PostTask(
-        FROM_HERE,
-        base::Bind(&IndexedDBContextImpl::ConnectionOpened,
-                   idb_context,
-                   kTestOrigin,
-                   &connection1));
+    IndexedDBFactory* factory = idb_context->GetIDBFactory();
 
-    MockConnection connection2(!kExpectForceClose);
-    idb_context->TaskRunner()->PostTask(
-        FROM_HERE,
-        base::Bind(&IndexedDBContextImpl::ConnectionOpened,
-                   idb_context,
-                   kTestOrigin,
-                   &connection2));
-    idb_context->TaskRunner()->PostTask(
-        FROM_HERE,
-        base::Bind(&IndexedDBContextImpl::ConnectionClosed,
-                   idb_context,
-                   kTestOrigin,
-                   &connection2));
+    test_path = idb_context->GetFilePathForTesting(
+        webkit_database::GetIdentifierFromOrigin(kTestOrigin));
+
+    factory->Open(base::ASCIIToUTF16("opendb"),
+                  0,
+                  0,
+                  open_callbacks,
+                  open_db_callbacks,
+                  kTestOrigin,
+                  idb_context->data_path());
+    factory->Open(base::ASCIIToUTF16("closeddb"),
+                  0,
+                  0,
+                  closed_callbacks,
+                  closed_db_callbacks,
+                  kTestOrigin,
+                  idb_context->data_path());
+
+    closed_callbacks->connection()->Close();
 
     idb_context->TaskRunner()->PostTask(
         FROM_HERE,
@@ -195,6 +204,8 @@ TEST_F(IndexedDBTest, ForceCloseOpenDatabasesOnDelete) {
   // Make sure we wait until the destructor has run.
   message_loop_.RunUntilIdle();
 
+  EXPECT_TRUE(open_db_callbacks->forced_close_called());
+  EXPECT_FALSE(closed_db_callbacks->forced_close_called());
   EXPECT_FALSE(base::DirectoryExists(test_path));
 }
author	cmumford@chromium.org <cmumford@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>	2014-01-10 21:11:10 +0000
committer	cmumford@chromium.org <cmumford@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>	2014-01-10 21:11:10 +0000
commit	791c1228d13ebccee5d79aa5d4e96779ab1f0e91 (patch)
tree	8012985818a878b8f5d4850ce12a769f13efd5d3 /content
parent	b2732a066c9ca0b106db440b3fd267befee2ea58 (diff)
download	chromium_src-791c1228d13ebccee5d79aa5d4e96779ab1f0e91.zip chromium_src-791c1228d13ebccee5d79aa5d4e96779ab1f0e91.tar.gz chromium_src-791c1228d13ebccee5d79aa5d4e96779ab1f0e91.tar.bz2