Bug 150468

As part of this, fix the test skipped in https://trac.webkit.org/changeset/191470

Created attachment 264141 [details] Patch v1

Add's basic support. There's lots of little edge cases that cannot be tested without other patches, and will be added/tested in upcoming work.

Attachment 264141 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/indexeddb/client/TransactionOperation.h:101: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 57 files If any of these errors are false positives, please file a bug against check-webkit-style.

GCC strikes again: ../../Source/WebCore/Modules/indexeddb/IDBKeyData.cpp:358:1: error: control reaches end of non-void function [-Werror=return-type] Except... no, because the switch block handles all cases. *sigh* Will workaround.

Created attachment 264143 [details] Patch v2

Attachment 264143 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/indexeddb/client/TransactionOperation.h:101: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 57 files If any of these errors are false positives, please file a bug against check-webkit-style.

Forgot one piece that needs to be in here.

Created attachment 264145 [details] Patch v3 NM, the thing I wanted to add was going to expand the scope of the patch way too much.

Attachment 264145 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/indexeddb/client/TransactionOperation.h:101: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 57 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 264143 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=264143&action=review > LayoutTests/storage/indexeddb/modern/basic-put.html:48 > + tx.onabort = function(event) { Can the putRequest or getRequest be aborted? Please add tests where things go wrong. > LayoutTests/storage/indexeddb/modern/keypath-basic.html:49 > + request3.onsuccess = function(event) { > + alert("Second object put SUCCESS - " + request3.result); > + } Is the order of these callbacks guaranteed, or will we have another flaky test when the callbacks happen in a different order. > Source/WebCore/Modules/indexeddb/IDBKeyData.cpp:343 > + return true; Don't you need to compare m_type and other.m_type? > Source/WebCore/Modules/indexeddb/IDBKeyData.cpp:358 > + RELEASE_ASSERT_NOT_REACHED(); This can't be reached. Did you mean to put a default before this? > Source/WebCore/Modules/indexeddb/IDBKeyData.h:106 > + hashCodes.append(m_isNull ? 1 : 0); > + hashCodes.append(m_isDeletedValue ? 1 : 0); Wouldn't it be better to add some large prime numbers instead of 1 or 0? > Source/WebCore/Modules/indexeddb/IDBKeyData.h:107 > + switch (m_type) { Why not add m_type to the hash function? That might be related to Invalid, Max, and Min always being equal, which seems strange, but might be correct. > Source/WebCore/Modules/indexeddb/IDBKeyData.h:121 > + for (auto& key : m_arrayValue) > + hashCodes.append(key.hash()); This is an unnecessary copy. Just hash in place. > Source/WebCore/Modules/indexeddb/client/IDBObjectStoreImpl.cpp:156 > + return adoptRef(request.leakRef()); I think we need to make a better way to make a RefPtr from a Ref. We're putting more and more of these in. > Source/WebCore/Modules/indexeddb/client/IDBTransactionImpl.cpp:342 > + request->setResultToStructuredClone(resultData.resultData()); Early return, ASSERT(request), or make it a Ref<IDBRequest> > Source/WebCore/Modules/indexeddb/client/IDBTransactionImpl.cpp:375 > + request->setResult(resultData.resultKey()); ditto. > Source/WebCore/Modules/indexeddb/client/TransactionOperation.h:101 > + m_completeFunction = [self, this, request, completeMethod](const IDBResultData& resultData) { Stylebot wants the { on the next line. Strange that it didn't complain about the lambda with no arguments just before this. > Source/WebCore/Modules/indexeddb/server/MemoryBackingStoreTransaction.cpp:84 > + { Why scope? > Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:179 > + return IDBError(IDBExceptionCode::Unknown, WTF::ASCIILiteral("No backing store object store found to put record")); There are a lot of error cases here that are untested. > Source/WebCore/Modules/indexeddb/server/MemoryObjectStore.cpp:86 > + m_keyValueStore->remove(key); What happens when we try to delete a record that is not there? Should there be an assert that it contains the key? (I'm guessing the answer is no, everything is ok.) > Source/WebCore/platform/ThreadSafeDataBuffer.h:42 > + m_data.swap(data); Is the swap operation thread safe?

(In reply to comment #11) > Comment on attachment 264143 [details] > Patch v2 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=264143&action=review > > > LayoutTests/storage/indexeddb/modern/basic-put.html:48 > > + tx.onabort = function(event) { > > Can the putRequest or getRequest be aborted? Please add tests where things > go wrong. Requests cannot be aborted directly. And so far in this implementation not even indirectly. > > > LayoutTests/storage/indexeddb/modern/keypath-basic.html:49 > > + request3.onsuccess = function(event) { > > + alert("Second object put SUCCESS - " + request3.result); > > + } > > Is the order of these callbacks guaranteed, or will we have another flaky > test when the callbacks happen in a different order. The order is guaranteed. The flaky test was not because of an order-of-callbacks issue, but because there *wasn't* an explicit callback for what we were trying to test. > > Source/WebCore/Modules/indexeddb/IDBKeyData.cpp:343 > > + return true; > > Don't you need to compare m_type and other.m_type? Yes. Which is why we do: bool IDBKeyData::operator==(const IDBKeyData& other) const { if (m_type != other.m_type || m_isNull != other.m_isNull || m_isDeletedValue != other.m_isDeletedValue) return false; ... > > > Source/WebCore/Modules/indexeddb/IDBKeyData.cpp:358 > > + RELEASE_ASSERT_NOT_REACHED(); > > This can't be reached. Did you mean to put a default before this? I agree that this cannot be reached. However the EFL compiler does *not* agree: https://webkit-queues.webkit.org/results/345012 So... yet another workaround for that breakage. > > > Source/WebCore/Modules/indexeddb/IDBKeyData.h:106 > > + hashCodes.append(m_isNull ? 1 : 0); > > + hashCodes.append(m_isDeletedValue ? 1 : 0); > > Wouldn't it be better to add some large prime numbers instead of 1 or 0? No. If you have data on why it would be, we should revisit all custom hashing project-wide. > > Source/WebCore/Modules/indexeddb/IDBKeyData.h:107 > > + switch (m_type) { > > Why not add m_type to the hash function? That might be related to Invalid, > Max, and Min always being equal, which seems strange, but might be correct. Min/Max/Invalid are not equal, so that's why we *do* hash m_type 3 lines above this: unsigned hash() const { Vector<unsigned> hashCodes; hashCodes.append(static_cast<unsigned>(m_type)); ... Or, did you mean something else...? > > > Source/WebCore/Modules/indexeddb/IDBKeyData.h:121 > > + for (auto& key : m_arrayValue) > > + hashCodes.append(key.hash()); > > This is an unnecessary copy. Just hash in place. I'm confused by your comment. The hash of "an array of objects" depends on the hash of each individual object in the array. I don't see what I'm copying here - I'm just getting the hash from the subobjects. > > > Source/WebCore/Modules/indexeddb/client/IDBObjectStoreImpl.cpp:156 > > + return adoptRef(request.leakRef()); > > I think we need to make a better way to make a RefPtr from a Ref. We're > putting more and more of these in. I agree. I hate doing this. Outside the scope of this patch. > > > Source/WebCore/Modules/indexeddb/client/IDBTransactionImpl.cpp:342 > > + request->setResultToStructuredClone(resultData.resultData()); > > Early return, ASSERT(request), or make it a Ref<IDBRequest> Will do. > > Source/WebCore/Modules/indexeddb/client/IDBTransactionImpl.cpp:375 > > + request->setResult(resultData.resultKey()); > > ditto. Will do. > > Source/WebCore/Modules/indexeddb/client/TransactionOperation.h:101 > > + m_completeFunction = [self, this, request, completeMethod](const IDBResultData& resultData) { > > Stylebot wants the { on the next line. Strange that it didn't complain about > the lambda with no arguments just before this. Stylebot is wrong, this is established style for lambdas. I've filed the bugzilla (a long time ago). > > > Source/WebCore/Modules/indexeddb/server/MemoryBackingStoreTransaction.cpp:84 > > + { > > Why scope? That's a great question - historical vestige, used to be a locker-type object in there, forgot to remove it. > > > Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:179 > > + return IDBError(IDBExceptionCode::Unknown, WTF::ASCIILiteral("No backing store object store found to put record")); > > There are a lot of error cases here that are untested. Yup. Some I don't know how they can go wrong ever, and others I cannot make go wrong *yet*. > > Source/WebCore/Modules/indexeddb/server/MemoryObjectStore.cpp:86 > > + m_keyValueStore->remove(key); > > What happens when we try to delete a record that is not there? Should there > be an assert that it contains the key? (I'm guessing the answer is no, > everything is ok.) No, everything is ok. > > > Source/WebCore/platform/ThreadSafeDataBuffer.h:42 > > + m_data.swap(data); > > Is the swap operation thread safe? The swap operation only takes place on the initial construction of the ThreadSafeDataBufferImpl, which by definition is on a single thread before the wrapper can have been passed to a background thread.

Comment on attachment 264143 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=264143&action=review >>> Source/WebCore/Modules/indexeddb/IDBKeyData.cpp:358 >>> + RELEASE_ASSERT_NOT_REACHED(); >> >> This can't be reached. Did you mean to put a default before this? > > I agree that this cannot be reached. > > However the EFL compiler does *not* agree: > https://webkit-queues.webkit.org/results/345012 > > So... yet another workaround for that breakage. :( I guess it could be reached if we somehow had a KeyType out of range, which will never happen. This is good. >>> Source/WebCore/Modules/indexeddb/IDBKeyData.h:107 >>> + switch (m_type) { >> >> Why not add m_type to the hash function? That might be related to Invalid, Max, and Min always being equal, which seems strange, but might be correct. > > Min/Max/Invalid are not equal, so that's why we *do* hash m_type 3 lines above this: > unsigned hash() const > { > Vector<unsigned> hashCodes; > hashCodes.append(static_cast<unsigned>(m_type)); > ... > > Or, did you mean something else...? Oh, I must have overlooked that. Same with the check in the operator ==. >>> Source/WebCore/Modules/indexeddb/IDBKeyData.h:121 >>> + hashCodes.append(key.hash()); >> >> This is an unnecessary copy. Just hash in place. > > I'm confused by your comment. The hash of "an array of objects" depends on the hash of each individual object in the array. I don't see what I'm copying here - I'm just getting the hash from the subobjects. I looked at this wrong. I thought we were copying the keys into the array, but we are indeed only appending the hash codes. This is fine.

Comment on attachment 264145 [details] Patch v3 All right, r=me once you remove the unneeded scope and add the asserts or early returns where noted earlier.

Created attachment 264155 [details] Patch v4 - Handled review feedback

(In reply to comment #14) > Comment on attachment 264145 [details] > Patch v3 > > All right, r=me once you remove the unneeded scope and add the asserts or > early returns where noted earlier. Oh okay! thx

Attachment 264155 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/indexeddb/client/TransactionOperation.h:102: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 57 files If any of these errors are false positives, please file a bug against check-webkit-style.