Bug 91123 – IndexedDB: inject index keys on cursor/objectstore/index get success handlers

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Alec Flett]

Once bug 90923 lands, we'll be able to full implement this. Note that this doesn't cover the createIndex() case

[Alec Flett]

Created attachment 154394 [details]
Patch

[Alec Flett]

finally finally the tree is almost ready for me to land this. This is the final patch to allow get/put/update to deal with renderer-side key injection. This leaves around a lot of cruft because the the patch for createIndex is still coming.

This particular patch includes that #include-reduction that I did, so you can ignore that stuff, it will be gone from the final patch and I'll update the patch here when webkit rolls next.

jsbell/dgrogan: mind taking a look.

[Joshua Bell]

Comment on attachment 154394 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        IndexedDB: inject index keys on cursor/objectstore/index get succes handlers

Typo "succes" -> "success"; I updated the bug itself.

> Source/WebCore/ChangeLog:19
> +        assertion was removed from IDBBindingUtilities.

Haven't gotten to that point in the patch, but it seems like we could either:
* Do an extract and only inject if they differ (so keep the assert)
* Modify the assert so that it's okay if the value being injected is the same as the current value
...
But now that I've gotten to that point, I agree that the binding utilities should stay "pure" and not be enforcing this logic. So I'd be tempted to add in an debug-only logic at the inject...() call site that either the value is not there or the value is there and matches what we'd inject.

> Source/WebCore/ChangeLog:26
> +        No new tests because this is a performance refactoring, and actual

Do we have any actual perf data on this? (I'm guessing "not yet", so might be worth being more conservative in the comments.)

> Source/WebCore/ChangeLog:27
> +        behavior hasn't changed.

Do we have any existing tests where the index key path is the same as the store's key path and autoincrement is used?

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:161
> +    } else {

The "if (usesInLineKeys) ..." code is really a runtime validation that the new value doesn't change the key. It seems like the "else" case here should run in any case so that nothing downstream needs to compute a key.

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:165
> +    return objectStore->put(IDBObjectStoreBackendInterface::CursorUpdate, IDBAny::create(this), context, value, key, ec);

i.e. just pass m-currentPrimaryKey in here instead of key.

> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:88
> +    ASSERT_NOT_REACHED();

I believe this method can be removed as well as any dependents on it (IDBCursorBackendInterface.h, IDBCursorBackendProxy.cpp, WebIDBCursorImpl.cpp) - the WEBKIT_ASSERT_NOT_REACHED in WebIDBCursor::update() is all that's necessary to keep Chromium building if nothing is calling it.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:105
> +static bool generateIndexKeysForValue(const IDBIndexMetadata& indexMetadata,

This never appears to return false?

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:109
> +    RefPtr<IDBKey> indexKey = createIDBKeyFromSerializedValueAndKeyPath(objectValue, indexMetadata.keyPath);

Add ASSERT(indexKeys.get()); (or pass as reference)

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:142
> +PassRefPtr<IDBRequest> IDBObjectStore::put(IDBObjectStoreBackendInterface::PutMode putMode, PassRefPtr<IDBAny> source, ScriptExecutionContext* context, PassRefPtr<SerializedScriptValue> prpValue, PassRefPtr<IDBKey> prpKey, ExceptionCode& ec)

Nice simplification. :)

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:198
>      if (key && !key->isValid()) {

By this point, the only case where key is NULL is if it's going to be autogenerated, correct?

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:127
> +    ASSERT_NOT_REACHED();

Same as above on removing this - only the WebIDBObjectStore::put() stub needs to remain, the ...BackendImpl, ...BackendInterface, ...BackendProxy and Web...Impl versions can go.

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:343
>  void IDBRequest::onSuccess(PassRefPtr<SerializedScriptValue> prpSerializedScriptValue, PassRefPtr<IDBKey> prpPrimaryKey, const IDBKeyPath& keyPath)

This should only be used in response to an IDBObjectStore.get() or IDBIndex.get(), correct? In that case, the keyPath should match the keyPath of the request's source's "effectiveObjectStore" (not a real concept, but borrowing from IDBCursor), right?

In that case... can we either drop the keyPath parameter or at least ASSERT that they match?

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:351
> +            injectIDBKeyIntoSerializedValue(primaryKey, value, keyPath);

Don't need to wrap this line.

Ideally this code would match the IDBCursor::setValueReady code. They seem to handle injection failures differently.

(Thinking towards further refactors, it would be nice if the updated cursor key/primarykey/value were passed back in onSuccessWithContinuation, stashed in the IDBRequest itself (not the cursor) and then the key injection for cursors could be done in IDBRequest.)

> Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:-104
> -    ASSERT(!object->Has(indexOrName));

Agreed, let's keep this pure.

[Alec Flett]

Comment on attachment 154394 [details]
Patch

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

Ok, nits addressed, new patch coming.

>> Source/WebCore/ChangeLog:19
>> +        assertion was removed from IDBBindingUtilities.
> 
> Haven't gotten to that point in the patch, but it seems like we could either:
> * Do an extract and only inject if they differ (so keep the assert)
> * Modify the assert so that it's okay if the value being injected is the same as the current value
> ...
> But now that I've gotten to that point, I agree that the binding utilities should stay "pure" and not be enforcing this logic. So I'd be tempted to add in an debug-only logic at the inject...() call site that either the value is not there or the value is there and matches what we'd inject.

I added (back) the assertions - they seem like reasonable checks

>> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:161
>> +    } else {
> 
> The "if (usesInLineKeys) ..." code is really a runtime validation that the new value doesn't change the key. It seems like the "else" case here should run in any case so that nothing downstream needs to compute a key.

Well as they say, "it's complicated" :)

it has to be set to null here mostly to get by other checks. But actually fix on the other end (In IDBObjectStore::put) is less complicated than simply passing m_currentPrimaryKey, so I did that.

>> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:105
>> +static bool generateIndexKeysForValue(const IDBIndexMetadata& indexMetadata,
> 
> This never appears to return false?

oh wow. Didn't even notice that I had removed all failure cases :) Fixed this and the other one in IDBObjectStoreImpl.cpp

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:198
>      if (key && !key->isValid()) {

this is one of those subtle cases of 
  if (key && !key->isValid())
rather than 
  if (!key || !key->isValid()) -

 i.e. the extracted key value is present, but invalid.

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:343
>  void IDBRequest::onSuccess(PassRefPtr<SerializedScriptValue> prpSerializedScriptValue, PassRefPtr<IDBKey> prpPrimaryKey, const IDBKeyPath& keyPath)

That's why this is like this - there isn't really enough in the way of breadcrumbs to track our way back to the equivalent of the effectiveObjectStore.

I added some debug code, which also required implementing operator== on IDBKeyPath

>> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:351
>> +            injectIDBKeyIntoSerializedValue(primaryKey, value, keyPath);
> 
> Don't need to wrap this line.
> 
> Ideally this code would match the IDBCursor::setValueReady code. They seem to handle injection failures differently.
> 
> (Thinking towards further refactors, it would be nice if the updated cursor key/primarykey/value were passed back in onSuccessWithContinuation, stashed in the IDBRequest itself (not the cursor) and then the key injection for cursors could be done in IDBRequest.)

Totally agree - been wanting to do that refactor for a while.

Good catch on the inconsistency in setValueReady - I at least made it succeed gracefully with a FIXME.

[Alec Flett]

Created attachment 154467 [details]
Patch

[Joshua Bell]

(In reply to comment #4)
> > (Thinking towards further refactors, it would be nice if the updated cursor key/primarykey/value were passed back in onSuccessWithContinuation, stashed in the IDBRequest itself (not the cursor) and then the key injection for cursors could be done in IDBRequest.)
> 
> Totally agree - been wanting to do that refactor for a while.

FYI, I have that (mostly) done locally, tracking in https://bugs.webkit.org/show_bug.cgi?id=92278

Needs to wait for the dust to settle, before breaking it up for landing, though.

[Alec Flett]

Already referenced that but in the patch! :)

[Joshua Bell]

Comment on attachment 154467 [details]
Patch

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

Did you see my question "Do we have any existing tests where the index key path is the same as the store's key path and autoincrement is used?"

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:258
> +        const IDBObjectStoreMetadata metadata = objectStore->metadata();

To simplify, maybe we want to have IDBObjectStore::keyPath() return an IDBKeyPath and implement the IDL keyPath attribute via a method named keyPathAny() instead.

Then we can ditch the metadata() uses here, and the IDBKeyPath::operator== can take an IDBKeyPaths instead of an IDBAny.

> Source/WebCore/Modules/indexeddb/IDBKeyPath.cpp:249
> +    case ArrayType:

Missing an other->type() == IDBAny::DOMStringListType check?

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:360
> +    ASSERT(keyPath == effectiveObjectStore(m_source)->keyPath());

Thanks, and sorry for the pain.

[Alec Flett]

Created attachment 154487 [details]
Patch

[Alec Flett]

Comment on attachment 154467 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:258
>> +        const IDBObjectStoreMetadata metadata = objectStore->metadata();
> 
> To simplify, maybe we want to have IDBObjectStore::keyPath() return an IDBKeyPath and implement the IDL keyPath attribute via a method named keyPathAny() instead.
> 
> Then we can ditch the metadata() uses here, and the IDBKeyPath::operator== can take an IDBKeyPaths instead of an IDBAny.

tried that but the binding code calls keyPath(), so we'd have to change the IDL... lets tackle that elsewhere

>> Source/WebCore/Modules/indexeddb/IDBKeyPath.cpp:249
>> +    case ArrayType:
> 
> Missing an other->type() == IDBAny::DOMStringListType check?

whoops! done.

[Alec Flett]

sorry I keep ignoring the question about the tests - I keep meaning to answer then getting into other stuff. let me look now.

[Alec Flett]

I can't find any at first pass.

I suspect that currently most practical uses of that will fail in the current implementation. Specifically if you  did:

objectStore = createObjectStore("XX", {autoIncrement: true, keyPath: "foo"})
index = objectStore.createIndex("foo", "foo");

Then when you call put() you're almost certainly adding objects that have the "foo" property set:

objectStore.put({"foo": "xyz"})

In the current code I believe this will fail because createObjectStore will look for the current key and fail because it's supposed to be autoIncrement.

If you tried
objectStore.put({})

Then I think the code might fail to generate an index key for "foo", then write the object to the autoIncremented new value, (because it does pre-generate the index keys first, even in the backend) and the same will happen after this patch, for wildly different reasons: because the key would be 'generated' until the object came out of the store.

I'll have to think about how to fix that case.

[Alec Flett]

Created attachment 154748 [details]
Patch

[Alec Flett]

ok, new patch includes a test for what we described above - sure enough, it was working on trunk, the previous patch broke it, so this new patch has a fix.

[Joshua Bell]

Comment on attachment 154748 [details]
Patch

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

> Source/WebCore/ChangeLog:27
> +        No new tests because this is a refactoring, and actual behavior

Update comment with new test name.

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:286
> +            if (indexKeyPath == objectStore->keyPath())

This creates an temporary IDBAny (via cast operator on IDBKeyPath); it would be nice to avoid it. (The other use of the IDBKeyPath::operator==(IDBAny) is in debug code.)

> LayoutTests/storage/indexeddb/resources/index-duplicate-keypaths.js:12
> +    request.onsuccess = testCollideObjstoreIndexSetup;

The "Objstore" contraction is a little odd. Use "ObjStore", "ObjectStore" or just "Store".

> LayoutTests/storage/indexeddb/resources/index-duplicate-keypaths.js:21
> +    request.onerror = unexpectedErrorCallback;

Catch onblocked too, please.

> LayoutTests/storage/indexeddb/resources/index-duplicate-keypaths.js:27
> +    trans = event.target.result;

FYI, using |event.target.result| rather than |request.result| makes the test slightly more work to port to Workers (which don't have a global |event|). Not a big deal, though.

> LayoutTests/storage/indexeddb/resources/index-duplicate-keypaths.js:28
> +    deleteAllObjectStores(db);

Duplicated.

> LayoutTests/storage/indexeddb/resources/index-duplicate-keypaths.js:40
> +        shouldBeEqualToString("JSON.stringify(result)", v);

I'm slightly worried that the serialization order is indeterminate, but I suspect in practice it's sorted by key to avoid such issues. I agree this is probably the best way to do a quick compare of these sorts of values.

> LayoutTests/storage/indexeddb/resources/index-duplicate-keypaths.js:69
> +    deleteAllObjectStores(db);

Duplicated.

> LayoutTests/storage/indexeddb/resources/index-duplicate-keypaths.js:84
> +    // insert some data to futz with the autoIncrement state

Capitalize, punctuate.

> LayoutTests/storage/indexeddb/resources/index-duplicate-keypaths.js:86
> +        objectStore.put({foo: i});

Can you do this in an evalAndLog() so a reader of the expected.txt knows what's going on?

> LayoutTests/storage/indexeddb/resources/index-duplicate-keypaths.js:90
> +        debug("got back " + JSON.stringify(event.target.result));

I think this debug() line is redundant with the following shouldBe() call.

[Alec Flett]

Comment on attachment 154748 [details]
Patch

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

test issues addressed.

>> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:286
>> +            if (indexKeyPath == objectStore->keyPath())
> 
> This creates an temporary IDBAny (via cast operator on IDBKeyPath); it would be nice to avoid it. (The other use of the IDBKeyPath::operator==(IDBAny) is in debug code.)

That's only in the FRONT end - in the backend, keyPath() returns an IDBKeyPath (Another reason to fix that!)

[Alec Flett]

Created attachment 154772 [details]
Patch

[Alec Flett]

Created attachment 154776 [details]
Patch

[Alec Flett]

the patch for cleaning up the keyPath() junk is bug 92434

[Alec Flett]

Created attachment 154780 [details]
Patch

[Alec Flett]

tony@ - r?

[Tony Chang]

Comment on attachment 154780 [details]
Patch

rs=me

[WebKit Review Bot]

Comment on attachment 154780 [details]
Patch

Clearing flags on attachment: 154780

Committed r123843: <http://trac.webkit.org/changeset/123843>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.