Bug 70065 – IndexedDB: Throw DATA_ERR on invalid keys, remove null key support

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

[Joshua Bell]

IndexedDB: Throw DATA_ERR on invalid keys, remove null key support

[Joshua Bell]

Created attachment 110917 [details]
Patch

[Joshua Bell]

Note: this requires 3 commits:
(1) WebKit Part 1 (this patch) - renames IDBKey::NullKey to IDBKey::InvalidKey and has the validation logic in the methods that take keys
(2) Chromium patch which drops dependency on WebIDBKey::NullType, treating it the same as the previously existing WebIDBKey::InvalidType
(3) WebKit Part 2 - removes WebIDBKey::NullType

[Joshua Bell]

Any feedback on how to handle the optional parameters in IDBKeyRange.h in a less grotesque way would be appreciated.

[Joshua Bell]

The Chromium side is at: http://codereview.chromium.org/8276023

[WebKit Review Bot]

Comment on attachment 110917 [details]
Patch

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

[Hans Wennborg]

Comment on attachment 110917 [details]
Patch

LGTM with nits; thanks for fixing.

View in context: https://bugs.webkit.org/attachment.cgi?id=110917&action=review
> Source/WebCore/storage/IDBKeyRange.cpp:44
> +PassRefPtr<IDBKeyRange> IDBKeyRange::only(PassRefPtr<IDBKey> prpValue, ExceptionCode& ec)

maybe rename prpValue to prpKey while you're at it?

> Source/WebCore/storage/IDBKeyRange.h:51
> +    static PassRefPtr<IDBKeyRange> only(PassRefPtr<IDBKey> value, ExceptionCode&);

i think you need to forward-declare ExceptionCode

> Source/WebCore/storage/IDBLevelDBCoding.cpp:355
> +        return Vector<char>();

being paranoid, if we ever end up in this code path, in a Release built (so we don't hit the assert), it might be bad if we end up writing an empty string for a key, since we could have problems parsing that later. So definitely keep the assert, but maybe return encodeByte(kIDBKeyNullTypeByte)?

> Source/WebCore/storage/IDBSQLiteBackingStore.cpp:345
> +    case IDBKey::InvalidType:

I believe this file is now actually gone :)

> Source/WebKit/chromium/src/AssertMatchingEnums.cpp:395
> +COMPILE_ASSERT_MATCHING_ENUM(WebIDBKey::NullType, IDBKey::InvalidType);

not that it makes much difference, but a FIXME that this is temporary would be nice.

> Source/WebKit/chromium/src/WebIDBKey.cpp:98
> +    m_private = 0;

why not IDBKey::createInvalid() ?

[Joshua Bell]

Comment on attachment 110917 [details]
Patch

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

Thanks, all the nits are good.

>> Source/WebKit/chromium/src/WebIDBKey.cpp:98
>> +    m_private = 0;
> 
> why not IDBKey::createInvalid() ?

WebIDBKey already has a notion of invalid which it stores with a null pointer. Since we're collapsing the the Null and Invalid types together, I made Null act like Invalid rather than vice versa. I could go the other way and have invalid WebIDBKeys store an Invalid IDBKey; I don't have a strong preference.

[Joshua Bell]

Created attachment 111087 [details]
Patch

[Hans Wennborg]

(In reply to comment #7)
> >> Source/WebKit/chromium/src/WebIDBKey.cpp:98
> >> +    m_private = 0;
> > 
> > why not IDBKey::createInvalid() ?
> 
> WebIDBKey already has a notion of invalid which it stores with a null pointer. Since we're collapsing the the Null and Invalid types together, I made Null act like Invalid rather than vice versa. I could go the other way and have invalid WebIDBKeys store an Invalid IDBKey; I don't have a strong preference.

Ok, that makes sense.

LGTM

[Joshua Bell]

Tony, can you give this a look-see?

[Tony Chang]

Comment on attachment 111087 [details]
Patch

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

> Source/WebCore/storage/IDBKeyRange.h:56
> +      return lowerBound(bound, false, ec);

Nit: WebKit style prefers using enums rather than bools for arguments.  It makes it more clear at the caller what the second param is.  Since the code was already using bools, this is just a suggestion for a future cleanup.  This would only apply to the private methods, not the idl methods.

[Joshua Bell]

Comment on attachment 111087 [details]
Patch

Filed https://bugs.webkit.org/show_bug.cgi?id=70743 as a TODO for the bool->enum nit

[WebKit Review Bot]

Comment on attachment 111087 [details]
Patch

Clearing flags on attachment: 111087

Committed r98278: <http://trac.webkit.org/changeset/98278>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.