IndexedDB: Ensure overlapping commits correctly update blob journals

content/browser/indexed_db/indexed_db_backing_store_unittest.cc

Index: content/browser/indexed_db/indexed_db_backing_store_unittest.cc
diff --git a/content/browser/indexed_db/indexed_db_backing_store_unittest.cc b/content/browser/indexed_db/indexed_db_backing_store_unittest.cc
index 0b8ecef0e27c2e9dd1792a37e3b7760b5be42296..a32bbc9e6bd900928033574235202e70731755b3 100644
--- a/content/browser/indexed_db/indexed_db_backing_store_unittest.cc
+++ b/content/browser/indexed_db/indexed_db_backing_store_unittest.cc
@@ -136,7 +136,7 @@ class TestableIndexedDBBackingStore : public IndexedDBBackingStore {
     return true;
   }
 
-  bool RemoveBlobFile(int64 database_id, int64 key) override {
+  bool RemoveBlobFile(int64 database_id, int64 key) const override {
     if (database_id_ != database_id ||
         !KeyPrefix::IsValidDatabaseId(database_id)) {
       return false;
@@ -169,7 +169,10 @@ class TestableIndexedDBBackingStore : public IndexedDBBackingStore {
 
   int64 database_id_;
   std::vector<Transaction::WriteDescriptor> writes_;
-  std::vector<int64> removals_;
+
+  // This is modified in an overridden virtual function that is properly const
+  // in the real implementation, therefore must be mutable here.

[cmumford, 2015/01/22 00:56:57]

Why is RemoveBlobFile properly const? Seams reasonable for it not to be.

[jsbell, 2015/01/22 19:40:45]

The various blob methods are a mix of modifying the transaction state and just
helpers factoring out chunks of work. I sprinkled const in to make better sense
of what was going on. Apart from the override in the test, RemoveBlobFile() is
const since all it does is delete the file on disk. So the calling functions can
be const as well, which made it all easier to understand... this seems like one
of the few times mutable makes sense. Otherwise, a whole tree of callers can't
be const.

WDYT?

+  mutable std::vector<int64> removals_;
 
   DISALLOW_COPY_AND_ASSIGN(TestableIndexedDBBackingStore);
 };
@@ -644,6 +647,42 @@ TEST_F(IndexedDBBackingStoreTest, DeleteRangeEmptyRange) {
   }
 }
 
+TEST_F(IndexedDBBackingStoreTest, BlobJournalInterleavedTransactions) {
+  IndexedDBBackingStore::Transaction transaction1(backing_store_.get());
+  transaction1.Begin();
+  ScopedVector<storage::BlobDataHandle> handles1;
+  IndexedDBBackingStore::RecordIdentifier record1;
+  EXPECT_TRUE(backing_store_->PutRecord(&transaction1, 1, 1, m_key3, &m_value3,
+                                        &handles1, &record1).ok());
+  scoped_refptr<TestCallback> callback1(new TestCallback());
+  EXPECT_TRUE(transaction1.CommitPhaseOne(callback1).ok());
+  task_runner_->RunUntilIdle();
+  EXPECT_TRUE(CheckBlobWrites());
+  EXPECT_TRUE(callback1->called);
+  EXPECT_TRUE(callback1->succeeded);
+  EXPECT_EQ(0U, backing_store_->removals().size());
+
+  IndexedDBBackingStore::Transaction transaction2(backing_store_.get());
+  transaction2.Begin();
+  ScopedVector<storage::BlobDataHandle> handles2;
+  IndexedDBBackingStore::RecordIdentifier record2;
+  EXPECT_TRUE(backing_store_->PutRecord(&transaction2, 1, 1, m_key1, &m_value1,
+                                        &handles2, &record2).ok());
+  scoped_refptr<TestCallback> callback2(new TestCallback());
+  EXPECT_TRUE(transaction2.CommitPhaseOne(callback2).ok());
+  task_runner_->RunUntilIdle();
+  EXPECT_TRUE(CheckBlobWrites());
+  EXPECT_TRUE(callback2->called);
+  EXPECT_TRUE(callback2->succeeded);
+  EXPECT_EQ(0U, backing_store_->removals().size());
+
+  EXPECT_TRUE(transaction1.CommitPhaseTwo().ok());
+  EXPECT_EQ(0U, backing_store_->removals().size());
+
+  EXPECT_TRUE(transaction2.CommitPhaseTwo().ok());
+  EXPECT_EQ(0U, backing_store_->removals().size());
+}
+
 TEST_F(IndexedDBBackingStoreTest, LiveBlobJournal) {
   {
     IndexedDBBackingStore::Transaction transaction1(backing_store_.get());