Bug 68726 – IndexedDB: Null key path gets stored as empty string key path

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

IndexedDB: Null key path gets stored as empty string key path

[Joshua Bell]

Created attachment 108530 [details]
Patch

[Joshua Bell]

Chromium issue is http://code.google.com/p/chromium/issues/detail?id=90635

Tests to detect regressions across browser sessions will be added on the chromium side.

[WebKit Review Bot]

Comment on attachment 108530 [details]
Patch

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

[Hans Wennborg]

Thanks! It will be very nice to get this landed.

The storage/indexeddb/migrate-basics.html layout test failed when I tried this locally.

[Joshua Bell]

Created attachment 108558 [details]
Patch

[Joshua Bell]

Hooray for tests. Ensure we don't try and decode a stop key. Passes the storage layout tests now

[WebKit Review Bot]

Comment on attachment 108558 [details]
Patch

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

[David Grogan]

From that bot:
Source/WebCore/storage/IDBLevelDBBackingStore.cpp:225: error: unused variable 'p'

[Hans Wennborg]

Comment on attachment 108558 [details]
Patch

Looks good, just a couple of nits.

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

> Source/WebCore/storage/IDBLevelDBBackingStore.cpp:226
> +    ASSERT(p);

you want ASSERT_UNUSED(p, p);

> Source/WebCore/storage/IDBLevelDBBackingStore.cpp:291
> +            hasKeyPath = *it->value().begin();

This works, but I would feel safer if we had specific encode/decode functions for bools. Would you like to add a FIXME about that?

> Source/WebCore/storage/IDBLevelDBBackingStore.cpp:409
> +    if (!deleteRange(m_currentTransaction.get(), ObjectStoreMetaDataKey::encode(databaseId, objectStoreId, 0), ObjectStoreMetaDataKey::encode(databaseId, objectStoreId, INT64_MAX)))

We try to avoid using INT64_MAX in this file by calling various *encodeMaxKey functions instead. You can add a ObjectStoreMetaDataKey::encodeMaxKey(int databaseId, int objectStoreId) function.

[Joshua Bell]

Created attachment 108681 [details]
Patch

[Hans Wennborg]

(In reply to comment #10)
> Created an attachment (id=108681) [details]
> Patch

LGTM

[Joshua Bell]

Comment on attachment 108681 [details]
Patch

Tony, can you please review?

[Tony Chang]

Comment on attachment 108681 [details]
Patch

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

> Source/WebCore/storage/IDBLevelDBBackingStore.cpp:215
> +bool checkObjectStoreAndMetaDataType(const LevelDBIterator* it, const Vector<char>& stopKey, int64_t objectStoreId, int64_t metaDataType)

Nit: Should this function be static?

[Joshua Bell]

Created attachment 108687 [details]
Patch

[Joshua Bell]

Comment on attachment 108687 [details]
Patch

Good call on static function

[WebKit Review Bot]

Comment on attachment 108687 [details]
Patch

Clearing flags on attachment: 108687

Committed r96007: <http://trac.webkit.org/changeset/96007>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.