Bug 103580 - IndexedDB: Propagate more leveldb errors to script
Summary: IndexedDB: Propagate more leveldb errors to script
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Grogan
URL:
Keywords:
Depends on:
Blocks: 103688
  Show dependency treegraph
 
Reported: 2012-11-28 17:13 PST by David Grogan
Modified: 2012-11-29 18:23 PST (History)
7 users (show)

See Also:


Attachments
Patch (22.39 KB, patch)
2012-11-28 17:39 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (24.29 KB, patch)
2012-11-28 18:15 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (24.45 KB, patch)
2012-11-29 13:19 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (24.43 KB, patch)
2012-11-29 15:35 PST, David Grogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Grogan 2012-11-28 17:13:39 PST
IndexedDB: Propagate more leveldb errors to script
Comment 1 David Grogan 2012-11-28 17:39:23 PST
Created attachment 176613 [details]
Patch
Comment 2 WebKit Review Bot 2012-11-28 17:43:36 PST
Comment on attachment 176613 [details]
Patch

Attachment 176613 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15016617
Comment 3 David Grogan 2012-11-28 18:15:14 PST
Created attachment 176621 [details]
Patch
Comment 4 David Grogan 2012-11-28 18:22:43 PST
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.
Comment 5 Joshua Bell 2012-11-29 11:30:42 PST
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!
Comment 6 Joshua Bell 2012-11-29 11:41:28 PST
(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.
Comment 7 David Grogan 2012-11-29 11:47:20 PST
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.
Comment 8 Alec Flett 2012-11-29 12:01:17 PST
A little late to the party but this looks great..
Comment 9 David Grogan 2012-11-29 13:19:33 PST
Created attachment 176802 [details]
Patch
Comment 10 David Grogan 2012-11-29 13:30:42 PST
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.
Comment 11 Alec Flett 2012-11-29 13:33:42 PST
(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
Comment 12 Joshua Bell 2012-11-29 14:25:39 PST
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"
Comment 13 David Grogan 2012-11-29 15:35:08 PST
Created attachment 176833 [details]
Patch
Comment 14 David Grogan 2012-11-29 15:36:12 PST
Phrasing fixed.

Tony, could you review this?
Comment 15 kov's GTK+ EWS bot 2012-11-29 15:43:38 PST
Comment on attachment 176833 [details]
Patch

Attachment 176833 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15036684
Comment 16 Tony Chang 2012-11-29 15:48:18 PST
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.
Comment 17 David Grogan 2012-11-29 16:14:46 PST
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?
Comment 18 WebKit Review Bot 2012-11-29 18:23:09 PST
Comment on attachment 176833 [details]
Patch

Clearing flags on attachment: 176833

Committed r136194: <http://trac.webkit.org/changeset/136194>
Comment 19 WebKit Review Bot 2012-11-29 18:23:14 PST
All reviewed patches have been landed.  Closing bug.