IndexedDB: Ensure overlapping commits correctly update blob journals

content/browser/indexed_db/indexed_db_backing_store.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 <algorithm>
+
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/format_macros.h"
 #include "base/json/json_reader.h"
 #include "base/json/json_writer.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 532 matching lines @@
 
   PutVarInt(leveldb_transaction, key, blob_key_generator_current_number);
   return true;
 }
 
 // TODO(ericu): Error recovery. If we persistently can't read the
 // blob journal, the safe thing to do is to clear it and leak the blobs,
 // though that may be costly. Still, database/directory deletion should always
 // clean things up, and we can write an fsck that will do a full correction if
 // need be.
-template <typename T>
-static leveldb::Status GetBlobJournal(const StringPiece& leveldb_key,
-                                      T* leveldb_transaction,
+
+// Read and decode the specified blob journal via the supplied transaction.
+// The key must be either the primary journal key or live journal key.
+template <typename TransactionType>
+static leveldb::Status GetBlobJournal(const StringPiece& key,
+                                      TransactionType* transaction,
                                       BlobJournalType* journal) {
   std::string data;
   bool found = false;
-  leveldb::Status s = leveldb_transaction->Get(leveldb_key, &data, &found);
+  leveldb::Status s = transaction->Get(key, &data, &found);
   if (!s.ok()) {
     INTERNAL_READ_ERROR(READ_BLOB_JOURNAL);
     return s;
   }
   journal->clear();
   if (!found || !data.size())
     return leveldb::Status::OK();
   StringPiece slice(data);
   if (!DecodeBlobJournal(&slice, journal)) {
     INTERNAL_CONSISTENCY_ERROR_UNTESTED(DECODE_BLOB_JOURNAL);
     s = InternalInconsistencyStatus();
   }
   return s;
 }
 
-static void ClearBlobJournal(LevelDBTransaction* leveldb_transaction,
-                             const std::string& level_db_key) {
-  leveldb_transaction->Remove(level_db_key);
+template <typename TransactionType>
+static leveldb::Status GetPrimaryBlobJournal(TransactionType* transaction,
+                                             BlobJournalType* journal) {
+  return GetBlobJournal(BlobJournalKey::Encode(), transaction, journal);
 }
 
-static void UpdatePrimaryJournalWithBlobList(
-    LevelDBTransaction* leveldb_transaction,
-    const BlobJournalType& journal) {
-  const std::string leveldb_key = BlobJournalKey::Encode();
+template <typename TransactionType>
+static leveldb::Status GetLiveBlobJournal(TransactionType* transaction,
+                                          BlobJournalType* journal) {
+  return GetBlobJournal(LiveBlobJournalKey::Encode(), transaction, journal);
+}
+
+// Clear the specified blob journal via the supplied transaction.
+// The key must be either the primary journal key or live journal key.
+template <typename TransactionType>
+static void ClearBlobJournal(TransactionType* transaction,
+                             const std::string& key) {
+  transaction->Remove(key);
+}
+
+// Overwrite the specified blob journal via the supplied transaction.
+// The key must be either the primary journal key or live journal key.
+template <typename TransactionType>
+static void UpdateBlobJournal(TransactionType* transaction,
+                              const std::string& key,
+                              const BlobJournalType& journal) {
   std::string data;
   EncodeBlobJournal(journal, &data);
-  leveldb_transaction->Put(leveldb_key, &data);
+  transaction->Put(key, &data);
 }
 
-static void UpdateLiveBlobJournalWithBlobList(
-    LevelDBTransaction* leveldb_transaction,
-    const BlobJournalType& journal) {
-  const std::string leveldb_key = LiveBlobJournalKey::Encode();
-  std::string data;
-  EncodeBlobJournal(journal, &data);
-  leveldb_transaction->Put(leveldb_key, &data);
+template <typename TransactionType>
+static void UpdatePrimaryBlobJournal(TransactionType* transaction,
+                                     const BlobJournalType& journal) {
+  UpdateBlobJournal(transaction, BlobJournalKey::Encode(), journal);
 }
 
-static leveldb::Status MergeBlobsIntoLiveBlobJournal(
-    LevelDBTransaction* leveldb_transaction,
+template <typename TransactionType>
+static void UpdateLiveBlobJournal(TransactionType* transaction,
+                                  const BlobJournalType& journal) {
+  UpdateBlobJournal(transaction, LiveBlobJournalKey::Encode(), journal);
+}
+
+// Append blobs to the specified blob journal via the supplied transaction.
+// The key must be either the primary journal key or live journal key.
+template <typename TransactionType>
+static leveldb::Status AppendBlobsToBlobJournal(
+    TransactionType* transaction,
+    const std::string& key,
     const BlobJournalType& journal) {
+  if (!journal.size())

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

Nit: if (journal.empty()) reads much better.

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

Fixed here and other blob places.

+    return leveldb::Status::OK();
   BlobJournalType old_journal;
-  const std::string key = LiveBlobJournalKey::Encode();
-  leveldb::Status s = GetBlobJournal(key, leveldb_transaction, &old_journal);
+  leveldb::Status s = GetBlobJournal(key, transaction, &old_journal);
   if (!s.ok())
     return s;
-
   old_journal.insert(old_journal.end(), journal.begin(), journal.end());
-
-  UpdateLiveBlobJournalWithBlobList(leveldb_transaction, old_journal);
+  UpdateBlobJournal(transaction, key, old_journal);
   return leveldb::Status::OK();
 }
 
-static void UpdateBlobJournalWithDatabase(
+template <typename TransactionType>
+static leveldb::Status AppendBlobsToPrimaryBlobJournal(
+    TransactionType* transaction,
+    const BlobJournalType& journal) {
+  return AppendBlobsToBlobJournal(transaction, BlobJournalKey::Encode(),
+                                  journal);
+}
+
+template <typename TransactionType>
+static leveldb::Status AppendBlobsToLiveBlobJournal(
+    TransactionType* transaction,
+    const BlobJournalType& journal) {
+  BlobJournalType old_journal;

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

Do we still need "old_journal"?

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

Nope - removed.

+  return AppendBlobsToBlobJournal(transaction, LiveBlobJournalKey::Encode(),
+                                  journal);
+}
+
+// Append a database to the specified blob journal via the supplied transaction.
+// The key must be either the primary journal key or live journal key.
+static leveldb::Status MergeDatabaseIntoBlobJournal(
+    LevelDBDirectTransaction* transaction,
+    const std::string& key,
+    int64 database_id) {
+  BlobJournalType journal;
+  leveldb::Status s = GetBlobJournal(key, transaction, &journal);
+  if (!s.ok())
+    return s;
+  journal.push_back(
+      std::make_pair(database_id, DatabaseMetaDataKey::kAllBlobsKey));
+  UpdateBlobJournal(transaction, key, journal);
+  return leveldb::Status::OK();
+}
+
+static leveldb::Status MergeDatabaseIntoPrimaryBlobJournal(
     LevelDBDirectTransaction* leveldb_transaction,
     int64 database_id) {
-  BlobJournalType journal;
-  journal.push_back(
-      std::make_pair(database_id, DatabaseMetaDataKey::kAllBlobsKey));
-  const std::string key = BlobJournalKey::Encode();
-  std::string data;
-  EncodeBlobJournal(journal, &data);
-  leveldb_transaction->Put(key, &data);
+  return MergeDatabaseIntoBlobJournal(leveldb_transaction,
+                                      BlobJournalKey::Encode(), database_id);
 }
 
 static leveldb::Status MergeDatabaseIntoLiveBlobJournal(
     LevelDBDirectTransaction* leveldb_transaction,
     int64 database_id) {
-  BlobJournalType journal;
-  const std::string key = LiveBlobJournalKey::Encode();
-  leveldb::Status s = GetBlobJournal(key, leveldb_transaction, &journal);
-  if (!s.ok())
-    return s;
-  journal.push_back(
-      std::make_pair(database_id, DatabaseMetaDataKey::kAllBlobsKey));
-  std::string data;
-  EncodeBlobJournal(journal, &data);
-  leveldb_transaction->Put(key, &data);
-  return leveldb::Status::OK();
+  return MergeDatabaseIntoBlobJournal(
+      leveldb_transaction, LiveBlobJournalKey::Encode(), database_id);
 }
 
 // Blob Data is encoded as a series of:
 //   { is_file [bool], key [int64 as varInt],
 //     type [string-with-length, may be empty],
 //     (for Blobs only) size [int64 as varInt]
 //     (for Files only) fileName [string-with-length]
 //   }
 // There is no length field; just read until you run out of data.
 static std::string EncodeBlobData(
@@ skipping 54 matching lines @@
     scoped_ptr<LevelDBComparator> comparator,
     base::SequencedTaskRunner* task_runner)
     : indexed_db_factory_(indexed_db_factory),
       origin_url_(origin_url),
       blob_path_(blob_path),
       origin_identifier_(ComputeOriginIdentifier(origin_url)),
       request_context_(request_context),
       task_runner_(task_runner),
       db_(db.Pass()),
       comparator_(comparator.Pass()),
-      active_blob_registry_(this) {
+      active_blob_registry_(this),
+      committing_transaction_count_(0) {
 }
 
 IndexedDBBackingStore::~IndexedDBBackingStore() {
   if (!blob_path_.empty() && !child_process_ids_granted_.empty()) {
     ChildProcessSecurityPolicyImpl* policy =
         ChildProcessSecurityPolicyImpl::GetInstance();
     for (const auto& pid : child_process_ids_granted_)
       policy->RevokeAllPermissionsForFile(pid, blob_path_);
   }
   STLDeleteContainerPairSecondPointers(incognito_blob_map_.begin(),
@@ skipping 677 matching lines @@
       transaction, database_id, object_store_id, start_key, stop_key, true);
 }
 
 leveldb::Status IndexedDBBackingStore::DeleteDatabase(
     const base::string16& name) {
   IDB_TRACE("IndexedDBBackingStore::DeleteDatabase");
   scoped_ptr<LevelDBDirectTransaction> transaction =
       LevelDBDirectTransaction::Create(db_.get());
 
   leveldb::Status s;
-  s = CleanUpBlobJournal(BlobJournalKey::Encode());
-  if (!s.ok())
-    return s;
 
   IndexedDBDatabaseMetadata metadata;
   bool success = false;
   s = GetIDBDatabaseMetaData(name, &metadata, &success);
   if (!s.ok())
     return s;
   if (!success)
     return leveldb::Status::OK();
 
   const std::string start_key = DatabaseMetaDataKey::Encode(
@@ skipping 13 matching lines @@
   const std::string key = DatabaseNameKey::Encode(origin_identifier_, name);
   transaction->Remove(key);
 
   bool need_cleanup = false;
   if (active_blob_registry()->MarkDeletedCheckIfUsed(
           metadata.id, DatabaseMetaDataKey::kAllBlobsKey)) {
     s = MergeDatabaseIntoLiveBlobJournal(transaction.get(), metadata.id);
     if (!s.ok())
       return s;
   } else {
-    UpdateBlobJournalWithDatabase(transaction.get(), metadata.id);
+    s = MergeDatabaseIntoPrimaryBlobJournal(transaction.get(), metadata.id);
+    if (!s.ok())
+      return s;
     need_cleanup = true;
   }
 
   s = transaction->Commit();
   if (!s.ok()) {
     INTERNAL_WRITE_ERROR_UNTESTED(DELETE_DATABASE);
     return s;
   }
 
+  // If another transaction is running, this will defer processing
+  // the journal until completion.
   if (need_cleanup)
-    CleanUpBlobJournal(BlobJournalKey::Encode());
+    CleanPrimaryJournalIgnoreReturn();
 
   db_->Compact(start_key, stop_key);
   return s;
 }
 
 static bool CheckObjectStoreAndMetaDataType(const LevelDBIterator* it,
                                             const std::string& stop_key,
                                             int64 object_store_id,
                                             int64 meta_data_type) {
   if (!it->IsValid() || CompareKeys(it->Key(), stop_key) >= 0)
@@ skipping 947 matching lines @@
 }
 
 void IndexedDBBackingStore::ReportBlobUnused(int64 database_id,
                                              int64 blob_key) {
   DCHECK(KeyPrefix::IsValidDatabaseId(database_id));
   bool all_blobs = blob_key == DatabaseMetaDataKey::kAllBlobsKey;
   DCHECK(all_blobs || DatabaseMetaDataKey::IsValidBlobKey(blob_key));
   scoped_refptr<LevelDBTransaction> transaction =
       IndexedDBClassFactory::Get()->CreateLevelDBTransaction(db_.get());
 
-  std::string live_blob_key = LiveBlobJournalKey::Encode();
-  BlobJournalType live_blob_journal;
-  if (!GetBlobJournal(live_blob_key, transaction.get(), &live_blob_journal)
-           .ok())
+  BlobJournalType live_blob_journal, primary_journal;
+  if (!GetLiveBlobJournal(transaction.get(), &live_blob_journal).ok())
     return;
   DCHECK(live_blob_journal.size());
-
-  std::string primary_key = BlobJournalKey::Encode();
-  BlobJournalType primary_journal;
-  if (!GetBlobJournal(primary_key, transaction.get(), &primary_journal).ok())
+  if (!GetPrimaryBlobJournal(transaction.get(), &primary_journal).ok())
     return;
 
   // There are several cases to handle.  If blob_key is kAllBlobsKey, we want to
   // remove all entries with database_id from the live_blob journal and add only
   // kAllBlobsKey to the primary journal.  Otherwise if IsValidBlobKey(blob_key)
   // and we hit kAllBlobsKey for the right database_id in the journal, we leave
   // the kAllBlobsKey entry in the live_blob journal but add the specific blob
   // to the primary.  Otherwise if IsValidBlobKey(blob_key) and we find a
   // matching (database_id, blob_key) tuple, we should move it to the primary
   // journal.
@@ skipping 20 matching lines @@
         break;
       }
     } else {
       new_live_blob_journal.push_back(*journal_iter);
     }
   }
   if (all_blobs) {
     primary_journal.push_back(
         std::make_pair(database_id, DatabaseMetaDataKey::kAllBlobsKey));
   }
-  UpdatePrimaryJournalWithBlobList(transaction.get(), primary_journal);
-  UpdateLiveBlobJournalWithBlobList(transaction.get(), new_live_blob_journal);
+  UpdatePrimaryBlobJournal(transaction.get(), primary_journal);
+  UpdateLiveBlobJournal(transaction.get(), new_live_blob_journal);
   transaction->Commit();
   // We could just do the deletions/cleaning here, but if there are a lot of
   // blobs about to be garbage collected, it'd be better to wait and do them all
   // at once.
   StartJournalCleaningTimer();
 }
 
 // The this reference is a raw pointer that's declared Unretained inside the
 // timer code, so this won't confuse IndexedDBFactory's check for
 // HasLastBackingStoreReference.  It's safe because if the backing store is
 // deleted, the timer will automatically be canceled on destruction.
 void IndexedDBBackingStore::StartJournalCleaningTimer() {
   journal_cleaning_timer_.Start(
       FROM_HERE,
       base::TimeDelta::FromSeconds(5),
       this,
       &IndexedDBBackingStore::CleanPrimaryJournalIgnoreReturn);
 }
 
 // This assumes a file path of dbId/second-to-LSB-of-counter/counter.
-FilePath IndexedDBBackingStore::GetBlobFileName(int64 database_id, int64 key) {
+FilePath IndexedDBBackingStore::GetBlobFileName(int64 database_id,
+                                                int64 key) const {
   return GetBlobFileNameForKey(blob_path_, database_id, key);
 }
 
 static bool CheckIndexAndMetaDataKey(const LevelDBIterator* it,
                                      const std::string& stop_key,
                                      int64 index_id,
                                      unsigned char meta_data_type) {
   if (!it->IsValid() || CompareKeys(it->Key(), stop_key) >= 0)
     return false;
 
@@ skipping 99 matching lines @@
     (*indexes)[index_id] = IndexedDBIndexMetadata(
         index_name, index_id, key_path, index_unique, index_multi_entry);
   }
 
   if (!s.ok())
     INTERNAL_READ_ERROR_UNTESTED(GET_INDEXES);
 
   return s;
 }
 
-bool IndexedDBBackingStore::RemoveBlobFile(int64 database_id, int64 key) {
+bool IndexedDBBackingStore::RemoveBlobFile(int64 database_id, int64 key) const {
   FilePath fileName = GetBlobFileName(database_id, key);
   return base::DeleteFile(fileName, false);
 }
 
-bool IndexedDBBackingStore::RemoveBlobDirectory(int64 database_id) {
+bool IndexedDBBackingStore::RemoveBlobDirectory(int64 database_id) const {
   FilePath dirName = GetBlobDirectoryName(blob_path_, database_id);
   return base::DeleteFile(dirName, true);
 }
 
-leveldb::Status IndexedDBBackingStore::CleanUpBlobJournal(
-    const std::string& level_db_key) {
-  scoped_refptr<LevelDBTransaction> journal_transaction =
-      IndexedDBClassFactory::Get()->CreateLevelDBTransaction(db_.get());
-  BlobJournalType journal;
-  leveldb::Status s =
-      GetBlobJournal(level_db_key, journal_transaction.get(), &journal);
-  if (!s.ok())
-    return s;
+leveldb::Status IndexedDBBackingStore::CleanUpBlobJournalEntries(
+    const BlobJournalType& journal) const {
   if (!journal.size())
     return leveldb::Status::OK();
   for (const auto& entry : journal) {
     int64 database_id = entry.first;
     int64 blob_key = entry.second;
     DCHECK(KeyPrefix::IsValidDatabaseId(database_id));
     if (blob_key == DatabaseMetaDataKey::kAllBlobsKey) {
       if (!RemoveBlobDirectory(database_id))
         return IOErrorStatus();
     } else {
       DCHECK(DatabaseMetaDataKey::IsValidBlobKey(blob_key));
       if (!RemoveBlobFile(database_id, blob_key))
         return IOErrorStatus();
     }
   }
+  return leveldb::Status::OK();
+}
+
+leveldb::Status IndexedDBBackingStore::CleanUpBlobJournal(
+    const std::string& level_db_key) const {
+  DCHECK(!committing_transaction_count_);
+  leveldb::Status s;
+  scoped_refptr<LevelDBTransaction> journal_transaction =
+      IndexedDBClassFactory::Get()->CreateLevelDBTransaction(db_.get());
+  BlobJournalType journal;
+
+  s = GetBlobJournal(level_db_key, journal_transaction.get(), &journal);
+  if (!s.ok())
+    return s;
+  if (!journal.size())
+    return leveldb::Status::OK();
+  s = CleanUpBlobJournalEntries(journal);
+  if (!s.ok())
+    return s;
   ClearBlobJournal(journal_transaction.get(), level_db_key);
   return journal_transaction->Commit();
 }
 
 leveldb::Status IndexedDBBackingStore::Transaction::GetBlobInfoForRecord(
     int64 database_id,
     const std::string& object_store_data_key,
     IndexedDBValue* value) {
   BlobChangeRecord* change_record = NULL;
   BlobChangeMap::const_iterator blob_iter =
@@ skipping 48 matching lines @@
           entry.set_last_modified(info.last_modified);
           entry.set_size(info.size);
         }
       }
     }
   }
   return leveldb::Status::OK();
 }
 
 void IndexedDBBackingStore::CleanPrimaryJournalIgnoreReturn() {
-  CleanUpBlobJournal(BlobJournalKey::Encode());
+  // While a transaction is busy it is not safe to clean the journal.
+  if (committing_transaction_count_ > 0)
+    StartJournalCleaningTimer();
+  else
+    CleanUpBlobJournal(BlobJournalKey::Encode());
 }
 
 WARN_UNUSED_RESULT static leveldb::Status SetMaxIndexId(
     LevelDBTransaction* transaction,
     int64 database_id,
     int64 object_store_id,
     int64 index_id) {
   int64 max_index_id = -1;
   const std::string max_index_id_key = ObjectStoreMetaDataKey::Encode(
       database_id, object_store_id, ObjectStoreMetaDataKey::MAX_INDEX_ID);
@@ skipping 1138 matching lines @@
   scoped_ptr<IndexCursorImpl> cursor(
       new IndexCursorImpl(this, transaction, database_id, cursor_options));
   if (!cursor->FirstSeek(s))
     return scoped_ptr<IndexedDBBackingStore::Cursor>();
 
   return cursor.Pass();
 }
 
 IndexedDBBackingStore::Transaction::Transaction(
     IndexedDBBackingStore* backing_store)
-    : backing_store_(backing_store), database_id_(-1) {
+    : backing_store_(backing_store), database_id_(-1), committing_(false) {
 }
 
 IndexedDBBackingStore::Transaction::~Transaction() {
   STLDeleteContainerPairSecondPointers(
       blob_change_map_.begin(), blob_change_map_.end());
   STLDeleteContainerPairSecondPointers(incognito_blob_map_.begin(),
                                        incognito_blob_map_.end());
+  DCHECK(!committing_);
 }
 
 void IndexedDBBackingStore::Transaction::Begin() {
   IDB_TRACE("IndexedDBBackingStore::Transaction::Begin");
   DCHECK(!transaction_.get());
   transaction_ = IndexedDBClassFactory::Get()->CreateLevelDBTransaction(
       backing_store_->db_.get());
 
   // If incognito, this snapshots blobs just as the above transaction_
   // constructor snapshots the leveldb.
   for (const auto& iter : backing_store_->incognito_blob_map_)
     incognito_blob_map_[iter.first] = iter.second->Clone().release();
 }
 
 static GURL getURLFromUUID(const string& uuid) {
   return GURL("blob:uuid/" + uuid);
 }
 
 leveldb::Status IndexedDBBackingStore::Transaction::HandleBlobPreTransaction(
     BlobEntryKeyValuePairVec* new_blob_entries,
     WriteDescriptorVec* new_files_to_write) {
   if (backing_store_->is_incognito())
     return leveldb::Status::OK();
 
-  new_blob_entries->clear();
-  new_files_to_write->clear();
-  if (!blob_change_map_.empty()) {
-    // Create LevelDBTransaction for the name generator seed and add-journal.
-    scoped_refptr<LevelDBTransaction> pre_transaction =
-        IndexedDBClassFactory::Get()->CreateLevelDBTransaction(
-            backing_store_->db_.get());
-    BlobJournalType journal;
-    for (auto& iter : blob_change_map_) {
-      std::vector<IndexedDBBlobInfo*> new_blob_keys;
-      for (auto& entry : iter.second->mutable_blob_info()) {
-        int64 next_blob_key = -1;
-        bool result = GetBlobKeyGeneratorCurrentNumber(
-            pre_transaction.get(), database_id_, &next_blob_key);
-        if (!result || next_blob_key < 0)
-          return InternalInconsistencyStatus();
-        BlobJournalEntryType journal_entry =
-            std::make_pair(database_id_, next_blob_key);
-        journal.push_back(journal_entry);
-        if (entry.is_file() && !entry.file_path().empty()) {
-          new_files_to_write->push_back(
-              WriteDescriptor(entry.file_path(),
-                              next_blob_key,
-                              entry.size(),
-                              entry.last_modified()));
-        } else {
-          new_files_to_write->push_back(
-              WriteDescriptor(getURLFromUUID(entry.uuid()),
-                              next_blob_key,
-                              entry.size(),
-                              entry.last_modified()));
-        }
-        entry.set_key(next_blob_key);
-        new_blob_keys.push_back(&entry);
-        result = UpdateBlobKeyGeneratorCurrentNumber(
-            pre_transaction.get(), database_id_, next_blob_key + 1);
-        if (!result)
-          return InternalInconsistencyStatus();
+  DCHECK(new_blob_entries->empty());
+  DCHECK(new_files_to_write->empty());
+  DCHECK(blobs_to_write_.empty());
+
+  if (blob_change_map_.empty())
+    return leveldb::Status::OK();
+
+  // Create LevelDBTransaction for the name generator seed and add-journal.
+  scoped_refptr<LevelDBTransaction> pre_transaction =
+      IndexedDBClassFactory::Get()->CreateLevelDBTransaction(
+          backing_store_->db_.get());
+
+  for (auto& iter : blob_change_map_) {
+    std::vector<IndexedDBBlobInfo*> new_blob_keys;
+    for (auto& entry : iter.second->mutable_blob_info()) {
+      int64 next_blob_key = -1;
+      bool result = GetBlobKeyGeneratorCurrentNumber(
+          pre_transaction.get(), database_id_, &next_blob_key);
+      if (!result || next_blob_key < 0)
+        return InternalInconsistencyStatus();
+      BlobJournalEntryType journal_entry =
+          std::make_pair(database_id_, next_blob_key);
+      blobs_to_write_.push_back(journal_entry);

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

Nit: Do you even need the journal_entry variable?

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

Nope - just made it inline.

+      if (entry.is_file() && !entry.file_path().empty()) {
+        new_files_to_write->push_back(
+            WriteDescriptor(entry.file_path(), next_blob_key, entry.size(),
+                            entry.last_modified()));
+      } else {
+        new_files_to_write->push_back(
+            WriteDescriptor(getURLFromUUID(entry.uuid()), next_blob_key,
+                            entry.size(), entry.last_modified()));
       }
-      BlobEntryKey blob_entry_key;
-      StringPiece key_piece(iter.second->key());
-      if (!BlobEntryKey::FromObjectStoreDataKey(&key_piece, &blob_entry_key)) {
-        NOTREACHED();
+      entry.set_key(next_blob_key);
+      new_blob_keys.push_back(&entry);
+      result = UpdateBlobKeyGeneratorCurrentNumber(
+          pre_transaction.get(), database_id_, next_blob_key + 1);
+      if (!result)
         return InternalInconsistencyStatus();
-      }
-      new_blob_entries->push_back(
-          std::make_pair(blob_entry_key, EncodeBlobData(new_blob_keys)));
     }
-    UpdatePrimaryJournalWithBlobList(pre_transaction.get(), journal);
-    leveldb::Status s = pre_transaction->Commit();
-    if (!s.ok())
+    BlobEntryKey blob_entry_key;
+    StringPiece key_piece(iter.second->key());
+    if (!BlobEntryKey::FromObjectStoreDataKey(&key_piece, &blob_entry_key)) {
+      NOTREACHED();
       return InternalInconsistencyStatus();
+    }
+    new_blob_entries->push_back(
+        std::make_pair(blob_entry_key, EncodeBlobData(new_blob_keys)));
   }
+
+  AppendBlobsToPrimaryBlobJournal(pre_transaction.get(), blobs_to_write_);
+  leveldb::Status s = pre_transaction->Commit();
+  if (!s.ok())
+    return InternalInconsistencyStatus();
+
   return leveldb::Status::OK();
 }
 
 bool IndexedDBBackingStore::Transaction::CollectBlobFilesToRemove() {
   if (backing_store_->is_incognito())
     return true;
 
   // Look up all old files to remove as part of the transaction, store their
   // names in blobs_to_remove_, and remove their old blob data entries.
   for (const auto& iter : blob_change_map_) {
@@ skipping 23 matching lines @@
       }
       for (const auto& blob : blob_info) {
         blobs_to_remove_.push_back(std::make_pair(database_id_, blob.key()));
         transaction_->Remove(blob_entry_key_bytes);
       }
     }
   }
   return true;
 }
 
-leveldb::Status IndexedDBBackingStore::Transaction::SortBlobsToRemove() {
+void IndexedDBBackingStore::Transaction::PartitionBlobsToRemove(
+    BlobJournalType* dead_blobs,
+    BlobJournalType* live_blobs) const {
   IndexedDBActiveBlobRegistry* registry =
       backing_store_->active_blob_registry();
-  BlobJournalType primary_journal, live_blob_journal;
   for (const auto& iter : blobs_to_remove_) {
     if (registry->MarkDeletedCheckIfUsed(iter.first, iter.second))
-      live_blob_journal.push_back(iter);
+      live_blobs->push_back(iter);
     else
-      primary_journal.push_back(iter);
+      dead_blobs->push_back(iter);
   }
-  UpdatePrimaryJournalWithBlobList(transaction_.get(), primary_journal);
-  leveldb::Status s =
-      MergeBlobsIntoLiveBlobJournal(transaction_.get(), live_blob_journal);
-  if (!s.ok())
-    return s;
-  // To signal how many blobs need attention right now.
-  blobs_to_remove_.swap(primary_journal);
-  return leveldb::Status::OK();
 }
 
 leveldb::Status IndexedDBBackingStore::Transaction::CommitPhaseOne(
     scoped_refptr<BlobWriteCallback> callback) {
   IDB_TRACE("IndexedDBBackingStore::Transaction::CommitPhaseOne");
   DCHECK(transaction_.get());
   DCHECK(backing_store_->task_runner()->RunsTasksOnCurrentThread());
 
   leveldb::Status s;
 
-  s = backing_store_->CleanUpBlobJournal(BlobJournalKey::Encode());
-  if (!s.ok()) {
-    INTERNAL_WRITE_ERROR(TRANSACTION_COMMIT_METHOD);
-    transaction_ = NULL;
-    return s;
-  }
-
   BlobEntryKeyValuePairVec new_blob_entries;
   WriteDescriptorVec new_files_to_write;
   s = HandleBlobPreTransaction(&new_blob_entries, &new_files_to_write);
   if (!s.ok()) {
     INTERNAL_WRITE_ERROR_UNTESTED(TRANSACTION_COMMIT_METHOD);
     transaction_ = NULL;
     return s;
   }
 
   DCHECK(!new_files_to_write.size() ||
          KeyPrefix::IsValidDatabaseId(database_id_));
   if (!CollectBlobFilesToRemove()) {
     INTERNAL_WRITE_ERROR_UNTESTED(TRANSACTION_COMMIT_METHOD);
     transaction_ = NULL;
     return InternalInconsistencyStatus();
   }
 
+  committing_ = true;
+  ++backing_store_->committing_transaction_count_;
+
   if (new_files_to_write.size()) {
     // This kicks off the writes of the new blobs, if any.
     // This call will zero out new_blob_entries and new_files_to_write.
     WriteNewBlobs(&new_blob_entries, &new_files_to_write, callback);
-    // Remove the add journal, if any; once the blobs are written, and we
-    // commit, this will do the cleanup.
-    ClearBlobJournal(transaction_.get(), BlobJournalKey::Encode());
   } else {
     callback->Run(true);
   }
 
   return leveldb::Status::OK();
 }
 
 leveldb::Status IndexedDBBackingStore::Transaction::CommitPhaseTwo() {
   IDB_TRACE("IndexedDBBackingStore::Transaction::CommitPhaseTwo");
   leveldb::Status s;
-  if (blobs_to_remove_.size()) {
-    s = SortBlobsToRemove();
-    if (!s.ok()) {
-      INTERNAL_READ_ERROR_UNTESTED(TRANSACTION_COMMIT_METHOD);
-      transaction_ = NULL;
+
+  DCHECK(committing_);
+  committing_ = false;
+  --backing_store_->committing_transaction_count_;

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

We sure it won't underflow? DCHECK maybe?

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

Added a DCHECK (here and in Rollback). I wasn't sure how fancy to get here. This
could be done by holding a refcounted object (and and check w/ HasOneRef instead
of count == 0) or something.

+
+  // Read the persisted states of the primary/live blob journals,
+  // so that they can be updated correctly by the transaction.
+  BlobJournalType primary_journal, live_journal;
+  {
+    scoped_refptr<LevelDBTransaction> journal_transaction =
+        IndexedDBClassFactory::Get()->CreateLevelDBTransaction(
+            backing_store_->db_.get());
+    s = GetPrimaryBlobJournal(journal_transaction.get(), &primary_journal);
+    if (!s.ok())
       return s;
-    }
+    s = GetLiveBlobJournal(journal_transaction.get(), &live_journal);
+    if (!s.ok())
+      return s;
   }
 
+  // Remove newly added blobs from the journal - they will be accounted
+  // for in blob entry tables in the transaction.
+  {
+    std::sort(primary_journal.begin(), primary_journal.end());
+    std::sort(blobs_to_write_.begin(), blobs_to_write_.end());
+    BlobJournalType new_journal = base::STLSetDifference<BlobJournalType>(
+        primary_journal, blobs_to_write_);
+    primary_journal.swap(new_journal);
+  }
+
+  // Append newly deleted blobs to appropriate primary/live journals.
+  BlobJournalType saved_primary_journal = primary_journal;
+  BlobJournalType dead_blobs, live_blobs;
+  if (blobs_to_remove_.size()) {
+    DCHECK(!backing_store_->is_incognito());
+    PartitionBlobsToRemove(&dead_blobs, &live_blobs);
+  }
+  primary_journal.insert(primary_journal.end(), dead_blobs.begin(),
+                         dead_blobs.end());
+  live_journal.insert(live_journal.end(), live_blobs.begin(), live_blobs.end());
+  UpdatePrimaryBlobJournal(transaction_.get(), primary_journal);
+  UpdateLiveBlobJournal(transaction_.get(), live_journal);
+
+  // Actually commit. If this succeeds, the journals will appropriately
+  // reflect pending blob work - dead files that should be deleted
+  // immediately, and live files to monitor.
   s = transaction_->Commit();
   transaction_ = NULL;
 
-  if (s.ok() && backing_store_->is_incognito() && !blob_change_map_.empty()) {
-    BlobChangeMap& target_map = backing_store_->incognito_blob_map_;
-    for (auto& iter : blob_change_map_) {
-      BlobChangeMap::iterator target_record = target_map.find(iter.first);
-      if (target_record != target_map.end()) {
-        delete target_record->second;
-        target_map.erase(target_record);
-      }
-      if (iter.second) {
-        target_map[iter.first] = iter.second;
-        iter.second = NULL;
+  if (!s.ok()) {
+    INTERNAL_WRITE_ERROR(TRANSACTION_COMMIT_METHOD);
+    return s;
+  }
+
+  if (backing_store_->is_incognito()) {
+    if (!blob_change_map_.empty()) {
+      BlobChangeMap& target_map = backing_store_->incognito_blob_map_;
+      for (auto& iter : blob_change_map_) {
+        BlobChangeMap::iterator target_record = target_map.find(iter.first);
+        if (target_record != target_map.end()) {
+          delete target_record->second;
+          target_map.erase(target_record);
+        }
+        if (iter.second) {
+          target_map[iter.first] = iter.second;
+          iter.second = NULL;
+        }
       }
     }
+    return leveldb::Status::OK();
   }
-  if (!s.ok())
-    INTERNAL_WRITE_ERROR(TRANSACTION_COMMIT_METHOD);
-  else if (blobs_to_remove_.size())
-    s = backing_store_->CleanUpBlobJournal(BlobJournalKey::Encode());
 
+  // Actually delete dead blob files, then remove those entries
+  // from the persisted primary journal.
+  if (!dead_blobs.size())
+    return leveldb::Status::OK();
+
+  s = backing_store_->CleanUpBlobJournalEntries(dead_blobs);
+  if (!s.ok()) {
+    INTERNAL_WRITE_ERROR_UNTESTED(TRANSACTION_COMMIT_METHOD);
+    return s;
+  }
+
+  scoped_refptr<LevelDBTransaction> update_journal_transaction =
+      IndexedDBClassFactory::Get()->CreateLevelDBTransaction(
+          backing_store_->db_.get());
+  UpdatePrimaryBlobJournal(update_journal_transaction.get(),
+                           saved_primary_journal);
+  s = update_journal_transaction->Commit();
   return s;
 }
 
 
 class IndexedDBBackingStore::Transaction::BlobWriteCallbackWrapper
     : public IndexedDBBackingStore::BlobWriteCallback {
  public:
   BlobWriteCallbackWrapper(IndexedDBBackingStore::Transaction* transaction,
                            scoped_refptr<BlobWriteCallback> callback)
       : transaction_(transaction), callback_(callback) {}
@@ skipping 31 matching lines @@
   // Creating the writer will start it going asynchronously.
   chained_blob_writer_ =
       new ChainedBlobWriterImpl(database_id_,
                                 backing_store_,
                                 new_files_to_write,
                                 new BlobWriteCallbackWrapper(this, callback));
 }
 
 void IndexedDBBackingStore::Transaction::Rollback() {
   IDB_TRACE("IndexedDBBackingStore::Transaction::Rollback");
+  if (committing_) {
+    committing_ = false;
+    --backing_store_->committing_transaction_count_;
+  }
+
   if (chained_blob_writer_.get()) {
     chained_blob_writer_->Abort();
     chained_blob_writer_ = NULL;
   }
   if (transaction_.get() == NULL)
     return;
   transaction_->Rollback();
   transaction_ = NULL;
 }
 
@@ skipping 109 matching lines @@
     int64_t size,
     base::Time last_modified)
     : is_file_(true),
       file_path_(file_path),
       key_(key),
       size_(size),
       last_modified_(last_modified) {
 }
 
 }  // namespace content