IndexedDB: Propagate more leveldb errors to script
Created attachment 176613 [details] Patch
Comment on attachment 176613 [details] Patch Attachment 176613 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15016617
Created attachment 176621 [details] Patch
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 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!
(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 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.
A little late to the party but this looks great..
Created attachment 176802 [details] Patch
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.
(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 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"
Created attachment 176833 [details] Patch
Phrasing fixed. Tony, could you review this?
Comment on attachment 176833 [details] Patch Attachment 176833 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/15036684
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.
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 on attachment 176833 [details] Patch Clearing flags on attachment: 176833 Committed r136194: <http://trac.webkit.org/changeset/136194>
All reviewed patches have been landed. Closing bug.