IndexedDB: Ensure overlapping commits correctly update blob journals

content/browser/indexed_db/indexed_db_backing_store_unittest.cc

 // Copyright (c) 2013 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 "content/browser/indexed_db/indexed_db_backing_store.h"
 
 #include "base/callback.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/logging.h"
@@ skipping 118 matching lines @@
     writes_.push_back(descriptor);
     task_runner()->PostTask(
         FROM_HERE,
         base::Bind(&Transaction::ChainedBlobWriter::ReportWriteCompletion,
                    chained_blob_writer,
                    true,
                    1));
     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;
     }
     removals_.push_back(key);
     return true;
   }
 
   // Timers don't play nicely with unit tests.
   void StartJournalCleaningTimer() override {
@@ skipping 12 matching lines @@
                               origin_url,
                               blob_path,
                               request_context,
                               db.Pass(),
                               comparator.Pass(),
                               task_runner),
         database_id_(0) {}
 
   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);
 };
 
 class TestIDBFactory : public IndexedDBFactoryImpl {
  public:
   explicit TestIDBFactory(IndexedDBContextImpl* idb_context)
       : IndexedDBFactoryImpl(idb_context) {}
 
   scoped_refptr<TestableIndexedDBBackingStore> OpenBackingStoreForTest(
@@ skipping 454 matching lines @@
       EXPECT_TRUE(transaction2.CommitPhaseOne(callback).ok());
       task_runner_->RunUntilIdle();
       EXPECT_TRUE(callback->called);
       EXPECT_TRUE(callback->succeeded);
       EXPECT_TRUE(transaction2.CommitPhaseTwo().ok());
       EXPECT_EQ(0UL, backing_store_->removals().size());
     }
   }
 }
 
+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());
     transaction1.Begin();
     ScopedVector<storage::BlobDataHandle> handles;
     IndexedDBBackingStore::RecordIdentifier record;
     EXPECT_TRUE(backing_store_->PutRecord(&transaction1,
                                           1,
                                           1,
                                           m_key3,
@@ skipping 346 matching lines @@
 
   std::vector<base::string16> names = backing_store_->GetDatabaseNames(&s);
   EXPECT_TRUE(s.ok());
   EXPECT_EQ(names.size(), 1ULL);
   EXPECT_EQ(names[0], db1_name);
 }
 
 }  // namespace
 
 }  // namespace content