Make iterating over a corrupted IndexedDB fail.

content/browser/indexed_db/indexed_db_cursor.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_cursor.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "content/browser/indexed_db/indexed_db_callbacks.h"
 #include "content/browser/indexed_db/indexed_db_database_error.h"
@@ skipping 42 matching lines @@
       task_type_,
       base::Bind(
           &IndexedDBCursor::CursorAdvanceOperation, this, count, callbacks));
 }
 
 void IndexedDBCursor::CursorAdvanceOperation(
     uint32 count,
     scoped_refptr<IndexedDBCallbacks> callbacks,
     IndexedDBTransaction* /*transaction*/) {
   IDB_TRACE("IndexedDBCursor::CursorAdvanceOperation");
-  if (!cursor_ || !cursor_->Advance(count)) {
+  leveldb::Status s;
+  if (!cursor_ || !cursor_->Advance(count, s)) {
     cursor_.reset();
     callbacks->OnSuccess(static_cast<IndexedDBValue*>(NULL));
     return;
   }
 
+  if (!s.ok()) {

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

Won't Advance() return false if !s.ok() ?

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

Done.

+    // TODO(cmumford): Handle corruption
+    if (s.IsCorruption()) {

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

Remove this placeholder code, make the TODO more verbose (if necessary)

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

Yeah, I shouldn't have left this block in there. I just removed it and improved
the TODO.

+    }
+    return;

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

Wouldn't this mean that the callbacks (i.e. request) is never resolved (via
Success or Error)? Shouldn't the script be told something?

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

Done.

+  }
+
   callbacks->OnSuccess(key(), primary_key(), Value());
 }
 
 void IndexedDBCursor::CursorIterationOperation(
     scoped_ptr<IndexedDBKey> key,
     scoped_ptr<IndexedDBKey> primary_key,
     scoped_refptr<IndexedDBCallbacks> callbacks,
     IndexedDBTransaction* /*transaction*/) {
   IDB_TRACE("IndexedDBCursor::CursorIterationOperation");
-  if (!cursor_ ||
-      !cursor_->Continue(
-           key.get(), primary_key.get(), IndexedDBBackingStore::Cursor::SEEK)) {
+  leveldb::Status s;
+  if (!cursor_ || !cursor_->Continue(key.get(),
+                                     primary_key.get(),
+                                     IndexedDBBackingStore::Cursor::SEEK,
+                                     s)) {

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

Does this need a corresponding check for s.ok() ?

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

Done. Also added TODO for handling of corrupted db.

     cursor_.reset();
     callbacks->OnSuccess(static_cast<IndexedDBValue*>(NULL));
     return;
   }
 
   callbacks->OnSuccess(this->key(), this->primary_key(), Value());
 }
 
 void IndexedDBCursor::PrefetchContinue(
     int number_to_fetch,
@@ skipping 14 matching lines @@
     IndexedDBTransaction* /*transaction*/) {
   IDB_TRACE("IndexedDBCursor::CursorPrefetchIterationOperation");
 
   std::vector<IndexedDBKey> found_keys;
   std::vector<IndexedDBKey> found_primary_keys;
   std::vector<IndexedDBValue> found_values;
 
   saved_cursor_.reset();
   const size_t max_size_estimate = 10 * 1024 * 1024;
   size_t size_estimate = 0;
+  leveldb::Status s;
 
   for (int i = 0; i < number_to_fetch; ++i) {
-    if (!cursor_ || !cursor_->Continue()) {
+    if (!cursor_ || !cursor_->Continue(s)) {
       cursor_.reset();
       break;
     }
 
     if (i == 0) {
       // First prefetched result is always used, so that's the position
       // a cursor should be reset to if the prefetch is invalidated.
       saved_cursor_.reset(cursor_->Clone());
     }
 
@@ skipping 14 matching lines @@
       default:
         NOTREACHED();
     }
     size_estimate += cursor_->key().size_estimate();
     size_estimate += cursor_->primary_key().size_estimate();
 
     if (size_estimate > max_size_estimate)
       break;
   }
 
+  if (!s.ok()) {

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

As above.

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

Done.

+    // TODO(cmumford): Handle corruption
+    if (s.IsCorruption()) {
+    }
+    return;
+  }
+
   if (!found_keys.size()) {
     callbacks->OnSuccess(static_cast<IndexedDBValue*>(NULL));
     return;
   }
 
   callbacks->OnSuccessWithPrefetch(
       found_keys, found_primary_keys, found_values);
 }
 
-void IndexedDBCursor::PrefetchReset(int used_prefetches,
-                                    int /* unused_prefetches */) {
+leveldb::Status IndexedDBCursor::PrefetchReset(int used_prefetches,
+                                               int /* unused_prefetches */) {
   IDB_TRACE("IndexedDBCursor::PrefetchReset");
   cursor_.swap(saved_cursor_);
   saved_cursor_.reset();
+  leveldb::Status s;

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

Initialized to the default here (i.e. OK), but elsewhere a Status is initialized
explicitly to Status::OK() - be consistent?

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

Else where I've been setting to Status::OK() only to initialize the supplied
parameter. When it's the initialization of a local variable I'm letting the
default constructor initialize it to OK.

But I'd be happy to change that - just let me know.

 
   if (closed_)
-    return;
+    return s;
   if (cursor_) {
     // First prefetched result is always used.
     DCHECK_GT(used_prefetches, 0);
     for (int i = 0; i < used_prefetches - 1; ++i) {
-      bool ok = cursor_->Continue();
+      bool ok = cursor_->Continue(s);
       DCHECK(ok);

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

Is this DCHECK is invalid in the face of corruption?

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

Yes, but false is also returned on other errors. I do think it should go, but
didn't take the time to fully track this down and wanted to play it safe with
this change. If you want I can remove it.

     }
   }
+
+  return s;
 }
 
 void IndexedDBCursor::Close() {
   IDB_TRACE("IndexedDBCursor::Close");
   closed_ = true;
   cursor_.reset();
   saved_cursor_.reset();
 }
 
 }  // namespace content