Revert 244240 "IndexedDBFactory now ForceCloses databases."

> 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

TBR=cmumford@chromium.org

Review URL: https://codereview.chromium.org/134503003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@244273 0039d316-1c4b-4281-b951-d872f2087c98

content/browser/indexed_db/indexed_db_context_impl.cc

@@ -187,17 +187,16 @@ base::ListValue* IndexedDBContextImpl::GetAllOriginsDetails() {
     // origins in the outer loop.
 
     if (factory_) {
-      std::pair<IndexedDBFactory::OriginDBMapIterator,
-                IndexedDBFactory::OriginDBMapIterator> range =
+      std::vector<IndexedDBDatabase*> databases =
           factory_->GetOpenDatabasesForOrigin(origin_url);
       // TODO(jsbell): Sort by name?
       scoped_ptr<base::ListValue> database_list(new base::ListValue());
 
-      for (IndexedDBFactory::OriginDBMapIterator it = range.first;
-           it != range.second;
+      for (std::vector<IndexedDBDatabase*>::iterator it = databases.begin();
+           it != databases.end();
            ++it) {
 
-        const IndexedDBDatabase* db = it->second;
+        const IndexedDBDatabase* db = *it;
         scoped_ptr<base::DictionaryValue> db_info(new base::DictionaryValue());
 
         db_info->SetString("name", db->name());
@@ -337,9 +336,21 @@ 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) {
@@ -347,10 +358,10 @@ size_t IndexedDBContextImpl::GetConnectionCount(const GURL& origin_url) {
   if (data_path_.empty() || !IsInOriginSet(origin_url))
     return 0;
 
-  if (!factory_)
+  if (connections_.find(origin_url) == connections_.end())
     return 0;
 
-  return factory_->GetConnectionCount(origin_url);
+  return connections_[origin_url].size();
 }
 
 base::FilePath IndexedDBContextImpl::GetFilePath(const GURL& origin_url) const {
@@ -372,12 +383,14 @@ 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);
@@ -390,18 +403,26 @@ 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);
   }
-  if (factory_ && factory_->GetConnectionCount(origin_url) == 0)
+  connections_[origin_url].erase(connection);
+  if (connections_[origin_url].size() == 0) {
     QueryDiskAndUpdateQuotaUsage(origin_url);
+    connections_.erase(origin_url);
+  }
 }
 
 void IndexedDBContextImpl::TransactionComplete(const GURL& origin_url) {
-  DCHECK(!factory_ || factory_->GetConnectionCount(origin_url) > 0);
+  DCHECK(connections_.find(origin_url) != connections_.end() &&
+         connections_[origin_url].size() > 0);
   QueryDiskAndUpdateQuotaUsage(origin_url);
   QueryAvailableQuota(origin_url);
 }

content/browser/indexed_db/indexed_db_context_impl.h

@@ -137,6 +137,8 @@ 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);
 };

content/browser/indexed_db/indexed_db_database.cc

@@ -1589,17 +1589,6 @@ 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());

content/browser/indexed_db/indexed_db_database.h

@@ -90,7 +90,6 @@ 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);

content/browser/indexed_db/indexed_db_factory.cc

@@ -25,31 +25,13 @@ IndexedDBFactory::IndexedDBFactory(IndexedDBContextImpl* context)
 
 IndexedDBFactory::~IndexedDBFactory() {}
 
-void IndexedDBFactory::RemoveDatabaseFromMaps(
-    const IndexedDBDatabase::Identifier& identifier) {
-  IndexedDBDatabaseMap::iterator it = database_map_.find(identifier);
-  DCHECK(it != database_map_.end());
-  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);
+  IndexedDBDatabaseMap::iterator it = database_map_.find(identifier);
+  DCHECK(it != database_map_.end());
+  DCHECK(!it->second->backing_store());
+  database_map_.erase(it);
 
   // No grace period on a forced-close, as the initiator is
   // assuming the backing store will be released once all
@@ -110,15 +92,6 @@ 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 */);
 }
@@ -210,9 +183,8 @@ void IndexedDBFactory::DeleteDatabase(
   }
 
   database_map_[unique_identifier] = database;
-  origin_dbs_.insert(std::make_pair(origin_url, database));
   database->DeleteDatabase(callbacks);
-  RemoveDatabaseFromMaps(unique_identifier);
+  database_map_.erase(unique_identifier);
   database = NULL;
   backing_store = NULL;
   ReleaseBackingStore(origin_url, false /* immediate */);
@@ -352,27 +324,20 @@ 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::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;
+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;
 }
 
 }  // namespace content

content/browser/indexed_db/indexed_db_factory.h

@@ -26,9 +26,6 @@ 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.
@@ -53,7 +50,8 @@ class CONTENT_EXPORT IndexedDBFactory
 
   void HandleBackingStoreFailure(const GURL& origin_url);
 
-  std::pair<OriginDBMapIterator, OriginDBMapIterator> GetOpenDatabasesForOrigin(
+  // Iterates over all databases; for diagnostics only.
+  std::vector<IndexedDBDatabase*> GetOpenDatabasesForOrigin(
       const GURL& origin_url) const;
 
   // Called by IndexedDBContext after all connections are closed, to
@@ -66,8 +64,6 @@ 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>;
 
@@ -108,14 +104,12 @@ 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;

content/browser/indexed_db/indexed_db_unittest.cc

@@ -116,44 +116,35 @@ TEST_F(IndexedDBTest, SetForceKeepSessionState) {
   EXPECT_TRUE(base::DirectoryExists(session_only_path));
 }
 
-class ForceCloseDBCallbacks : public IndexedDBCallbacks {
+class MockConnection : public IndexedDBConnection {
  public:
-  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_);
+  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_);
   }
 
-  IndexedDBConnection* connection() { return connection_; }
+  virtual void ForceClose() OVERRIDE {
+    ASSERT_TRUE(expect_force_close_);
+    force_close_called_ = true;
+  }
 
- protected:
-  virtual ~ForceCloseDBCallbacks() {}
+  virtual bool IsConnected() OVERRIDE {
+    return !force_close_called_;
+  }
 
  private:
-  scoped_refptr<IndexedDBContextImpl> idb_context_;
-  GURL origin_url_;
-  IndexedDBConnection* connection_;
-  DISALLOW_COPY_AND_ASSIGN(ForceCloseDBCallbacks);
+  bool expect_force_close_;
+  bool force_close_called_;
 };
 
 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.
@@ -165,33 +156,33 @@ TEST_F(IndexedDBTest, ForceCloseOpenDatabasesOnDelete) {
     scoped_refptr<IndexedDBContextImpl> idb_context = new IndexedDBContextImpl(
         temp_dir.path(), special_storage_policy_, NULL, task_runner_);
 
-    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(
         webkit_database::GetIdentifierFromOrigin(kTestOrigin));
+    ASSERT_TRUE(base::CreateDirectory(test_path));
 
-    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,
+    const bool kExpectForceClose = true;
+
+    MockConnection connection1(kExpectForceClose);
+    idb_context->TaskRunner()->PostTask(
+        FROM_HERE,
+        base::Bind(&IndexedDBContextImpl::ConnectionOpened,
+                   idb_context,
                    kTestOrigin,
-                  idb_context->data_path());
+                   &connection1));
 
-    closed_callbacks->connection()->Close();
+    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));
 
     idb_context->TaskRunner()->PostTask(
         FROM_HERE,
@@ -204,8 +195,6 @@ 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));
 }