Bug 103964 – IndexedDB: Abort transactions because of leveldb errors part 4

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

[David Grogan]

IndexedDB: Abort transactions because of leveldb errors part 4

[David Grogan]

Created attachment 177399 [details]
Patch

[David Grogan]

Created attachment 177593 [details]
Patch

[David Grogan]

Created attachment 177648 [details]
Patch

[David Grogan]

Created attachment 177652 [details]
Patch

[David Grogan]

Created attachment 177863 [details]
Patch

[David Grogan]

Created attachment 177871 [details]
Patch

[David Grogan]

Josh or Alec, could one of you take a look?

[Joshua Bell]

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  <[e-mail]>

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?

[Joshua Bell]

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.

[David Grogan]

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  <[e-mail]>
> 
> 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).

[David Grogan]

Created attachment 178055 [details]
Patch

[David Grogan]

Now with more aborting per Josh's suggestions.

[David Grogan]

Tony, could you review this?

[WebKit Review Bot]

Comment on attachment 178055 [details]
Patch

Clearing flags on attachment: 178055

Committed r136897: <http://trac.webkit.org/changeset/136897>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.