Blob support for IDB [Chromium]

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 "base/file_util.h"
 #include "base/files/file_path.h"
 #include "base/format_macros.h"
 #include "base/json/json_reader.h"
@@ skipping 1302 matching lines @@
   if (int_version == IndexedDBDatabaseMetadata::NO_INT_VERSION)
     int_version = IndexedDBDatabaseMetadata::DEFAULT_INT_VERSION;
   DCHECK_GE(int_version, 0) << "int_version was " << int_version;
   PutVarInt(transaction->transaction(),
             DatabaseMetaDataKey::Encode(row_id,
                                         DatabaseMetaDataKey::USER_INT_VERSION),
             int_version);
   return true;
 }
 
-static leveldb::Status DeleteRange(LevelDBTransaction* transaction,
-                                   const std::string& begin,
-                                   const std::string& end) {
+// If you're deleting a range that contains user keys that have blob info, this
+// won't clean up the blobs.
+static leveldb::Status DeleteRangeBasic(LevelDBTransaction* transaction,
+                                        const std::string& begin,
+                                        const std::string& end,
+                                        bool upper_open) {
   scoped_ptr<LevelDBIterator> it = transaction->CreateIterator();
   leveldb::Status s;
-  for (s = it->Seek(begin);
-       s.ok() && it->IsValid() && CompareKeys(it->Key(), end) < 0;
+  for (s = it->Seek(begin); s.ok() && it->IsValid() &&
+                                (upper_open ? CompareKeys(it->Key(), end) < 0
+                                            : CompareKeys(it->Key(), end) <= 0);
        s = it->Next())
     transaction->Remove(it->Key());
   return s;
 }
 
-static leveldb::Status DeleteBlobsInObjectStore(
+static leveldb::Status DeleteBlobsInRange(
     IndexedDBBackingStore::Transaction* transaction,
     int64 database_id,
-    int64 object_store_id) {
-  std::string start_key, end_key;
-  start_key =
-      BlobEntryKey::EncodeMinKeyForObjectStore(database_id, object_store_id);
-  end_key =
-      BlobEntryKey::EncodeStopKeyForObjectStore(database_id, object_store_id);
-
+    int64 object_store_id,
+    const std::string& start_key,
+    const std::string& end_key,
+    bool upper_open) {
   scoped_ptr<LevelDBIterator> it = transaction->transaction()->CreateIterator();
-
   leveldb::Status s = it->Seek(start_key);
-  for (; s.ok() && it->IsValid() && CompareKeys(it->Key(), end_key) < 0;
+  for (; s.ok() && it->IsValid() &&
+             (upper_open ? CompareKeys(it->Key(), end_key) < 0
+                         : CompareKeys(it->Key(), end_key) <= 0);
        s = it->Next()) {
     StringPiece key_piece(it->Key());
     std::string user_key =
         BlobEntryKey::ReencodeToObjectStoreDataKey(&key_piece);
     if (!user_key.size()) {
       INTERNAL_CONSISTENCY_ERROR_UNTESTED(GET_IDBDATABASE_METADATA);
       return InternalInconsistencyStatus();
     }
     transaction->PutBlobInfo(
         database_id, object_store_id, user_key, NULL, NULL);
   }
   return s;
 }
 
+static leveldb::Status DeleteBlobsInObjectStore(
+    IndexedDBBackingStore::Transaction* transaction,
+    int64 database_id,
+    int64 object_store_id) {
+  std::string start_key, end_key;

[cmumford, 2014/05/28 20:34:54]

Nit: You're already using "stop_key" elsewhere, and the function contains
"Stop". Should you stick with that name?

[ericu, 2014/05/28 22:50:42]

Done.

+  start_key =
+      BlobEntryKey::EncodeMinKeyForObjectStore(database_id, object_store_id);
+  end_key =
+      BlobEntryKey::EncodeStopKeyForObjectStore(database_id, object_store_id);
+  return DeleteBlobsInRange(
+      transaction, database_id, object_store_id, start_key, end_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;
@@ skipping 27 matching lines @@
   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);
     need_cleanup = true;
   }
 
-  // TODO(ericu): Remove these fake calls, added to avoid "defined but unused"
-  // compiler errors until the code that makes the real calls can be added.
-  if (false) {
-    std::vector<IndexedDBBlobInfo*> fake;
-    EncodeBlobData(fake);
-
-    scoped_refptr<LevelDBTransaction> fake_transaction =
-        new LevelDBTransaction(NULL);
-    BlobJournalType fake_journal;
-    MergeBlobsIntoLiveBlobJournal(fake_transaction.get(), fake_journal);
-    UpdateBlobKeyGeneratorCurrentNumber(fake_transaction.get(), 0, 0);
-    int64 arg;
-    GetBlobKeyGeneratorCurrentNumber(fake_transaction.get(), 0, &arg);
-  }
-
   s = transaction->Commit();
   if (!s.ok()) {
     INTERNAL_WRITE_ERROR_UNTESTED(DELETE_DATABASE);
     return s;
   }
 
   if (need_cleanup)
     CleanUpBlobJournal(BlobJournalKey::Encode());
 
   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)
     return false;
 
   StringPiece slice(it->Key());
   ObjectStoreMetaDataKey meta_data_key;
-  bool ok = ObjectStoreMetaDataKey::Decode(&slice, &meta_data_key);
+  bool ok =
+      ObjectStoreMetaDataKey::Decode(&slice, &meta_data_key) && slice.empty();
   DCHECK(ok);
   if (meta_data_key.ObjectStoreId() != object_store_id)
     return false;
   if (meta_data_key.MetaDataType() != meta_data_type)
     return false;
-  return true;
+  return ok;
 }
 
 // TODO(jsbell): This should do some error handling rather than
 // plowing ahead when bad data is encountered.
 leveldb::Status IndexedDBBackingStore::GetObjectStores(
     int64 database_id,
     IndexedDBDatabaseMetadata::ObjectStoreMap* object_stores) {
   IDB_TRACE("IndexedDBBackingStore::GetObjectStores");
   if (!KeyPrefix::IsValidDatabaseId(database_id))
     return InvalidDBKeyStatus();
@@ skipping 274 matching lines @@
     INTERNAL_CONSISTENCY_ERROR_UNTESTED(DELETE_OBJECT_STORE);
     return InternalInconsistencyStatus();
   }
 
   s = DeleteBlobsInObjectStore(transaction, database_id, object_store_id);
   if (!s.ok()) {
     INTERNAL_CONSISTENCY_ERROR_UNTESTED(DELETE_OBJECT_STORE);
     return s;
   }
 
-  s = DeleteRange(
+  s = DeleteRangeBasic(
       leveldb_transaction,
       ObjectStoreMetaDataKey::Encode(database_id, object_store_id, 0),
-      ObjectStoreMetaDataKey::EncodeMaxKey(database_id, object_store_id));
+      ObjectStoreMetaDataKey::EncodeMaxKey(database_id, object_store_id),
+      true);
 
   if (s.ok()) {
     leveldb_transaction->Remove(
         ObjectStoreNamesKey::Encode(database_id, object_store_name));
 
-    s = DeleteRange(
+    s = DeleteRangeBasic(
         leveldb_transaction,
         IndexFreeListKey::Encode(database_id, object_store_id, 0),
-        IndexFreeListKey::EncodeMaxKey(database_id, object_store_id));
+        IndexFreeListKey::EncodeMaxKey(database_id, object_store_id),
+        true);
   }
 
   if (s.ok()) {
-    s = DeleteRange(
+    s = DeleteRangeBasic(
         leveldb_transaction,
         IndexMetaDataKey::Encode(database_id, object_store_id, 0, 0),
-        IndexMetaDataKey::EncodeMaxKey(database_id, object_store_id));
+        IndexMetaDataKey::EncodeMaxKey(database_id, object_store_id),
+        true);
   }
 
   if (!s.ok()) {
     INTERNAL_WRITE_ERROR_UNTESTED(DELETE_OBJECT_STORE);
     return s;
   }
 
   return ClearObjectStore(transaction, database_id, object_store_id);
 }
 
@@ skipping 123 matching lines @@
     int64 object_store_id) {
   IDB_TRACE("IndexedDBBackingStore::ClearObjectStore");
   if (!KeyPrefix::ValidIds(database_id, object_store_id))
     return InvalidDBKeyStatus();
   const std::string start_key =
       KeyPrefix(database_id, object_store_id).Encode();
   const std::string stop_key =
       KeyPrefix(database_id, object_store_id + 1).Encode();
 
   leveldb::Status s =
-      DeleteRange(transaction->transaction(), start_key, stop_key);
+      DeleteRangeBasic(transaction->transaction(), start_key, stop_key, true);
   if (!s.ok()) {
     INTERNAL_WRITE_ERROR(CLEAR_OBJECT_STORE);
     return s;
   }
   return DeleteBlobsInObjectStore(transaction, database_id, object_store_id);
 }
 
 leveldb::Status IndexedDBBackingStore::DeleteRecord(
     IndexedDBBackingStore::Transaction* transaction,
     int64 database_id,
     int64 object_store_id,
     const RecordIdentifier& record_identifier) {
   IDB_TRACE("IndexedDBBackingStore::DeleteRecord");
   if (!KeyPrefix::ValidIds(database_id, object_store_id))
     return InvalidDBKeyStatus();
   LevelDBTransaction* leveldb_transaction = transaction->transaction();
 
   const std::string object_store_data_key = ObjectStoreDataKey::Encode(
       database_id, object_store_id, record_identifier.primary_key());
   leveldb_transaction->Remove(object_store_data_key);
   transaction->PutBlobInfo(
       database_id, object_store_id, object_store_data_key, NULL, NULL);
 
   const std::string exists_entry_key = ExistsEntryKey::Encode(
       database_id, object_store_id, record_identifier.primary_key());
   leveldb_transaction->Remove(exists_entry_key);
   return leveldb::Status::OK();
 }
 
+leveldb::Status IndexedDBBackingStore::DeleteRange(
+    IndexedDBBackingStore::Transaction* transaction,
+    int64 database_id,
+    int64 object_store_id,
+    const IndexedDBKeyRange& key_range) {
+  leveldb::Status s;
+  scoped_ptr<IndexedDBBackingStore::Cursor> start_cursor =
+      OpenObjectStoreCursor(transaction,
+                            database_id,
+                            object_store_id,
+                            key_range,
+                            indexed_db::CURSOR_NEXT,
+                            &s);
+  if (!s.ok())
+    return s;
+  if (!start_cursor)
+    return leveldb::Status::OK();  // Empty range == delete success.
+
+  scoped_ptr<IndexedDBBackingStore::Cursor> end_cursor =

[cmumford, 2014/05/28 20:34:54]

Nit: Same "stop" vs. "end" question.

[ericu, 2014/05/28 22:50:42]

Done.

+      OpenObjectStoreCursor(transaction,
+                            database_id,
+                            object_store_id,
+                            key_range,
+                            indexed_db::CURSOR_PREV,
+                            &s);
+
+  if (!s.ok())
+    return s;
+  if (!end_cursor)
+    return leveldb::Status::OK();  // Empty range == delete success.
+
+  BlobEntryKey start_blob_key, end_blob_key;
+
+  std::string start_key = ObjectStoreDataKey::Encode(
+      database_id, object_store_id, start_cursor->key());
+  base::StringPiece start_key_piece(start_key);
+  if (!BlobEntryKey::FromObjectStoreDataKey(&start_key_piece, &start_blob_key))
+    return InternalInconsistencyStatus();
+  std::string end_key = ObjectStoreDataKey::Encode(
+      database_id, object_store_id, end_cursor->key());
+  base::StringPiece end_key_piece(end_key);
+  if (!BlobEntryKey::FromObjectStoreDataKey(&end_key_piece, &end_blob_key))
+    return InternalInconsistencyStatus();
+
+  s = DeleteBlobsInRange(transaction,
+                         database_id,
+                         object_store_id,
+                         start_blob_key.Encode(),
+                         end_blob_key.Encode(),
+                         false);

[cmumford, 2014/05/28 20:34:54]

Just curious: Why is this one not "upper_open"?

[jsbell, 2014/05/28 21:29:15]

It's found above by the end_cursor, so it's a key for an actual record (last one
in range)

[ericu, 2014/05/28 22:50:42]

The OpenObjectStoreCursor that created end_cursor positioned it at the last
entry we want to remove, hence a closed interval.

+  if (!s.ok())
+    return s;
+  s = DeleteRangeBasic(transaction->transaction(), start_key, end_key, false);
+  if (!s.ok())

[cmumford, 2014/05/28 20:34:54]

Should we bail on the first error, or instead do a best effort when deleting a
range? What would happen if the "Exists" entries were there, but the Blob
entries were not?

[jsbell, 2014/05/28 21:29:15]

We should bail, since we will abort the whole transaction anyway.

[ericu, 2014/05/28 22:50:42]

The Blob entries often won't be there, if there aren't any blobs.  But an empty
range will return success from DeleteRangeBasic.  An error will be caused by
corruption, a locked/unreadable disk, etc., so I think it's better to fail the
transaction completely rather than possibly corrupt a database over a temporary
IO error.

+    return s;
+  start_key =
+      ExistsEntryKey::Encode(database_id, object_store_id, start_cursor->key());

[jsbell, 2014/05/28 21:29:15]

Were we "leaking" ExistsEntryKeys before in some cases?

[ericu, 2014/05/28 22:50:42]

I don't think so--we didn't have a DeleteRange at all, so it all went through
DeleteRecord, which should work.  What are you thinking of?

[jsbell, 2014/05/28 23:35:18]

Nothing specific. I thought when poking around that I saw a path that was
missing deleting ExistsEntryKeys, but not finding it. I should have deleted this
comment.

+  end_key =
+      ExistsEntryKey::Encode(database_id, object_store_id, end_cursor->key());
+  return DeleteRangeBasic(
+      transaction->transaction(), start_key, end_key, false);
+}
+
 leveldb::Status IndexedDBBackingStore::GetKeyGeneratorCurrentNumber(
     IndexedDBBackingStore::Transaction* transaction,
     int64 database_id,
     int64 object_store_id,
     int64* key_generator_current_number) {
   if (!KeyPrefix::ValidIds(database_id, object_store_id))
     return InvalidDBKeyStatus();
   LevelDBTransaction* leveldb_transaction = transaction->transaction();
 
   const std::string key_generator_current_number_key =
@@ skipping 698 matching lines @@
     int64 index_id) {
   IDB_TRACE("IndexedDBBackingStore::DeleteIndex");
   if (!KeyPrefix::ValidIds(database_id, object_store_id, index_id))
     return InvalidDBKeyStatus();
   LevelDBTransaction* leveldb_transaction = transaction->transaction();
 
   const std::string index_meta_data_start =
       IndexMetaDataKey::Encode(database_id, object_store_id, index_id, 0);
   const std::string index_meta_data_end =
       IndexMetaDataKey::EncodeMaxKey(database_id, object_store_id, index_id);
-  leveldb::Status s = DeleteRange(
-      leveldb_transaction, index_meta_data_start, index_meta_data_end);
+  leveldb::Status s = DeleteRangeBasic(
+      leveldb_transaction, index_meta_data_start, index_meta_data_end, true);
 
   if (s.ok()) {
     const std::string index_data_start =
         IndexDataKey::EncodeMinKey(database_id, object_store_id, index_id);
     const std::string index_data_end =
         IndexDataKey::EncodeMaxKey(database_id, object_store_id, index_id);
-    s = DeleteRange(leveldb_transaction, index_data_start, index_data_end);
+    s = DeleteRangeBasic(
+        leveldb_transaction, index_data_start, index_data_end, true);
   }
 
   if (!s.ok())
     INTERNAL_WRITE_ERROR_UNTESTED(DELETE_INDEX);
 
   return s;
 }
 
 leveldb::Status IndexedDBBackingStore::PutIndexDataForRecord(
     IndexedDBBackingStore::Transaction* transaction,
@@ skipping 1067 matching lines @@
 
   // If incognito, this snapshots blobs just as the above transaction_
   // constructor snapshots the leveldb.
   BlobChangeMap::const_iterator iter;
   for (iter = backing_store_->incognito_blob_map_.begin();
        iter != backing_store_->incognito_blob_map_.end();
        ++iter)
     incognito_blob_map_[iter->first] = iter->second->Clone().release();
 }
 
-leveldb::Status IndexedDBBackingStore::Transaction::Commit() {
-  IDB_TRACE("IndexedDBBackingStore::Transaction::Commit");
-  DCHECK(transaction_.get());
-  leveldb::Status s = transaction_->Commit();
+static GURL getURLFromUUID(const string& uuid) {
+  return GURL("blob:uuid/" + uuid);

[jsbell, 2014/05/28 21:29:15]

FYI: There's been some blather about standardizing blob URLs on the mailing
lists, e.g. tacking the origin on the front, and persisting that when a blob is
written/read from storage. We might need to persist more information.

[ericu, 2014/05/28 22:50:42]

Hmm...I've read most of the blather, but I missed the bit about persistence. 
I've been assuming that we could just treat something read from storage as
coming from the same origin as the storage.  If we need to add origin info, we
can do that, but it'll be a change.

Actually, we're using blobs here, not blob URLs; this URL is an internal
implementation detail used for writing blobs.  Since the blob itself is handed
to us by script, that means it is of local origin, as it's either locally
created or structured-cloned by postMessage, right?

[jsbell, 2014/05/28 23:35:18]

I haven't even been following the blather enough to comment sensibly. I think
the URL is something minted on request via URL.createFor(blob) and would be
tagged with the origin in place at that time. So presumably when written/read
it's effectively a fresh Blob? Not sure. If we need to do anything, could be a
follow up CL.

+}
+
+leveldb::Status IndexedDBBackingStore::Transaction::HandleBlobPreTransaction(
+    BlobEntryKeyValuePairVec* new_blob_entries,
+    WriteDescriptorVec* new_files_to_write) {
+  if (backing_store_->is_incognito())
+    return leveldb::Status::OK();
+
+  BlobChangeMap::iterator iter = blob_change_map_.begin();
+  new_blob_entries->clear();
+  new_files_to_write->clear();
+  if (iter != blob_change_map_.end()) {
+    // Create LevelDBTransaction for the name generator seed and add-journal.
+    scoped_refptr<LevelDBTransaction> pre_transaction =
+        new LevelDBTransaction(backing_store_->db_.get());
+    BlobJournalType journal;
+    for (; iter != blob_change_map_.end(); ++iter) {
+      std::vector<IndexedDBBlobInfo>::iterator info_iter;
+      std::vector<IndexedDBBlobInfo*> new_blob_keys;
+      for (info_iter = iter->second->mutable_blob_info().begin();
+           info_iter != iter->second->mutable_blob_info().end();
+           ++info_iter) {
+        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 (info_iter->is_file()) {
+          new_files_to_write->push_back(
+              WriteDescriptor(info_iter->file_path(), next_blob_key));
+        } else {
+          new_files_to_write->push_back(WriteDescriptor(
+              getURLFromUUID(info_iter->uuid()), next_blob_key));
+        }
+        info_iter->set_key(next_blob_key);
+        new_blob_keys.push_back(&*info_iter);
+        result = UpdateBlobKeyGeneratorCurrentNumber(
+            pre_transaction.get(), database_id_, next_blob_key + 1);
+        if (!result)
+          return InternalInconsistencyStatus();
+      }
+      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)));
+    }
+    UpdatePrimaryJournalWithBlobList(pre_transaction.get(), journal);
+    if (!pre_transaction->Commit().ok())
+      return InternalInconsistencyStatus();

[cmumford, 2014/05/28 20:34:54]

Why not return the actual return value of transaction::Commit()?

[ericu, 2014/05/28 22:50:42]

Done.

+  }
+  return leveldb::Status::OK();
+}
+
+bool IndexedDBBackingStore::Transaction::CollectBlobFilesToRemove() {

[cmumford, 2014/05/28 20:34:54]

Should this method clear blobs_to_remove_, or at least
DCHECK(blobs_to_remove_.empty())? What if CollectBlobFilesToRemove is
successfully called (populating that vector), and then an error happens and
SortBlobsToRemove isn't called and therefore blobs_to_remove_ is left with the
prior contents. Can that happen?

[ericu, 2014/05/28 22:50:42]

If any error happens, the transaction is aborted.  In particular in this case,
transaction_ gets cleared as soon as this function returns an error, which
returns to IndexedDBTransaction::Commit, which aborts.

+  if (backing_store_->is_incognito())
+    return true;
+
+  BlobChangeMap::const_iterator iter = blob_change_map_.begin();
+  // 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.
+  if (iter != blob_change_map_.end()) {
+    scoped_ptr<LevelDBIterator> db_iter = transaction_->CreateIterator();
+    for (; iter != blob_change_map_.end(); ++iter) {
+      BlobEntryKey blob_entry_key;
+      StringPiece key_piece(iter->second->key());
+      if (!BlobEntryKey::FromObjectStoreDataKey(&key_piece, &blob_entry_key)) {
+        NOTREACHED();
+        INTERNAL_WRITE_ERROR(TRANSACTION_COMMIT_METHOD);
+        transaction_ = NULL;
+        return false;
+      }
+      if (database_id_ < 0)
+        database_id_ = blob_entry_key.database_id();
+      else
+        DCHECK_EQ(database_id_, blob_entry_key.database_id());
+      std::string blob_entry_key_bytes = blob_entry_key.Encode();
+      db_iter->Seek(blob_entry_key_bytes);
+      if (db_iter->IsValid() &&
+          !CompareKeys(db_iter->Key(), blob_entry_key_bytes)) {
+        std::vector<IndexedDBBlobInfo> blob_info;
+        if (!DecodeBlobData(db_iter->Value().as_string(), &blob_info)) {
+          INTERNAL_READ_ERROR(TRANSACTION_COMMIT_METHOD);
+          transaction_ = NULL;
+          return false;
+        }
+        std::vector<IndexedDBBlobInfo>::iterator blob_info_iter;
+        for (blob_info_iter = blob_info.begin();
+             blob_info_iter != blob_info.end();
+             ++blob_info_iter)
+          blobs_to_remove_.push_back(
+              std::make_pair(database_id_, blob_info_iter->key()));
+        transaction_->Remove(blob_entry_key_bytes);
+      }
+    }
+  }
+  return true;
+}
+
+leveldb::Status IndexedDBBackingStore::Transaction::SortBlobsToRemove() {

[cmumford, 2014/05/28 20:34:54]

Does this method actually sort (reorder) anything?

[jsbell, 2014/05/28 21:29:15]

'Sort' is a bit misleading here - can we use another verb? 'Identify',
'Prepare', ... ?

[ericu, 2014/05/28 22:50:42]

No, it's just sorting into two bins [live_blob or primary].
I'll rename to "Collect".

+  IndexedDBActiveBlobRegistry* registry =
+      backing_store_->active_blob_registry();
+  BlobJournalType::iterator iter;
+  BlobJournalType primary_journal, live_blob_journal;
+  for (iter = blobs_to_remove_.begin(); iter != blobs_to_remove_.end();
+       ++iter) {
+    if (registry->MarkDeletedCheckIfUsed(iter->first, iter->second))
+      live_blob_journal.push_back(*iter);
+    else
+      primary_journal.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_);
+  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(TRANSACTION_COMMIT_METHOD);
+    transaction_ = NULL;
+    return s;
+  }
+
+  DCHECK(!new_files_to_write.size() ||
+         KeyPrefix::IsValidDatabaseId(database_id_));
+  if (!CollectBlobFilesToRemove()) {
+    INTERNAL_WRITE_ERROR(TRANSACTION_COMMIT_METHOD);

[cmumford, 2014/05/28 20:34:54]

Should use the _UNTESTED versions of this macro in most (all) places unless line
exercised by a unit test.

[ericu, 2014/05/28 22:50:42]

Done; I'll get to the failure unit tests after we've landed everything.

+    transaction_ = NULL;
+    return InternalInconsistencyStatus();
+  }
+
+  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_WRITE_ERROR(TRANSACTION_COMMIT_METHOD);

[cmumford, 2014/05/28 20:34:54]

SortBlobsToRemove doesn't write to the db (does it?) Should this be
INTERNAL_READ_ERROR? Also, you may need the _UNTESTED variant if this isn't hit
by a unit test.

[ericu, 2014/05/28 22:50:42]

It writes to the transaction, not the db, so while it's logically writing, any
errors would actually come from reading the journal it's merging into.  Changed
to READ UNTESTED.

+      transaction_ = NULL;
+      return s;
+    }
+  }
+
+  s = transaction_->Commit();
   transaction_ = NULL;
 
   if (s.ok() && backing_store_->is_incognito() && !blob_change_map_.empty()) {
     BlobChangeMap& target_map = backing_store_->incognito_blob_map_;
     BlobChangeMap::iterator iter;
     for (iter = blob_change_map_.begin(); iter != blob_change_map_.end();
          ++iter) {
       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_UNTESTED(TRANSACTION_COMMIT_METHOD);
+  else if (blobs_to_remove_.size())
+    s = backing_store_->CleanUpBlobJournal(BlobJournalKey::Encode());
+
   return s;
 }
 
 
 class IndexedDBBackingStore::Transaction::BlobWriteCallbackWrapper
     : public IndexedDBBackingStore::BlobWriteCallback {
  public:
   BlobWriteCallbackWrapper(IndexedDBBackingStore::Transaction* transaction,
                            scoped_refptr<BlobWriteCallback> callback)
       : transaction_(transaction), callback_(callback) {}
@@ skipping 115 matching lines @@
     const GURL& url,
     int64_t key)
     : is_file_(false), url_(url), key_(key) {}
 
 IndexedDBBackingStore::Transaction::WriteDescriptor::WriteDescriptor(
     const FilePath& file_path,
     int64_t key)
     : is_file_(true), file_path_(file_path), key_(key) {}
 
 }  // namespace content