Bug 76487 – IndexedDB: Need to distinguish key paths that don't yield value vs. yield invalid key

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joshua Bell]

Following on from: https://bugs.webkit.org/show_bug.cgi?id=76075

There are some subtleties in the IDB spec:

* The object store uses in-line keys and the result of evaluating the object store's key path yields a value and that value is not a valid key.
* If there are any indexes referencing this object store whose key path is a string, evaluating their key path on the value parameter yields a value, and that value is not a valid key.

To implement these we need to ensure the fetchKeyFromKeyPath() functions distinguish the two cases:
* key path does not yield a value
* key path yields a value which is not a valid key

The obvious way to do this is have the function return NULL vs. an IDBKey (which has InvalidType). Unfortunately, these distinct cases are not represented in the WebKit API for the chromium port - Source/chromium/src/WebIDBKey.cpp collapses the two cases. 

The behavior difference is really only notable for adding/putting object store records where there is an index. When the index's keyPath yields nothing then the index doesn't get an entry for that object store record. When the index's keyPath yields a value but that value is not a valid key the spec indicates the add/put call should raise an exception.

Currently, the storage/indexeddb/objectstore-basics.html test fails on this in the Chromium port:


   store = db.createObjectStore('storeName', null)
   index = store.createIndex('indexName', 'x', {unique: true})
   ...   
   transaction = db.transaction(['storeName'], webkitIDBTransaction.READ_WRITE)
   store = transaction.objectStore('storeName')
   request = store.add({x: null}, 'validkey')

The last line should, per spec, throw DATA_ERR (see https://bugs.webkit.org/show_bug.cgi?id=76075). The current implementation checks later and thus fails the request, sending an Error event - this works in DumpRenderTree. In Chromium the request succeeds.

[Joshua Bell]

Landing this will require disabling some tests and extending some enumeration-sensitive code in the Chromium port first. See http://code.google.com/p/chromium/issues/detail?id=110956 for the "final state" of the Chromium changes.

[Joshua Bell]

(In reply to comment #1)
> Landing this will require disabling some tests and extending some enumeration-sensitive code in the Chromium port first. See http://code.google.com/p/chromium/issues/detail?id=110956 for the "final state" of the Chromium changes.

Er, https://chromiumcodereview.appspot.com/9212038

[Joshua Bell]

Created attachment 123664 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[WebKit Review Bot]

Comment on attachment 123664 [details]
Patch

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

New failing tests:
storage/indexeddb/objectstore-basics.html

[Joshua Bell]

Created attachment 123767 [details]
Patch

[Joshua Bell]

Once everything has passed try-bots and reviews, the plan is:

1. Land https://chromiumcodereview.appspot.com/9117040 in Chromium
2. Land https://bugs.webkit.org/show_bug.cgi?id=76487 (this)
3. Land https://chromiumcodereview.appspot.com/9212038 in Chromium

The Chromium port will have some broken functionality between steps #2 and #3, but the affected build and tests are addressed in #1.

[Joshua Bell]

tony@, fishd@ - can I get an r?

The corresponding Chromium changes have been lgtm'd, and obviously I'll land "part 1" before landing this.

[David Grogan]

Comment on attachment 123767 [details]
Patch

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

LGTM

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:164
> +        for (IndexMap::iterator it = m_indexes.begin(); it != m_indexes.end(); ++it) {

So if an index's keypath yields no value, the index doesn't get an entry, but if the index's keypath yields an invalid value, DATA_ERR is thrown?  Is the check at line 262-264 of this file now redundant or is there a difference between indexKey->valid() and indexKey->type() == IDBKey::InvalidType ?

> LayoutTests/storage/indexeddb/keypath-edges.html:100
> +        debug("Key path doesn't resolve to a value - internally, key should be null, put request should succeed");

I would have thought the key would be autoincremented to 1.  Is the comment wrong or is my understanding wrong?

[Tony Chang]

Comment on attachment 123767 [details]
Patch

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

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:158
> +            } else {
> +                if (keyPathKey && !keyPathKey->valid()) {

Nit: Merge lines into an "else if"?

[Joshua Bell]

(In reply to comment #9)
> (From update of attachment 123767 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123767&action=review
> 
> LGTM
> 
> > Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:164
> > +        for (IndexMap::iterator it = m_indexes.begin(); it != m_indexes.end(); ++it) {
> 
> So if an index's keypath yields no value, the index doesn't get an entry, but if the index's keypath yields an invalid value, DATA_ERR is thrown? 

That's correct. In the spec: 

* For the add/put call(): "If any of the following conditions are true, this method throws a DOMException of type DataError: ... If there are any indexes referencing this object store whose key path is a string, evaluating their key path on the value parameter yields a value, and that value is not a valid key."
* During the async store operation: "If index's key path is a string, then evaluate it on value. If this does not yield a value don't take any further actions for this index."

> Is the check at line 262-264 of this file now redundant or is there a difference between indexKey->valid() and indexKey->type() == IDBKey::InvalidType ?

Those expressions are the same, and the checks in putInternal (and selectKeyForPut) should now indeed be redundant. I'll see if I can safely take them out now (was going to do cleanup on them later once I'd come up for air).

> > LayoutTests/storage/indexeddb/keypath-edges.html:100
> > +        debug("Key path doesn't resolve to a value - internally, key should be null, put request should succeed");
> 
> I would have thought the key would be autoincremented to 1.  Is the comment wrong or is my understanding wrong?

The comment is extremely unclear. I meant the result of evaluating the key path would yield a "null" key; you're right, the actual key being applied would end up being 1. I'll fix the comment.

[Joshua Bell]

FYI, I've filed https://bugs.webkit.org/show_bug.cgi?id=76952 for further cleanup that can be done following some spec discussion on the list, and added a FIXME comment with the URL. New patch coming shortly.

[Joshua Bell]

Created attachment 123833 [details]
Patch

[WebKit Review Bot]

Comment on attachment 123833 [details]
Patch

Rejecting attachment 123833 [details] from commit-queue.

New failing tests:
media/audio-garbage-collect.html
Full output: http://queues.webkit.org/results/11264148

[David Grogan]

Comment on attachment 123833 [details]
Patch

I'm re-marking cq+.  That audio test had to be a flake.

[David Grogan]

Comment on attachment 123833 [details]
Patch

On second look, marking cq- because we didn't hear from Darin.

[Joshua Bell]

(In reply to comment #16)
> (From update of attachment 123833 [details])
> On second look, marking cq- because we didn't hear from Darin.

Oops, yeah, thanks.

[Darin Fisher (:fishd, Google)]

Comment on attachment 123833 [details]
Patch

API changes LGTM ... sorry for the latency.  CQ+

[WebKit Review Bot]

Comment on attachment 123833 [details]
Patch

Clearing flags on attachment: 123833

Committed r105891: <http://trac.webkit.org/changeset/105891>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.