Bug 103580 – IndexedDB: Propagate more leveldb errors to script

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[David Grogan]

IndexedDB: Propagate more leveldb errors to script

[David Grogan]

Created attachment 176613 [details]
Patch

[WebKit Review Bot]

Comment on attachment 176613 [details]
Patch

Attachment 176613 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15016617

[David Grogan]

Created attachment 176621 [details]
Patch

[David Grogan]

Josh or Alec, could you take a look at this? I fear that one of you had considered this approach and rejected it for a reason not yet apparent to me.

Do either of you know of any other source of leveldb errors that we don't propagate?

I haven't thought hard about a test yet but I'm hoping a webkit unit test will be easy and a browser test won't be necessary.

[Joshua Bell]

Comment on attachment 176621 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176621&action=review

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:726
> +        LOG_ERROR("Leveldb had problems looking up keyGeneratorCurrentNumberKey");

Can this use InternalError(IDBLevelDBBackingStoreReadError) ?

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:770
> +            LOG_ERROR("Disk error in getKeyGeneratorCurrentNumber");

InternalError(...)?

> Source/WebCore/Modules/indexeddb/IDBBackingStore.h:84
> +    virtual bool getKeyGeneratorCurrentNumber(IDBBackingStore::Transaction*, int64_t databaseId, int64_t objectStoreId, int64_t& currentNumber);

We should mark this with WARN_UNUSED_RETURN (or add a FIXME to do so), here and any other places where we're returning success/failure.

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:235
> +        LOG_ERROR("keyExistsInObjectStore ran in to some corruption");

Should the backingStore take care of logging/histograms/etc, and leave the consequences (i.e. abort, etc) to the caller?

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:236
> +        transaction->abort();

Can we pass a reason (IDBDatabaseError) to the abort? Or is the default good enough?

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:338
> +            callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::UNKNOWN_ERR, "Disk Error"));

I'd be careful about calling it a disk error; don't want to blame the hardware if it's a software issue!

[Joshua Bell]

(In reply to comment #4)
> Josh or Alec, could you take a look at this? I fear that one of you had considered this approach and rejected it for a reason not yet apparent to me.

I think this is the right direction. Reading over old email threads, I think the big picture is:

* DEFINITELY: Propagate I/O errors up from all read/write points; never drop them on the floor
* DEFINITELY: Abort the transaction when an I/O error occurs
* MAYBE: Consider closing the connection when an I/O error occurs.
* MAYBE: Consider closing all other connections using the same backing store (i.e. in the same origin) when an I/O error occurs

This patch is part of the first two bullets - seems the best place to start.

> Do either of you know of any other source of leveldb errors that we don't propagate?

From old email, just a note saying "the error from LevelDBTransaction::get() is not propagated up through IDBLevelDBBackingStore::getObjectStoreRecord(). Right now, dummy data is returned with no indication this is the case." and "there are more instances of the above, where errors are not propagated through - e.g. getNewObjectStoreId - we need to catch all of these" - but I didn't have a list, just noticed a few skimming the code.

[David Grogan]

Comment on attachment 176621 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176621&action=review

Thanks for taking a look. This approach passing your smoke test enough for you to comment gives me more confidence to proceed.

>> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:726
>> +        LOG_ERROR("Leveldb had problems looking up keyGeneratorCurrentNumberKey");
> 
> Can this use InternalError(IDBLevelDBBackingStoreReadError) ?

Yes, it should.

>> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:770
>> +            LOG_ERROR("Disk error in getKeyGeneratorCurrentNumber");
> 
> InternalError(...)?

Will do.

>> Source/WebCore/Modules/indexeddb/IDBBackingStore.h:84
>> +    virtual bool getKeyGeneratorCurrentNumber(IDBBackingStore::Transaction*, int64_t databaseId, int64_t objectStoreId, int64_t& currentNumber);
> 
> We should mark this with WARN_UNUSED_RETURN (or add a FIXME to do so), here and any other places where we're returning success/failure.

Agreed.

>> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:235
>> +        LOG_ERROR("keyExistsInObjectStore ran in to some corruption");
> 
> Should the backingStore take care of logging/histograms/etc, and leave the consequences (i.e. abort, etc) to the caller?

I was thinking something similar even though this patch doesn't yet follow the rules:
BackingStore MUST emit to a histogram.
Caller MAY emit to a histogram (though I'm not sure if any do yet).
Caller MUST surface the error to script.
Liberal logging.

>> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:236
>> +        transaction->abort();
> 
> Can we pass a reason (IDBDatabaseError) to the abort? Or is the default good enough?

A reason would be better, I don't know what the default is.  Will look into.

>> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:338
>> +            callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::UNKNOWN_ERR, "Disk Error"));
> 
> I'd be careful about calling it a disk error; don't want to blame the hardware if it's a software issue!

True. I will come up with something different. I didn't want to just use "Internal Error" everywhere, I want to offer better breadcrumbs for us to follow.

[Alec Flett]

A little late to the party but this looks great..

[David Grogan]

Created attachment 176802 [details]
Patch

[David Grogan]

Josh,

This is a cleaned up revision with the comments addressed. Could you take another look before I send it to Tony? I wish interdiff worked :(

What do you think of increasing the number of entries in the IDBLevelDBBackingStoreInternalErrorType enum?  There are ~25 uses of IDBLevelDBBackingStoreReadError in IDBBackingStore. If a vast majority of our histogram entries come from one method, that'd be good to know.

[Alec Flett]

(In reply to comment #10)
> Josh,
> 
> This is a cleaned up revision with the comments addressed. Could you take another look before I send it to Tony? I wish interdiff worked :(
> 
> What do you think of increasing the number of entries in the IDBLevelDBBackingStoreInternalErrorType enum?  There are ~25 uses of IDBLevelDBBackingStoreReadError in IDBBackingStore. If a vast majority of our histogram entries come from one method, that'd be good to know.

+1

[Joshua Bell]

Comment on attachment 176802 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176802&action=review

+1 to adding enum values, especially if it will distinguish logic/consistency errors (e.g. we forgot to add an entry while migrating, etc) from more spectacular cases.

lgtm, just wordsmithing

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:237
> +        LOG_ERROR("keyExistsInObjectStore ran in to some corruption");

Maybe just "keyExistsInObjectStore reported an error" ?

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:238
> +        transaction->abort(IDBDatabaseError::create(IDBDatabaseException::UNKNOWN_ERR, "Internal Error setting index keys."));

s/Error/error/

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:314
> +            callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::UNKNOWN_ERR, "Internal Error checking key existence."));

s/Error/error/

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:342
> +            callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::UNKNOWN_ERR, "Internal Error updating key generator."));

s/Error/error/

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:551
> +        LOG_ERROR("Disk error while doing getKeyGeneratorCurrentNumber");

Looking around WebCore, a more common phrasing would be something like:
"Failed to getKeyGeneratorCurrentNumber"
"getKeyGeneratorCurrentNumber failed"
"getKeyGeneratorCurrentNumber reported an error"

[David Grogan]

Created attachment 176833 [details]
Patch

[David Grogan]

Phrasing fixed.

Tony, could you review this?

[kov's GTK+ EWS bot]

Comment on attachment 176833 [details]
Patch

Attachment 176833 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15036684

[Tony Chang]

Comment on attachment 176833 [details]
Patch

I'm not a huge fan of COM style success/failure returned from every function, but maybe that's unavoidable if you want to track errors.

[David Grogan]

Agreed that it is lame. This was the first time in a while that I've longed to use exceptions. Do you know of other alternative patterns?

[WebKit Review Bot]

Comment on attachment 176833 [details]
Patch

Clearing flags on attachment: 176833

Committed r136194: <http://trac.webkit.org/changeset/136194>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.