Make iterating over a corrupted IndexedDB fail.

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/json/json_reader.h"
 #include "base/json/json_writer.h"
@@ skipping 60 matching lines @@
   VERSION_EXISTS,
   DELETE_OBJECT_STORE,
   SET_MAX_OBJECT_STORE_ID,
   SET_MAX_INDEX_ID,
   GET_NEW_DATABASE_ID,
   GET_NEW_VERSION_NUMBER,
   CREATE_IDBDATABASE_METADATA,
   DELETE_DATABASE,
   TRANSACTION_COMMIT_METHOD,  // TRANSACTION_COMMIT is a WinNT.h macro
   GET_DATABASE_NAMES,
+  DELETE_INDEX,
+  CLEAR_OBJECT_STORE,
   INTERNAL_ERROR_MAX,
 };
 
 static void RecordInternalError(const char* type,
                                 IndexedDBBackingStoreErrorSource location) {
   std::string name;
   name.append("WebCore.IndexedDB.BackingStore.").append(type).append("Error");
   base::Histogram::FactoryGet(name,
                               1,
                               INTERNAL_ERROR_MAX,
@@ skipping 225 matching lines @@
     // Upgrade old backing store.
     DCHECK_LE(db_schema_version, kLatestKnownSchemaVersion);
     if (db_schema_version < 1) {
       db_schema_version = 1;
       PutInt(transaction.get(), schema_version_key, db_schema_version);
       const std::string start_key =
           DatabaseNameKey::EncodeMinKeyForOrigin(origin_identifier);
       const std::string stop_key =
           DatabaseNameKey::EncodeStopKeyForOrigin(origin_identifier);
       scoped_ptr<LevelDBIterator> it = db->CreateIterator();
-      for (it->Seek(start_key);
-           it->IsValid() && CompareKeys(it->Key(), stop_key) < 0;
-           it->Next()) {
+      for (s = it->Seek(start_key);
+           s.ok() && it->IsValid() && CompareKeys(it->Key(), stop_key) < 0;
+           s = it->Next()) {
         int64 database_id = 0;
         found = false;
         s = GetInt(transaction.get(), it->Key(), &database_id, &found);
         if (!s.ok()) {
           INTERNAL_READ_ERROR_UNTESTED(SET_UP_METADATA);
           return false;
         }
         if (!found) {
           INTERNAL_CONSISTENCY_ERROR_UNTESTED(SET_UP_METADATA);
           return false;
         }
         std::string int_version_key = DatabaseMetaDataKey::Encode(
             database_id, DatabaseMetaDataKey::USER_INT_VERSION);
         PutVarInt(transaction.get(),
                   int_version_key,
                   IndexedDBDatabaseMetadata::DEFAULT_INT_VERSION);
       }
     }
     if (db_schema_version < 2) {

[jsbell, 2014/04/14 20:44:20]

Not strictly necessary, but I'd add a `s->ok() &&` clause here.

[cmumford, 2014/04/14 23:39:23]

Done.

       db_schema_version = 2;
       PutInt(transaction.get(), schema_version_key, db_schema_version);
       db_data_version = blink::kSerializedScriptValueVersion;
       PutInt(transaction.get(), data_version_key, db_data_version);
     }
   }
 
+  if (!s.ok()) {
+    INTERNAL_READ_ERROR_UNTESTED(SET_UP_METADATA);
+    return false;
+  }
+
   // All new values will be written using this serialization version.
   found = false;
   s = GetInt(transaction.get(), data_version_key, &db_data_version, &found);
   if (!s.ok()) {
     INTERNAL_READ_ERROR_UNTESTED(SET_UP_METADATA);
     return false;
   }
   if (!found) {
     INTERNAL_CONSISTENCY_ERROR_UNTESTED(SET_UP_METADATA);
     return false;
@@ skipping 438 matching lines @@
                                 db.Pass(),
                                 comparator.Pass(),
                                 task_runner));
   if (!SetUpMetadata(backing_store->db_.get(),
                      backing_store->origin_identifier_))
     return scoped_refptr<IndexedDBBackingStore>();
 
   return backing_store;
 }
 
-std::vector<base::string16> IndexedDBBackingStore::GetDatabaseNames() {
+std::vector<base::string16> IndexedDBBackingStore::GetDatabaseNames(
+    leveldb::Status& s) {
+  s = leveldb::Status::OK();
   std::vector<base::string16> found_names;
   const std::string start_key =
       DatabaseNameKey::EncodeMinKeyForOrigin(origin_identifier_);
   const std::string stop_key =
       DatabaseNameKey::EncodeStopKeyForOrigin(origin_identifier_);
 
   DCHECK(found_names.empty());
 
   scoped_ptr<LevelDBIterator> it = db_->CreateIterator();
-  for (it->Seek(start_key);
-       it->IsValid() && CompareKeys(it->Key(), stop_key) < 0;
-       it->Next()) {
+  for (s = it->Seek(start_key);
+       s.ok() && it->IsValid() && CompareKeys(it->Key(), stop_key) < 0;
+       s = it->Next()) {
     StringPiece slice(it->Key());
     DatabaseNameKey database_name_key;
     if (!DatabaseNameKey::Decode(&slice, &database_name_key)) {
       INTERNAL_CONSISTENCY_ERROR_UNTESTED(GET_DATABASE_NAMES);
       continue;
     }
     found_names.push_back(database_name_key.database_name());
   }
+
+  if (!s.ok())
+    INTERNAL_READ_ERROR_UNTESTED(GET_DATABASE_NAMES);
+
   return found_names;
 }
 
 leveldb::Status IndexedDBBackingStore::GetIDBDatabaseMetaData(
     const base::string16& name,
     IndexedDBDatabaseMetadata* metadata,
     bool* found) {
   const std::string key = DatabaseNameKey::Encode(origin_identifier_, name);
   *found = false;
 
   leveldb::Status s = GetInt(db_.get(), key, &metadata->id, found);
   if (!s.ok()) {
-    INTERNAL_READ_ERROR_UNTESTED(GET_IDBDATABASE_METADATA);
+    INTERNAL_READ_ERROR(GET_IDBDATABASE_METADATA);
     return s;
   }
   if (!*found)
     return leveldb::Status::OK();
 
   s = GetString(db_.get(),
                 DatabaseMetaDataKey::Encode(metadata->id,
                                             DatabaseMetaDataKey::USER_VERSION),
                 &metadata->version,
                 found);
@@ skipping 95 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 void DeleteRange(LevelDBTransaction* transaction,
-                        const std::string& begin,
-                        const std::string& end) {
+static leveldb::Status DeleteRange(LevelDBTransaction* transaction,
+                                   const std::string& begin,
+                                   const std::string& end) {
   scoped_ptr<LevelDBIterator> it = transaction->CreateIterator();
-  for (it->Seek(begin); it->IsValid() && CompareKeys(it->Key(), end) < 0;
-       it->Next())
+  leveldb::Status s;
+  for (s = it->Seek(begin);
+       s.ok() && it->IsValid() && CompareKeys(it->Key(), end) < 0;
+       s = it->Next())
     transaction->Remove(it->Key());
+  return s;
 }
 
 leveldb::Status IndexedDBBackingStore::DeleteDatabase(
     const base::string16& name) {
   IDB_TRACE("IndexedDBBackingStore::DeleteDatabase");
   scoped_ptr<LevelDBDirectTransaction> transaction =
       LevelDBDirectTransaction::Create(db_.get());
 
   IndexedDBDatabaseMetadata metadata;
   bool success = false;
   leveldb::Status s = GetIDBDatabaseMetaData(name, &metadata, &success);
   if (!s.ok())
     return s;
   if (!success)
     return leveldb::Status::OK();
 
   const std::string start_key = DatabaseMetaDataKey::Encode(
       metadata.id, DatabaseMetaDataKey::ORIGIN_NAME);
   const std::string stop_key = DatabaseMetaDataKey::Encode(
       metadata.id + 1, DatabaseMetaDataKey::ORIGIN_NAME);
   scoped_ptr<LevelDBIterator> it = db_->CreateIterator();
-  for (it->Seek(start_key);
-       it->IsValid() && CompareKeys(it->Key(), stop_key) < 0;
-       it->Next())
+  for (s = it->Seek(start_key);
+       s.ok() && it->IsValid() && CompareKeys(it->Key(), stop_key) < 0;
+       s = it->Next())
     transaction->Remove(it->Key());
+  if (!s.ok()) {
+    INTERNAL_WRITE_ERROR_UNTESTED(DELETE_DATABASE);
+    return s;
+  }
 
   const std::string key = DatabaseNameKey::Encode(origin_identifier_, name);
   transaction->Remove(key);
 
   s = transaction->Commit();
   if (!s.ok()) {
     INTERNAL_WRITE_ERROR_UNTESTED(DELETE_DATABASE);
     return s;
   }
   db_->Compact(start_key, stop_key);
@@ skipping 27 matching lines @@
   if (!KeyPrefix::IsValidDatabaseId(database_id))
     return InvalidDBKeyStatus();
   const std::string start_key =
       ObjectStoreMetaDataKey::Encode(database_id, 1, 0);
   const std::string stop_key =
       ObjectStoreMetaDataKey::EncodeMaxKey(database_id);
 
   DCHECK(object_stores->empty());
 
   scoped_ptr<LevelDBIterator> it = db_->CreateIterator();
-  it->Seek(start_key);
-  while (it->IsValid() && CompareKeys(it->Key(), stop_key) < 0) {
+  leveldb::Status s = it->Seek(start_key);
+  while (s.ok() && it->IsValid() && CompareKeys(it->Key(), stop_key) < 0) {
     StringPiece slice(it->Key());
     ObjectStoreMetaDataKey meta_data_key;
     bool ok = ObjectStoreMetaDataKey::Decode(&slice, &meta_data_key);
     DCHECK(ok);
     if (meta_data_key.MetaDataType() != ObjectStoreMetaDataKey::NAME) {
       INTERNAL_CONSISTENCY_ERROR_UNTESTED(GET_OBJECT_STORES);
       // Possible stale metadata, but don't fail the load.
-      it->Next();
+      s = it->Next();

[jsbell, 2014/04/14 20:44:20]

Check value of s here? (see next comment...)

       continue;
     }
 
     int64 object_store_id = meta_data_key.ObjectStoreId();
 
     // TODO(jsbell): Do this by direct key lookup rather than iteration, to
     // simplify.
     base::string16 object_store_name;
     {
       StringPiece slice(it->Value());
       if (!DecodeString(&slice, &object_store_name) || !slice.empty())
         INTERNAL_CONSISTENCY_ERROR_UNTESTED(GET_OBJECT_STORES);
     }
 
-    it->Next();
+    s = it->Next();
+    if (!s.ok())
+      continue;

[jsbell, 2014/04/14 20:44:20]

continue or break?

They'll do the same thing since the first clause in the while() test is s.ok(),
but break might be clearer to readers.

[cmumford, 2014/04/14 23:39:23]

I'll do what you think is most obvious, but if we do breaks everywhere then the
check in the while loop is redundant (which is odd), and if we remove that check
then that's also odd as future editors will be expecting it to be checked. Just
my 2¢

I'd rather just throw an exception ;-)

     if (!CheckObjectStoreAndMetaDataType(it.get(),
                                          stop_key,
                                          object_store_id,
                                          ObjectStoreMetaDataKey::KEY_PATH)) {
       INTERNAL_CONSISTENCY_ERROR_UNTESTED(GET_OBJECT_STORES);
       break;
     }
     IndexedDBKeyPath key_path;
     {
       StringPiece slice(it->Value());
       if (!DecodeIDBKeyPath(&slice, &key_path) || !slice.empty())
         INTERNAL_CONSISTENCY_ERROR_UNTESTED(GET_OBJECT_STORES);
     }
 
-    it->Next();
+    s = it->Next();
+    if (!s.ok())
+      continue;
     if (!CheckObjectStoreAndMetaDataType(
              it.get(),
              stop_key,
              object_store_id,
              ObjectStoreMetaDataKey::AUTO_INCREMENT)) {
       INTERNAL_CONSISTENCY_ERROR_UNTESTED(GET_OBJECT_STORES);
       break;
     }
     bool auto_increment;
     {
       StringPiece slice(it->Value());
       if (!DecodeBool(&slice, &auto_increment) || !slice.empty())
         INTERNAL_CONSISTENCY_ERROR_UNTESTED(GET_OBJECT_STORES);
     }
 
-    it->Next();  // Is evicatble.
+    s = it->Next();  // Is evicatble.
+    if (!s.ok())
+      continue;
     if (!CheckObjectStoreAndMetaDataType(it.get(),
                                          stop_key,
                                          object_store_id,
                                          ObjectStoreMetaDataKey::EVICTABLE)) {
       INTERNAL_CONSISTENCY_ERROR_UNTESTED(GET_OBJECT_STORES);
       break;
     }
 
-    it->Next();  // Last version.
+    s = it->Next();  // Last version.
+    if (!s.ok())
+      continue;
     if (!CheckObjectStoreAndMetaDataType(
              it.get(),
              stop_key,
              object_store_id,
              ObjectStoreMetaDataKey::LAST_VERSION)) {
       INTERNAL_CONSISTENCY_ERROR_UNTESTED(GET_OBJECT_STORES);
       break;
     }
 
-    it->Next();  // Maximum index id allocated.
+    s = it->Next();  // Maximum index id allocated.
+    if (!s.ok())
+      continue;
     if (!CheckObjectStoreAndMetaDataType(
              it.get(),
              stop_key,
              object_store_id,
              ObjectStoreMetaDataKey::MAX_INDEX_ID)) {
       INTERNAL_CONSISTENCY_ERROR_UNTESTED(GET_OBJECT_STORES);
       break;
     }
     int64 max_index_id;
     {
       StringPiece slice(it->Value());
       if (!DecodeInt(&slice, &max_index_id) || !slice.empty())
         INTERNAL_CONSISTENCY_ERROR_UNTESTED(GET_OBJECT_STORES);
     }
 
-    it->Next();  // [optional] has key path (is not null)
+    s = it->Next();  // [optional] has key path (is not null)
+    if (!s.ok())
+      continue;
     if (CheckObjectStoreAndMetaDataType(it.get(),
                                         stop_key,
                                         object_store_id,
                                         ObjectStoreMetaDataKey::HAS_KEY_PATH)) {
       bool has_key_path;
       {
         StringPiece slice(it->Value());
         if (!DecodeBool(&slice, &has_key_path))
           INTERNAL_CONSISTENCY_ERROR_UNTESTED(GET_OBJECT_STORES);
       }
       // This check accounts for two layers of legacy coding:
       // (1) Initially, has_key_path was added to distinguish null vs. string.
       // (2) Later, null vs. string vs. array was stored in the key_path itself.
       // So this check is only relevant for string-type key_paths.
       if (!has_key_path &&
           (key_path.type() == blink::WebIDBKeyPathTypeString &&
            !key_path.string().empty())) {
         INTERNAL_CONSISTENCY_ERROR_UNTESTED(GET_OBJECT_STORES);
         break;
       }
       if (!has_key_path)
         key_path = IndexedDBKeyPath();
-      it->Next();
+      s = it->Next();
+      if (!s.ok())
+        continue;
     }
 
     int64 key_generator_current_number = -1;
     if (CheckObjectStoreAndMetaDataType(
             it.get(),
             stop_key,
             object_store_id,
             ObjectStoreMetaDataKey::KEY_GENERATOR_CURRENT_NUMBER)) {
       StringPiece slice(it->Value());
       if (!DecodeInt(&slice, &key_generator_current_number) || !slice.empty())
         INTERNAL_CONSISTENCY_ERROR_UNTESTED(GET_OBJECT_STORES);
 
       // TODO(jsbell): Return key_generator_current_number, cache in
       // object store, and write lazily to backing store.  For now,
       // just assert that if it was written it was valid.
       DCHECK_GE(key_generator_current_number, kKeyGeneratorInitialNumber);
-      it->Next();
+      s = it->Next();
+      if (!s.ok())
+        continue;
     }
 
     IndexedDBObjectStoreMetadata metadata(object_store_name,
                                           object_store_id,
                                           key_path,
                                           auto_increment,
                                           max_index_id);
-    leveldb::Status s =
-        GetIndexes(database_id, object_store_id, &metadata.indexes);
+    s = GetIndexes(database_id, object_store_id, &metadata.indexes);
     if (!s.ok())
-      return s;
+      continue;
     (*object_stores)[object_store_id] = metadata;
   }
-  return leveldb::Status::OK();
+
+  if (!s.ok())
+    INTERNAL_READ_ERROR_UNTESTED(GET_OBJECT_STORES);
+
+  return s;
 }
 
 WARN_UNUSED_RESULT static leveldb::Status SetMaxObjectStoreId(
     LevelDBTransaction* transaction,
     int64 database_id,
     int64 object_store_id) {
   const std::string max_object_store_id_key = DatabaseMetaDataKey::Encode(
       database_id, DatabaseMetaDataKey::MAX_OBJECT_STORE_ID);
   int64 max_object_store_id = -1;
   leveldb::Status s = GetMaxObjectStoreId(
@@ skipping 83 matching lines @@
                 &found);
   if (!s.ok()) {
     INTERNAL_READ_ERROR_UNTESTED(DELETE_OBJECT_STORE);
     return s;
   }
   if (!found) {
     INTERNAL_CONSISTENCY_ERROR_UNTESTED(DELETE_OBJECT_STORE);
     return InternalInconsistencyStatus();
   }
 
-  DeleteRange(
+  s = DeleteRange(
       leveldb_transaction,
       ObjectStoreMetaDataKey::Encode(database_id, object_store_id, 0),
       ObjectStoreMetaDataKey::EncodeMaxKey(database_id, object_store_id));
 
-  leveldb_transaction->Remove(
-      ObjectStoreNamesKey::Encode(database_id, object_store_name));
+  if (s.ok()) {
+    leveldb_transaction->Remove(
+        ObjectStoreNamesKey::Encode(database_id, object_store_name));
 
-  DeleteRange(leveldb_transaction,
-              IndexFreeListKey::Encode(database_id, object_store_id, 0),
-              IndexFreeListKey::EncodeMaxKey(database_id, object_store_id));
-  DeleteRange(leveldb_transaction,
-              IndexMetaDataKey::Encode(database_id, object_store_id, 0, 0),
-              IndexMetaDataKey::EncodeMaxKey(database_id, object_store_id));
+    s = DeleteRange(
+        leveldb_transaction,
+        IndexFreeListKey::Encode(database_id, object_store_id, 0),
+        IndexFreeListKey::EncodeMaxKey(database_id, object_store_id));
+  }
+
+  if (s.ok()) {
+    s = DeleteRange(
+        leveldb_transaction,
+        IndexMetaDataKey::Encode(database_id, object_store_id, 0, 0),
+        IndexMetaDataKey::EncodeMaxKey(database_id, object_store_id));
+  }
+  if (!s.ok()) {

[jsbell, 2014/04/14 20:44:20]

Nit: make the blank lines before the if()s consistent

+    INTERNAL_WRITE_ERROR_UNTESTED(DELETE_OBJECT_STORE);
+    return s;
+  }
 
   return ClearObjectStore(transaction, database_id, object_store_id);
 }
 
 leveldb::Status IndexedDBBackingStore::GetRecord(
     IndexedDBBackingStore::Transaction* transaction,
     int64 database_id,
     int64 object_store_id,
     const IndexedDBKey& key,
     IndexedDBValue* record) {
@@ skipping 109 matching lines @@
     int64 database_id,
     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();
 
-  DeleteRange(transaction->transaction(), start_key, stop_key);
-  return leveldb::Status::OK();
+  leveldb::Status s =
+      DeleteRange(transaction->transaction(), start_key, stop_key);
+  if (!s.ok())
+    INTERNAL_WRITE_ERROR_UNTESTED(CLEAR_OBJECT_STORE);
+  return s;
 }
 
 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();
@@ skipping 49 matching lines @@
   // key generator state must be preserved.
   // TODO(jsbell): Fix this for all stores on database open?
   const std::string start_key =
       ObjectStoreDataKey::Encode(database_id, object_store_id, MinIDBKey());
   const std::string stop_key =
       ObjectStoreDataKey::Encode(database_id, object_store_id, MaxIDBKey());
 
   scoped_ptr<LevelDBIterator> it = leveldb_transaction->CreateIterator();
   int64 max_numeric_key = 0;
 
-  for (it->Seek(start_key);
-       it->IsValid() && CompareKeys(it->Key(), stop_key) < 0;
-       it->Next()) {
+  for (s = it->Seek(start_key);
+       s.ok() && it->IsValid() && CompareKeys(it->Key(), stop_key) < 0;
+       s = it->Next()) {
     StringPiece slice(it->Key());
     ObjectStoreDataKey data_key;
     if (!ObjectStoreDataKey::Decode(&slice, &data_key)) {
       INTERNAL_READ_ERROR_UNTESTED(GET_KEY_GENERATOR_CURRENT_NUMBER);
       return InternalInconsistencyStatus();
     }
     scoped_ptr<IndexedDBKey> user_key = data_key.user_key();
     if (user_key->type() == blink::WebIDBKeyTypeNumber) {
       int64 n = static_cast<int64>(user_key->number());
       if (n > max_numeric_key)
         max_numeric_key = n;
     }
   }
 
-  *key_generator_current_number = max_numeric_key + 1;
+  if (s.ok())
+    *key_generator_current_number = max_numeric_key + 1;
+  else
+    INTERNAL_READ_ERROR_UNTESTED(GET_KEY_GENERATOR_CURRENT_NUMBER);
+
   return s;
 }
 
 leveldb::Status IndexedDBBackingStore::MaybeUpdateKeyGeneratorCurrentNumber(
     IndexedDBBackingStore::Transaction* transaction,
     int64 database_id,
     int64 object_store_id,
     int64 new_number,
     bool check_current) {
   if (!KeyPrefix::ValidIds(database_id, object_store_id))
@@ skipping 86 matching lines @@
   if (!KeyPrefix::ValidIds(database_id, object_store_id))
     return InvalidDBKeyStatus();
   const std::string start_key =
       IndexMetaDataKey::Encode(database_id, object_store_id, 0, 0);
   const std::string stop_key =
       IndexMetaDataKey::Encode(database_id, object_store_id + 1, 0, 0);
 
   DCHECK(indexes->empty());
 
   scoped_ptr<LevelDBIterator> it = db_->CreateIterator();
-  it->Seek(start_key);
-  while (it->IsValid() && CompareKeys(it->Key(), stop_key) < 0) {
+  leveldb::Status s = it->Seek(start_key);
+  while (s.ok() && it->IsValid() && CompareKeys(it->Key(), stop_key) < 0) {
     StringPiece slice(it->Key());
     IndexMetaDataKey meta_data_key;
     bool ok = IndexMetaDataKey::Decode(&slice, &meta_data_key);
     DCHECK(ok);
     if (meta_data_key.meta_data_type() != IndexMetaDataKey::NAME) {
       INTERNAL_CONSISTENCY_ERROR_UNTESTED(GET_INDEXES);
       // Possible stale metadata due to http://webkit.org/b/85557 but don't fail
       // the load.
-      it->Next();
+      s = it->Next();

[jsbell, 2014/04/14 20:44:20]

Same as GetObjectStores here - `if(!s.ok()) break;` ?

[cmumford, 2014/04/14 23:39:23]

Done.

       continue;
     }
 
     // TODO(jsbell): Do this by direct key lookup rather than iteration, to
     // simplify.
     int64 index_id = meta_data_key.IndexId();
     base::string16 index_name;
     {
       StringPiece slice(it->Value());
       if (!DecodeString(&slice, &index_name) || !slice.empty())
         INTERNAL_CONSISTENCY_ERROR_UNTESTED(GET_INDEXES);
     }
 
-    it->Next();  // unique flag
+    s = it->Next();  // unique flag
+    if (!s.ok())
+      continue;

[jsbell, 2014/04/14 20:44:20]

Same as GetObjectStores here - `continue` or `break` ?

[cmumford, 2014/04/14 23:39:23]

Done.

     if (!CheckIndexAndMetaDataKey(
              it.get(), stop_key, index_id, IndexMetaDataKey::UNIQUE)) {
       INTERNAL_CONSISTENCY_ERROR_UNTESTED(GET_INDEXES);
       break;
     }
     bool index_unique;
     {
       StringPiece slice(it->Value());
       if (!DecodeBool(&slice, &index_unique) || !slice.empty())
         INTERNAL_CONSISTENCY_ERROR_UNTESTED(GET_INDEXES);
     }
 
-    it->Next();  // key_path
+    s = it->Next();  // key_path
+    if (!s.ok())
+      continue;
     if (!CheckIndexAndMetaDataKey(
              it.get(), stop_key, index_id, IndexMetaDataKey::KEY_PATH)) {
       INTERNAL_CONSISTENCY_ERROR_UNTESTED(GET_INDEXES);
       break;
     }
     IndexedDBKeyPath key_path;
     {
       StringPiece slice(it->Value());
       if (!DecodeIDBKeyPath(&slice, &key_path) || !slice.empty())
         INTERNAL_CONSISTENCY_ERROR_UNTESTED(GET_INDEXES);
     }
 
-    it->Next();  // [optional] multi_entry flag
+    s = it->Next();  // [optional] multi_entry flag
+    if (!s.ok())
+      continue;
     bool index_multi_entry = false;
     if (CheckIndexAndMetaDataKey(
             it.get(), stop_key, index_id, IndexMetaDataKey::MULTI_ENTRY)) {
       StringPiece slice(it->Value());
       if (!DecodeBool(&slice, &index_multi_entry) || !slice.empty())
         INTERNAL_CONSISTENCY_ERROR_UNTESTED(GET_INDEXES);
 
-      it->Next();
+      s = it->Next();
+      if (!s.ok())
+        continue;
     }
 
     (*indexes)[index_id] = IndexedDBIndexMetadata(
         index_name, index_id, key_path, index_unique, index_multi_entry);
   }
-  return leveldb::Status::OK();
+
+  if (!s.ok())
+    INTERNAL_READ_ERROR_UNTESTED(GET_INDEXES);
+
+  return s;
 }
 
 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 58 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);
-  DeleteRange(leveldb_transaction, index_meta_data_start, index_meta_data_end);
+  leveldb::Status s = DeleteRange(
+      leveldb_transaction, index_meta_data_start, index_meta_data_end);
 
-  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);
-  DeleteRange(leveldb_transaction, index_data_start, index_data_end);
-  return leveldb::Status::OK();
+  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);
+  }
+
+  if (!s.ok())
+    INTERNAL_WRITE_ERROR_UNTESTED(DELETE_INDEX);
+
+  return s;
 }
 
 leveldb::Status IndexedDBBackingStore::PutIndexDataForRecord(
     IndexedDBBackingStore::Transaction* transaction,
     int64 database_id,
     int64 object_store_id,
     int64 index_id,
     const IndexedDBKey& key,
     const RecordIdentifier& record_identifier) {
   IDB_TRACE("IndexedDBBackingStore::PutIndexDataForRecord");
@@ skipping 15 matching lines @@
   std::string data;
   EncodeVarInt(record_identifier.version(), &data);
   data.append(record_identifier.primary_key());
 
   transaction->transaction()->Put(index_data_key, &data);
   return leveldb::Status::OK();
 }
 
 static bool FindGreatestKeyLessThanOrEqual(LevelDBTransaction* transaction,
                                            const std::string& target,
-                                           std::string* found_key) {
+                                           std::string* found_key,
+                                           leveldb::Status& s) {
   scoped_ptr<LevelDBIterator> it = transaction->CreateIterator();
-  it->Seek(target);
+  s = it->Seek(target);
+  if (!s.ok())
+    return false;
 
   if (!it->IsValid()) {
-    it->SeekToLast();
-    if (!it->IsValid())
+    s = it->SeekToLast();
+    if (!s.ok() || !it->IsValid())
       return false;
   }
 
   while (CompareIndexKeys(it->Key(), target) > 0) {
-    it->Prev();
-    if (!it->IsValid())
+    s = it->Prev();
+    if (!s.ok() || !it->IsValid())
       return false;
   }
 
   do {
     *found_key = it->Key().as_string();
 
     // There can be several index keys that compare equal. We want the last one.
-    it->Next();
-  } while (it->IsValid() && !CompareIndexKeys(it->Key(), target));
+    s = it->Next();
+  } while (s.ok() && it->IsValid() && !CompareIndexKeys(it->Key(), target));
 
   return true;
 }
 
 static leveldb::Status VersionExists(LevelDBTransaction* transaction,
                                      int64 database_id,
                                      int64 object_store_id,
                                      int64 version,
                                      const std::string& encoded_primary_key,
                                      bool* exists) {
@@ skipping 28 matching lines @@
   IDB_TRACE("IndexedDBBackingStore::FindKeyInIndex");
   DCHECK(KeyPrefix::ValidIds(database_id, object_store_id, index_id));
 
   DCHECK(found_encoded_primary_key->empty());
   *found = false;
 
   LevelDBTransaction* leveldb_transaction = transaction->transaction();
   const std::string leveldb_key =
       IndexDataKey::Encode(database_id, object_store_id, index_id, key);
   scoped_ptr<LevelDBIterator> it = leveldb_transaction->CreateIterator();
-  it->Seek(leveldb_key);
+  leveldb::Status s = it->Seek(leveldb_key);
+  if (!s.ok()) {
+    INTERNAL_READ_ERROR_UNTESTED(FIND_KEY_IN_INDEX);
+    return s;
+  }
 
   for (;;) {
     if (!it->IsValid())
       return leveldb::Status::OK();
     if (CompareIndexKeys(it->Key(), leveldb_key) > 0)
       return leveldb::Status::OK();
 
     StringPiece slice(it->Value());
 
     int64 version;
     if (!DecodeVarInt(&slice, &version)) {
       INTERNAL_READ_ERROR_UNTESTED(FIND_KEY_IN_INDEX);
       return InternalInconsistencyStatus();
     }
     *found_encoded_primary_key = slice.as_string();
 
     bool exists = false;
-    leveldb::Status s = VersionExists(leveldb_transaction,
-                                      database_id,
-                                      object_store_id,
-                                      version,
-                                      *found_encoded_primary_key,
-                                      &exists);
+    s = VersionExists(leveldb_transaction,
+                      database_id,
+                      object_store_id,
+                      version,
+                      *found_encoded_primary_key,
+                      &exists);
     if (!s.ok())
       return s;
     if (!exists) {
       // Delete stale index data entry and continue.
       leveldb_transaction->Remove(it->Key());
-      it->Next();
+      s = it->Next();
       continue;
     }
     *found = true;
     return s;
   }
 }
 
 leveldb::Status IndexedDBBackingStore::GetPrimaryKeyViaIndex(
     IndexedDBBackingStore::Transaction* transaction,
     int64 database_id,
@@ skipping 78 matching lines @@
 
 IndexedDBBackingStore::Cursor::Cursor(
     const IndexedDBBackingStore::Cursor* other)
     : transaction_(other->transaction_),
       cursor_options_(other->cursor_options_),
       current_key_(new IndexedDBKey(*other->current_key_)) {
   if (other->iterator_) {
     iterator_ = transaction_->CreateIterator();
 
     if (other->iterator_->IsValid()) {
-      iterator_->Seek(other->iterator_->Key());
+      leveldb::Status s = iterator_->Seek(other->iterator_->Key());
+      // TODO(cmumford): Handle this error
       DCHECK(iterator_->IsValid());
     }
   }
 }
 
 IndexedDBBackingStore::Cursor::Cursor(LevelDBTransaction* transaction,
                                       const CursorOptions& cursor_options)
     : transaction_(transaction), cursor_options_(cursor_options) {}
 IndexedDBBackingStore::Cursor::~Cursor() {}
 
-bool IndexedDBBackingStore::Cursor::FirstSeek() {
+bool IndexedDBBackingStore::Cursor::FirstSeek(leveldb::Status& s) {
   iterator_ = transaction_->CreateIterator();
   if (cursor_options_.forward)
-    iterator_->Seek(cursor_options_.low_key);
+    s = iterator_->Seek(cursor_options_.low_key);
   else
-    iterator_->Seek(cursor_options_.high_key);
+    s = iterator_->Seek(cursor_options_.high_key);
+  if (!s.ok())
+    return false;
 
-  return Continue(0, READY);
+  return Continue(0, READY, s);
 }
 
-bool IndexedDBBackingStore::Cursor::Advance(uint32 count) {
+bool IndexedDBBackingStore::Cursor::Advance(uint32 count, leveldb::Status& s) {
+  s = leveldb::Status::OK();
   while (count--) {
-    if (!Continue())
+    if (!Continue(s))
       return false;
   }
   return true;
 }
 
 bool IndexedDBBackingStore::Cursor::Continue(const IndexedDBKey* key,
                                              const IndexedDBKey* primary_key,
-                                             IteratorState next_state) {
+                                             IteratorState next_state,
+                                             leveldb::Status& s) {
   DCHECK(!key || key->IsValid());
   DCHECK(!primary_key || primary_key->IsValid());
+  s = leveldb::Status::OK();
 
   // TODO(alecflett): avoid a copy here?
   IndexedDBKey previous_key = current_key_ ? *current_key_ : IndexedDBKey();
 
   bool first_iteration = true;
 
   // When iterating with PrevNoDuplicate, spec requires that the
   // value we yield for each key is the first duplicate in forwards
   // order.
   IndexedDBKey last_duplicate_key;
 
   bool forward = cursor_options_.forward;
 
   for (;;) {
     if (next_state == SEEK) {
       // TODO(jsbell): Optimize seeking for reverse cursors as well.
       if (first_iteration && key && forward) {
         std::string leveldb_key;
         if (primary_key) {
           leveldb_key = EncodeKey(*key, *primary_key);
         } else {
           leveldb_key = EncodeKey(*key);
         }
-        iterator_->Seek(leveldb_key);
+        s = iterator_->Seek(leveldb_key);
         first_iteration = false;
       } else if (forward) {
-        iterator_->Next();
+        s = iterator_->Next();
       } else {
-        iterator_->Prev();
+        s = iterator_->Prev();
       }
+      if (!s.ok())
+        return false;
     } else {
       next_state = SEEK;  // for subsequent iterations
     }
 
     if (!iterator_->IsValid()) {
       if (!forward && last_duplicate_key.IsValid()) {
         // We need to walk forward because we hit the end of
         // the data.
         forward = true;
         continue;
@@ skipping 461 matching lines @@
   if (!lower_bound) {
     cursor_options->low_key =
         ObjectStoreDataKey::Encode(database_id, object_store_id, MinIDBKey());
     cursor_options->low_open = true;  // Not included.
   } else {
     cursor_options->low_key =
         ObjectStoreDataKey::Encode(database_id, object_store_id, range.lower());
     cursor_options->low_open = range.lowerOpen();
   }
 
+  leveldb::Status s;
+
   if (!upper_bound) {
     cursor_options->high_key =
         ObjectStoreDataKey::Encode(database_id, object_store_id, MaxIDBKey());
 
     if (cursor_options->forward) {
       cursor_options->high_open = true;  // Not included.
     } else {
       // We need a key that exists.
+      // TODO(cmumford): Handle this error
       if (!FindGreatestKeyLessThanOrEqual(transaction,
                                           cursor_options->high_key,
-                                          &cursor_options->high_key))
+                                          &cursor_options->high_key,
+                                          s))
         return false;
       cursor_options->high_open = false;
     }
   } else {
     cursor_options->high_key =
         ObjectStoreDataKey::Encode(database_id, object_store_id, range.upper());
     cursor_options->high_open = range.upperOpen();
 
     if (!cursor_options->forward) {
       // For reverse cursors, we need a key that exists.
       std::string found_high_key;
       if (!FindGreatestKeyLessThanOrEqual(
-               transaction, cursor_options->high_key, &found_high_key))
+              transaction, cursor_options->high_key, &found_high_key, s))

[jsbell, 2014/04/14 20:44:20]

Add TODO here as well?

[cmumford, 2014/04/14 23:39:23]

Done.

         return false;
 
       // If the target key should not be included, but we end up with a smaller
       // key, we should include that.
       if (cursor_options->high_open &&
           CompareIndexKeys(found_high_key, cursor_options->high_key) < 0)
         cursor_options->high_open = false;
 
       cursor_options->high_key = found_high_key;
     }
@@ skipping 29 matching lines @@
   if (!lower_bound) {
     cursor_options->low_key =
         IndexDataKey::EncodeMinKey(database_id, object_store_id, index_id);
     cursor_options->low_open = false;  // Included.
   } else {
     cursor_options->low_key = IndexDataKey::Encode(
         database_id, object_store_id, index_id, range.lower());
     cursor_options->low_open = range.lowerOpen();
   }
 
+  // TODO(cmumford): Handle this error

[jsbell, 2014/04/14 20:44:20]

Nit: be consistent in placing the TODOs - near the variable or near the call
that uses it.

[cmumford, 2014/04/14 23:39:23]

Done.

+  leveldb::Status s;
+
   if (!upper_bound) {
     cursor_options->high_key =
         IndexDataKey::EncodeMaxKey(database_id, object_store_id, index_id);
     cursor_options->high_open = false;  // Included.
 
     if (!cursor_options->forward) {  // We need a key that exists.
       if (!FindGreatestKeyLessThanOrEqual(transaction,
                                           cursor_options->high_key,
-                                          &cursor_options->high_key))
+                                          &cursor_options->high_key,
+                                          s))
         return false;
       cursor_options->high_open = false;
     }
   } else {
     cursor_options->high_key = IndexDataKey::Encode(
         database_id, object_store_id, index_id, range.upper());
     cursor_options->high_open = range.upperOpen();
 
     std::string found_high_key;
     // Seek to the *last* key in the set of non-unique keys
     if (!FindGreatestKeyLessThanOrEqual(
-             transaction, cursor_options->high_key, &found_high_key))
+            transaction, cursor_options->high_key, &found_high_key, s))
       return false;
 
     // If the target key should not be included, but we end up with a smaller
     // key, we should include that.
     if (cursor_options->high_open &&
         CompareIndexKeys(found_high_key, cursor_options->high_key) < 0)
       cursor_options->high_open = false;
 
     cursor_options->high_key = found_high_key;
   }
 
   return true;
 }
 
 scoped_ptr<IndexedDBBackingStore::Cursor>
 IndexedDBBackingStore::OpenObjectStoreCursor(
     IndexedDBBackingStore::Transaction* transaction,
     int64 database_id,
     int64 object_store_id,
     const IndexedDBKeyRange& range,
-    indexed_db::CursorDirection direction) {
+    indexed_db::CursorDirection direction,
+    leveldb::Status& s) {
   IDB_TRACE("IndexedDBBackingStore::OpenObjectStoreCursor");
+  s = leveldb::Status::OK();
   LevelDBTransaction* leveldb_transaction = transaction->transaction();
   IndexedDBBackingStore::Cursor::CursorOptions cursor_options;
   if (!ObjectStoreCursorOptions(leveldb_transaction,
                                 database_id,
                                 object_store_id,
                                 range,
                                 direction,
                                 &cursor_options))
     return scoped_ptr<IndexedDBBackingStore::Cursor>();
   scoped_ptr<ObjectStoreCursorImpl> cursor(
       new ObjectStoreCursorImpl(leveldb_transaction, cursor_options));
-  if (!cursor->FirstSeek())
+  if (!cursor->FirstSeek(s))
     return scoped_ptr<IndexedDBBackingStore::Cursor>();
 
   return cursor.PassAs<IndexedDBBackingStore::Cursor>();
 }
 
 scoped_ptr<IndexedDBBackingStore::Cursor>
 IndexedDBBackingStore::OpenObjectStoreKeyCursor(
     IndexedDBBackingStore::Transaction* transaction,
     int64 database_id,
     int64 object_store_id,
     const IndexedDBKeyRange& range,
-    indexed_db::CursorDirection direction) {
+    indexed_db::CursorDirection direction,
+    leveldb::Status& s) {
   IDB_TRACE("IndexedDBBackingStore::OpenObjectStoreKeyCursor");
+  s = leveldb::Status::OK();
   LevelDBTransaction* leveldb_transaction = transaction->transaction();
   IndexedDBBackingStore::Cursor::CursorOptions cursor_options;
   if (!ObjectStoreCursorOptions(leveldb_transaction,
                                 database_id,
                                 object_store_id,
                                 range,
                                 direction,
                                 &cursor_options))
     return scoped_ptr<IndexedDBBackingStore::Cursor>();
   scoped_ptr<ObjectStoreKeyCursorImpl> cursor(
       new ObjectStoreKeyCursorImpl(leveldb_transaction, cursor_options));
-  if (!cursor->FirstSeek())
+  if (!cursor->FirstSeek(s))
     return scoped_ptr<IndexedDBBackingStore::Cursor>();
 
   return cursor.PassAs<IndexedDBBackingStore::Cursor>();
 }
 
 scoped_ptr<IndexedDBBackingStore::Cursor>
 IndexedDBBackingStore::OpenIndexKeyCursor(
     IndexedDBBackingStore::Transaction* transaction,
     int64 database_id,
     int64 object_store_id,
     int64 index_id,
     const IndexedDBKeyRange& range,
-    indexed_db::CursorDirection direction) {
+    indexed_db::CursorDirection direction,
+    leveldb::Status& s) {
   IDB_TRACE("IndexedDBBackingStore::OpenIndexKeyCursor");
+  s = leveldb::Status::OK();
   LevelDBTransaction* leveldb_transaction = transaction->transaction();
   IndexedDBBackingStore::Cursor::CursorOptions cursor_options;
   if (!IndexCursorOptions(leveldb_transaction,
                           database_id,
                           object_store_id,
                           index_id,
                           range,
                           direction,
                           &cursor_options))
     return scoped_ptr<IndexedDBBackingStore::Cursor>();
   scoped_ptr<IndexKeyCursorImpl> cursor(
       new IndexKeyCursorImpl(leveldb_transaction, cursor_options));
-  if (!cursor->FirstSeek())
+  if (!cursor->FirstSeek(s))
     return scoped_ptr<IndexedDBBackingStore::Cursor>();
 
   return cursor.PassAs<IndexedDBBackingStore::Cursor>();
 }
 
 scoped_ptr<IndexedDBBackingStore::Cursor>
 IndexedDBBackingStore::OpenIndexCursor(
     IndexedDBBackingStore::Transaction* transaction,
     int64 database_id,
     int64 object_store_id,
     int64 index_id,
     const IndexedDBKeyRange& range,
-    indexed_db::CursorDirection direction) {
+    indexed_db::CursorDirection direction,
+    leveldb::Status& s) {
   IDB_TRACE("IndexedDBBackingStore::OpenIndexCursor");
   LevelDBTransaction* leveldb_transaction = transaction->transaction();
   IndexedDBBackingStore::Cursor::CursorOptions cursor_options;
   if (!IndexCursorOptions(leveldb_transaction,
                           database_id,
                           object_store_id,
                           index_id,
                           range,
                           direction,
                           &cursor_options))
     return scoped_ptr<IndexedDBBackingStore::Cursor>();
   scoped_ptr<IndexCursorImpl> cursor(
       new IndexCursorImpl(leveldb_transaction, cursor_options));
-  if (!cursor->FirstSeek())
+  if (!cursor->FirstSeek(s))
     return scoped_ptr<IndexedDBBackingStore::Cursor>();
 
   return cursor.PassAs<IndexedDBBackingStore::Cursor>();
 }
 
 IndexedDBBackingStore::Transaction::Transaction(
     IndexedDBBackingStore* backing_store)
     : backing_store_(backing_store) {}
 
 IndexedDBBackingStore::Transaction::~Transaction() {}
@@ skipping 15 matching lines @@
 }
 
 void IndexedDBBackingStore::Transaction::Rollback() {
   IDB_TRACE("IndexedDBBackingStore::Transaction::Rollback");
   DCHECK(transaction_.get());
   transaction_->Rollback();
   transaction_ = NULL;
 }
 
 }  // namespace content