IndexedDB: Fix index data invalidation bugs.
Created attachment 102662 [details] Patch
Comment on attachment 102662 [details] Patch Oops, forgot to upload the expectations.
Created attachment 102665 [details] Patch
David: would you like to do an informal review, and then I'll find someone who can properly review it?
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?
(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.
Created attachment 102758 [details] Patch Use CONSTRAINT_ERR for uniqueness constraint violation.
Tony, would you like to take a look?
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.
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.
(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.
Committed r92364: <http://trac.webkit.org/changeset/92364>