Make iterating over a corrupted IndexedDB fail.

Make iterating over a corrupted IndexedDB fail.

If an open corrupted IndexedDB was iterated over it would silently fail and the
web page would never be informed. This change fixes that bug so that cursor
iteration fails just like a get operation did previously.

BUG=332524
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264256

[cmumford, 2014-04-14 16:58:39]

PTAL.

I left six TODO(cmumford) comments in the code to handle additional errors. I
started to work on these, but doing so  complicated this already large patch. I
will create a new bug for those checks and address in a subsequent commit.

[jsbell, 2014-04-14 18:35:39]

Since this comment affects nearly every diff in the CL, sending this out now and
then I'll do a "logical" pass through the change.

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
File content/browser/indexed_db/indexed_db_backing_store.h (right):

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.h:100: virtual
std::vector<base::string16> GetDatabaseNames(leveldb::Status&);
Chromium style is to use pointers, not non-const references.

[cmumford, 2014-04-14 20:25:56]

Fixed out param types.

[jsbell, 2014-04-14 20:44:19]

Despite the volume of comments, overall this is looking good. :)

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.cc:349: if
(db_schema_version < 2) {
Not strictly necessary, but I'd add a `s->ok() &&` clause here.

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.cc:1070: s = it->Next();
Check value of s here? (see next comment...)

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.cc:1087: continue;
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.

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.cc:1349: if (!s.ok()) {
Nit: make the blank lines before the if()s consistent

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.cc:1694: s = it->Next();
Same as GetObjectStores here - `if(!s.ok()) break;` ?

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.cc:1710: continue;
Same as GetObjectStores here - `continue` or `break` ?

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.cc:2674: transaction,
cursor_options->high_key, &found_high_key, s))
Add TODO here as well?

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.cc:2724: // TODO(cmumford):
Handle this error
Nit: be consistent in placing the TODOs - near the variable or near the call
that uses it.

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
File content/browser/indexed_db/indexed_db_cursor.cc (right):

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_cursor.cc:70: if (!s.ok()) {
Won't Advance() return false if !s.ok() ?

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_cursor.cc:72: if (s.IsCorruption()) {
Remove this placeholder code, make the TODO more verbose (if necessary)

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_cursor.cc:74: return;
Wouldn't this mean that the callbacks (i.e. request) is never resolved (via
Success or Error)? Shouldn't the script be told something?

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_cursor.cc:90: s)) {
Does this need a corresponding check for s.ok() ?

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_cursor.cc:163: if (!s.ok()) {
As above.

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_cursor.cc:184: leveldb::Status s;
Initialized to the default here (i.e. OK), but elsewhere a Status is initialized
explicitly to Status::OK() - be consistent?

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_cursor.cc:193: DCHECK(ok);
Is this DCHECK is invalid in the face of corruption?

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_database.cc:100: return s.ok() ? database
: 0;
Does this compile everywhere? The trinary operator is a bit picky if the
arguments aren't obviously the same type.

Also, can you make it NULL? (0 is webkit-ism, probably a result of copy/pasting
this code)

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_database.cc:1101: // Why is Success being
called?
In script, if a cursor is opened and there's nothing in range, it's like it hit
the end of its range - i.e. success w/ NULL.

(I'm assuming that logic is applying here as well.)

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_database.cc:1183: // Should probably check
for corruption here too?
Make this a TODO

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:877: if (s.ok())
This test seems backwards?

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:878: DLOG(ERROR) <<
"Unable to reset prefetch";
Should there be a TODO to do more here?

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
File content/browser/indexed_db/indexed_db_factory.cc (right):

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_factory.cc:196: if (s.ok()) {
I'd reduce this to just an !s.ok() case for now, since OnSuccess is called
either way.

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_factory.cc:252: backing_store = NULL;  //
Closes the LevelDB so that it can be deleted
Does it actually close, or just unblock closing?

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/l...
File content/browser/indexed_db/leveldb/leveldb_transaction.cc (right):

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/l...
content/browser/indexed_db/leveldb/leveldb_transaction.cc:210: leveldb::Status s
= data_iterator_->SeekToLast();
DCHECK(s.ok()) since data_iterator_ should never fail?

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/l...
content/browser/indexed_db/leveldb/leveldb_transaction.cc:286: s =
non_current->Seek(Key());
Does s need to be tested here?

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/l...
content/browser/indexed_db/leveldb/leveldb_transaction.cc:303: if (s.ok())
This repeated testing of s.ok() is weird. Can we just early exit?

https://codereview.chromium.org/237143006/diff/1/content/test/data/indexeddb/...
File content/test/data/indexeddb/corrupted_open_db_detection.html (right):

https://codereview.chromium.org/237143006/diff/1/content/test/data/indexeddb/...
content/test/data/indexeddb/corrupted_open_db_detection.html:131: else {
nit: Put on same line as }

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

Most issues addressed, a few questions from me.

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.cc:349: if
(db_schema_version < 2) {
On 2014/04/14 20:44:20, jsbell wrote:
> Not strictly necessary, but I'd add a `s->ok() &&` clause here.

Done.

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.cc:1087: continue;
On 2014/04/14 20:44:20, jsbell wrote:
> 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.

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 ;-)

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.cc:1694: s = it->Next();
On 2014/04/14 20:44:20, jsbell wrote:
> Same as GetObjectStores here - `if(!s.ok()) break;` ?

Done.

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.cc:1710: continue;
On 2014/04/14 20:44:20, jsbell wrote:
> Same as GetObjectStores here - `continue` or `break` ?

Done.

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.cc:2674: transaction,
cursor_options->high_key, &found_high_key, s))
On 2014/04/14 20:44:20, jsbell wrote:
> Add TODO here as well?

Done.

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.cc:2724: // TODO(cmumford):
Handle this error
On 2014/04/14 20:44:20, jsbell wrote:
> Nit: be consistent in placing the TODOs - near the variable or near the call
> that uses it.

Done.

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
File content/browser/indexed_db/indexed_db_cursor.cc (right):

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_cursor.cc:70: if (!s.ok()) {
On 2014/04/14 20:44:20, jsbell wrote:
> Won't Advance() return false if !s.ok() ?

Done.

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_cursor.cc:72: if (s.IsCorruption()) {
On 2014/04/14 20:44:20, jsbell wrote:
> Remove this placeholder code, make the TODO more verbose (if necessary)

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

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_cursor.cc:74: return;
On 2014/04/14 20:44:20, jsbell wrote:
> Wouldn't this mean that the callbacks (i.e. request) is never resolved (via
> Success or Error)? Shouldn't the script be told something?

Done.

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_cursor.cc:90: s)) {
On 2014/04/14 20:44:20, jsbell wrote:
> Does this need a corresponding check for s.ok() ?

Done. Also added TODO for handling of corrupted db.

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_cursor.cc:163: if (!s.ok()) {
On 2014/04/14 20:44:20, jsbell wrote:
> As above.

Done.

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_cursor.cc:184: leveldb::Status s;
On 2014/04/14 20:44:20, jsbell wrote:
> Initialized to the default here (i.e. OK), but elsewhere a Status is
initialized
> explicitly to Status::OK() - be consistent?

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.

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_cursor.cc:193: DCHECK(ok);
On 2014/04/14 20:44:20, jsbell wrote:
> Is this DCHECK is invalid in the face of corruption?

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.

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_database.cc:100: return s.ok() ? database
: 0;
On 2014/04/14 20:44:20, jsbell wrote:
> Does this compile everywhere? The trinary operator is a bit picky if the
> arguments aren't obviously the same type.
> 
> Also, can you make it NULL? (0 is webkit-ism, probably a result of
copy/pasting
> this code)

I didn't compile it everywhere. I'll be safe and go back to the prior form, but
with NULL.

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_database.cc:1101: // Why is Success being
called?
On 2014/04/14 20:44:20, jsbell wrote:
> In script, if a cursor is opened and there's nothing in range, it's like it
hit
> the end of its range - i.e. success w/ NULL.
> 
> (I'm assuming that logic is applying here as well.)

Are you saying that if the db is corrupt you want Success(NULL) to be called?
Like above at line 1095?

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_database.cc:1183: // Should probably check
for corruption here too?
On 2014/04/14 20:44:20, jsbell wrote:
> Make this a TODO

Done.

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:877: if (s.ok())
On 2014/04/14 20:44:20, jsbell wrote:
> This test seems backwards?

You are correct - thx!

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:878: DLOG(ERROR) <<
"Unable to reset prefetch";
On 2014/04/14 20:44:20, jsbell wrote:
> Should there be a TODO to do more here?

Done.

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
File content/browser/indexed_db/indexed_db_factory.cc (right):

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_factory.cc:196: if (s.ok()) {
On 2014/04/14 20:44:20, jsbell wrote:
> I'd reduce this to just an !s.ok() case for now, since OnSuccess is called
> either way.

Done.

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_factory.cc:252: backing_store = NULL;  //
Closes the LevelDB so that it can be deleted
On 2014/04/14 20:44:20, jsbell wrote:
> Does it actually close, or just unblock closing?

If I'm reading that correctly that results in the leveldb::DBImpl instance being
deleted which is the equivalent of closing the LevelDB database.

That being said that NULL ptr assignment isn't strictly related to this bug so I
am going to take it out. I have three other changes related to specific API
failures and that change leaked into this CL from one of the others.

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/l...
File content/browser/indexed_db/leveldb/leveldb_transaction.cc (right):

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/l...
content/browser/indexed_db/leveldb/leveldb_transaction.cc:210: leveldb::Status s
= data_iterator_->SeekToLast();
On 2014/04/14 20:44:20, jsbell wrote:
> DCHECK(s.ok()) since data_iterator_ should never fail?

Yes, DataIterator::SeekToLast() always returns OK(). Fixed.

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/l...
content/browser/indexed_db/leveldb/leveldb_transaction.cc:286: s =
non_current->Seek(Key());
On 2014/04/14 20:44:20, jsbell wrote:
> Does s need to be tested here?

Done.

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/l...
content/browser/indexed_db/leveldb/leveldb_transaction.cc:303: if (s.ok())
On 2014/04/14 20:44:20, jsbell wrote:
> This repeated testing of s.ok() is weird. Can we just early exit?

Done.

https://codereview.chromium.org/237143006/diff/1/content/test/data/indexeddb/...
File content/test/data/indexeddb/corrupted_open_db_detection.html (right):

https://codereview.chromium.org/237143006/diff/1/content/test/data/indexeddb/...
content/test/data/indexeddb/corrupted_open_db_detection.html:131: else {
On 2014/04/14 20:44:20, jsbell wrote:
> nit: Put on same line as }

Done.

[jsbell, 2014-04-15 16:39:56]

lgtm with tiny suggestions.

If the "Why is Success being called?" question is answered, can you remove the
comment? If not, I'm confused!

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/237143006/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_database.cc:1101: // Why is Success being
called?
On 2014/04/14 23:39:23, cmumford wrote:
> On 2014/04/14 20:44:20, jsbell wrote:
> > In script, if a cursor is opened and there's nothing in range, it's like it
> hit
> > the end of its range - i.e. success w/ NULL.
> > 
> > (I'm assuming that logic is applying here as well.)
> 
> Are you saying that if the db is corrupt you want Success(NULL) to be called?
> Like above at line 1095?

I'm just answering the question "Why is Success being called?": if you have keys
in the range 0...10 and open a cursor on range (20,30) then script sees
onsuccess w/ result == null; IndexedDBBackingStore::OpenXXXCursor has the same
semantics, that backing_store_cursor will be NULL in such a case.

https://codereview.chromium.org/237143006/diff/40001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_database_unittest.cc (right):

https://codereview.chromium.org/237143006/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database_unittest.cc:62: &s);
Can you toss in ASSERT(s.ok()) after these calls?

https://codereview.chromium.org/237143006/diff/40001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_transaction_unittest.cc (right):

https://codereview.chromium.org/237143006/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_transaction_unittest.cc:27: &s);
Toss in an ASSERT(s.ok()) ?

https://codereview.chromium.org/237143006/diff/40001/content/browser/indexed_...
File content/browser/indexed_db/leveldb/leveldb_transaction.cc (right):

https://codereview.chromium.org/237143006/diff/40001/content/browser/indexed_...
content/browser/indexed_db/leveldb/leveldb_transaction.cc:262: if (s.ok())
Early exit here if !s.ok() ? (as you did below, I missed this one)

[cmumford, 2014-04-15 17:34:36]

Got the lgtm so will check the commit box at 11am. Leaving a few minutes for a
quick check if anybody wants to make one.

https://codereview.chromium.org/237143006/diff/40001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_database_unittest.cc (right):

https://codereview.chromium.org/237143006/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database_unittest.cc:62: &s);
On 2014/04/15 16:39:57, jsbell wrote:
> Can you toss in ASSERT(s.ok()) after these calls?

Assumed you meant gtest's ASSERT_TRUE - done.

https://codereview.chromium.org/237143006/diff/40001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_transaction_unittest.cc (right):

https://codereview.chromium.org/237143006/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_transaction_unittest.cc:27: &s);
On 2014/04/15 16:39:57, jsbell wrote:
> Toss in an ASSERT(s.ok()) ?

Done, but strangely harder than expected. You can see in patch set #4.

https://codereview.chromium.org/237143006/diff/40001/content/browser/indexed_...
File content/browser/indexed_db/leveldb/leveldb_transaction.cc (right):

https://codereview.chromium.org/237143006/diff/40001/content/browser/indexed_...
content/browser/indexed_db/leveldb/leveldb_transaction.cc:262: if (s.ok())
On 2014/04/15 16:39:57, jsbell wrote:
> Early exit here if !s.ok() ? (as you did below, I missed this one)

Done.

[jsbell, 2014-04-15 17:48:45]

still lgtm

https://codereview.chromium.org/237143006/diff/40002/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_transaction_unittest.cc (right):

https://codereview.chromium.org/237143006/diff/40002/content/browser/indexed_...
content/browser/indexed_db/indexed_db_transaction_unittest.cc:25: // DB is
created here instead of the constructor to workaround a
I think you can shorten the comment a bit, e.g. 'Can't use ASSERT* macros in
constructors'

[cmumford, 2014-04-15 17:55:25]

The CQ bit was checked by cmumford@chromium.org

[commit-bot: I haz the power, 2014-04-15 17:58:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cmumford@chromium.org/237143006/40002

[commit-bot: I haz the power, 2014-04-15 20:35:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-15 20:36:00]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[cmumford, 2014-04-16 15:46:29]

The CQ bit was checked by cmumford@chromium.org

[commit-bot: I haz the power, 2014-04-16 15:47:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cmumford@chromium.org/237143006/40002

[commit-bot: I haz the power, 2014-04-16 18:32:21]

Change committed as 264256