IndexedDB: Abort transactions because of leveldb errors part 4
Created attachment 177399 [details] Patch
Created attachment 177593 [details] Patch
Created attachment 177648 [details] Patch
Created attachment 177652 [details] Patch
Created attachment 177863 [details] Patch
Created attachment 177871 [details] Patch
Josh or Alec, could one of you take a look?
Comment on attachment 177871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177871&action=review Overall lgtm... > Source/WebKit/chromium/ChangeLog:-360 > -2012-11-30 David Grogan <dgrogan@chromium.org> Automatic merge glitch? > Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:125 > +WARN_UNUSED_RETURN static bool getVarInt(DBOrTransaction* db, const LevelDBSlice& key, int64_t& foundInt, bool& found) Just curious - is this placement required for implementations? In forward/class declarations I've seen it after the (), although it looks like you used it as a prefix in IDBObjectStoreBackendImpl.cpp. > Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:373 > + found = getInt(m_db.get(), key, metadata->id); getInt() will get the found-vs.-error treatment in a later patch? > Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:382 > + ASSERT(found); Would !found would indicate a coding consistency error, correct? That is, LevelDB didn't report an error but the metadata was malformed? Should that be an InternalError as well, just a different type? > Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:389 > + ASSERT(found); Ditto. (This could e.g. indicate a problem with the schema migration code.) > Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:641 > + bool ok = getString(levelDBTransaction, ObjectStoreMetaDataKey::encode(databaseId, objectStoreId, ObjectStoreMetaDataKey::Name), objectStoreName, found); Add an ASSERT(found) and/or InternalError if !found?
Comment on attachment 177871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177871&action=review > Source/WebCore/Modules/indexeddb/IDBBackingStore.h:55 > + virtual bool getIDBDatabaseMetaData(const String& name, IDBDatabaseMetadata*, bool& success) WARN_UNUSED_RETURN; Oops, also meant to say: Probably too late now, but I wonder if "typedef bool BackingStoreSuccess;" would improve readability for these headers.
Comment on attachment 177871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177871&action=review >> Source/WebKit/chromium/ChangeLog:-360 >> -2012-11-30 David Grogan <dgrogan@chromium.org> > > Automatic merge glitch? There was the other day, I'm retroactively correcting it here. *shrug* >> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:125 >> +WARN_UNUSED_RETURN static bool getVarInt(DBOrTransaction* db, const LevelDBSlice& key, int64_t& foundInt, bool& found) > > Just curious - is this placement required for implementations? In forward/class declarations I've seen it after the (), although it looks like you used it as a prefix in IDBObjectStoreBackendImpl.cpp. In class method declarations it definitely goes after the (). In non-static standalone functions it also goes after the (). For some reason in static functions it goes in the beginning. Take this with a grain of salt though, it was obtained through trial and error. >> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:373 >> + found = getInt(m_db.get(), key, metadata->id); > > getInt() will get the found-vs.-error treatment in a later patch? Yep. It's the last place that drops errors that I know of. Once its callers are updated, this work is done. >> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:382 >> + ASSERT(found); > > Would !found would indicate a coding consistency error, correct? That is, LevelDB didn't report an error but the metadata was malformed? Should that be an InternalError as well, just a different type? For getString, I think it can only mean that the key wasn't found. For getVarInt you're right, it could indicate an encoding error as well. I had wanted the return value to indicate that leveldb reported an error, so !found shouldn't cause a |return false|. But after thinking about it, a different InternalError along with a return false would probably be best. We don't want release mode to proceed in the (ok && !found) case, we want an abort. >> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:389 >> + ASSERT(found); > > Ditto. (This could e.g. indicate a problem with the schema migration code.) Good point. Yeah, will change back. >> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:641 >> + bool ok = getString(levelDBTransaction, ObjectStoreMetaDataKey::encode(databaseId, objectStoreId, ObjectStoreMetaDataKey::Name), objectStoreName, found); > > Add an ASSERT(found) and/or InternalError if !found? Oops. Will add. >> Source/WebCore/Modules/indexeddb/IDBBackingStore.h:55 >> + virtual bool getIDBDatabaseMetaData(const String& name, IDBDatabaseMetadata*, bool& success) WARN_UNUSED_RETURN; > > Oops, also meant to say: > > Probably too late now, but I wonder if "typedef bool BackingStoreSuccess;" would improve readability for these headers. I like that idea. Seems like a patch that could be easily slipped whenever (so, not too late).
Created attachment 178055 [details] Patch
Now with more aborting per Josh's suggestions.
Tony, could you review this?
Comment on attachment 178055 [details] Patch Clearing flags on attachment: 178055 Committed r136897: <http://trac.webkit.org/changeset/136897>
All reviewed patches have been landed. Closing bug.