Bug 65547 – IndexedDB: Fix index data invalidation bugs.

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

[Hans Wennborg]

IndexedDB: Fix index data invalidation bugs.

[Hans Wennborg]

Created attachment 102662 [details]
Patch

[Hans Wennborg]

Comment on attachment 102662 [details]
Patch

Oops, forgot to upload the expectations.

[Hans Wennborg]

Created attachment 102665 [details]
Patch

[Hans Wennborg]

David: would you like to do an informal review, and then I'll find someone who can properly review it?

[David Grogan]

Comment on attachment 102665 [details]
Patch

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

> LayoutTests/storage/indexeddb/index-unique.html:76
> +    shouldBe("event.target.errorCode", "webkitIDBDatabaseException.UNKNOWN_ERR");

We throw UNKNOWN_ERR?  Is that to spec or just what we currently throw?

> LayoutTests/storage/indexeddb/index-unique.html:111
> +    request = evalAndLog("transaction.objectStore('store').put({x: 1}, 'bar')");

This is the first operation whose behavior is altered by this patch, correct?

> LayoutTests/storage/indexeddb/index-unique.html:130
> +    request = evalAndLog("transaction.objectStore('store').put({x: 1}, 'baz')");

Should this should test with 'bar' instead of 'baz'?  It was the primary key whose ExistsEntryKey was erroneously left around, wasn't it?

> Source/WebCore/storage/IDBLevelDBBackingStore.cpp:-804
> -PassRefPtr<IDBKey> IDBLevelDBBackingStore::getPrimaryKeyViaIndex(int64_t databaseId, int64_t objectStoreId, int64_t indexId, const IDBKey& key)

It seems that nothing was wrong with this function, it was just refactored so that keyExistsInIndex could use most of its functionality.  Accurate?

> Source/WebCore/storage/IDBLevelDBBackingStore.cpp:824
>          if (!p)

Is this expected to be triggered, or does it indicate corruption?

> Source/WebCore/storage/IDBLevelDBBackingStore.cpp:830
> +            transaction->remove(it->key());

We end up with stale index data when a (primary key, value) pair is overwritten?

[Hans Wennborg]

(In reply to comment #5)
> (From update of attachment 102665 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102665&action=review
> 
> > LayoutTests/storage/indexeddb/index-unique.html:76
> > +    shouldBe("event.target.errorCode", "webkitIDBDatabaseException.UNKNOWN_ERR");
> 
> We throw UNKNOWN_ERR?  Is that to spec or just what we currently throw?
It's what we currently throw. I now see that the specc says it should be CONSTRAINT_ERR, so let's fix that while we're at it.

> > LayoutTests/storage/indexeddb/index-unique.html:111
> > +    request = evalAndLog("transaction.objectStore('store').put({x: 1}, 'bar')");
> 
> This is the first operation whose behavior is altered by this patch, correct?
Exactly.

> 
> > LayoutTests/storage/indexeddb/index-unique.html:130
> > +    request = evalAndLog("transaction.objectStore('store').put({x: 1}, 'baz')");
> 
> Should this should test with 'bar' instead of 'baz'?  It was the primary key whose ExistsEntryKey was erroneously left around, wasn't it?
The ExistsEntryKey was erroneously left around, which meant that the x=1 key in the index, which pointed to that primary key, was considered "active".

The point is that we can add a value with x = 1 again.. the primary key of the new entry is not important.

> 
> > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:-804
> > -PassRefPtr<IDBKey> IDBLevelDBBackingStore::getPrimaryKeyViaIndex(int64_t databaseId, int64_t objectStoreId, int64_t indexId, const IDBKey& key)
> 
> It seems that nothing was wrong with this function, it was just refactored so that keyExistsInIndex could use most of its functionality.  Accurate?
Yes, I just factored out all the stuff except the decoding of the primary key into a new function. The diff makes it harder to read than it really is.


> 
> > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:824
> >          if (!p)
> 
> Is this expected to be triggered, or does it indicate corruption?
Right, this is not expected to be triggered.

> 
> > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:830
> > +            transaction->remove(it->key());
> 
> We end up with stale index data when a (primary key, value) pair is overwritten?

Yes, either (primary key, value) is overwritten (and thus gets a new version number), or the (primary key, value) is removed.

[Hans Wennborg]

Created attachment 102758 [details]
Patch

Use CONSTRAINT_ERR for uniqueness constraint violation.

[Hans Wennborg]

Tony, would you like to take a look?

[David Grogan]

LGTM


(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 102665 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=102665&action=review
> > > LayoutTests/storage/indexeddb/index-unique.html:130
> > > +    request = evalAndLog("transaction.objectStore('store').put({x: 1}, 'baz')");
> > 
> > Should this should test with 'bar' instead of 'baz'?  It was the primary key whose ExistsEntryKey was erroneously left around, wasn't it?
> The ExistsEntryKey was erroneously left around, which meant that the x=1 key in the index, which pointed to that primary key, was considered "active".
> 
> The point is that we can add a value with x = 1 again.. the primary key of the new entry is not important.

Ah, ok.  I can see that.

[Tony Chang]

Comment on attachment 102758 [details]
Patch

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

> Source/WebCore/storage/IDBLevelDBBackingStore.cpp:808
> +static bool findKeyInIndex(LevelDBTransaction* transaction, int64_t databaseId, int64_t objectStoreId, int64_t indexId, const IDBKey& key, Vector<char>* foundEncodedPrimaryKey)

Nit: In WebKit, it's more common to use pass by reference for out variables.  I would change |foundEncodedPrimaryKey| to be consistent with the rest of the file.

[Hans Wennborg]

(In reply to comment #10)
> (From update of attachment 102758 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102758&action=review

Thanks Tony!

> 
> > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:808
> > +static bool findKeyInIndex(LevelDBTransaction* transaction, int64_t databaseId, int64_t objectStoreId, int64_t indexId, const IDBKey& key, Vector<char>* foundEncodedPrimaryKey)
> 
> Nit: In WebKit, it's more common to use pass by reference for out variables.  I would change |foundEncodedPrimaryKey| to be consistent with the rest of the file.
Fixed.

[Hans Wennborg]

Committed r92364: <http://trac.webkit.org/changeset/92364>