Bug 164359

<rdar://problem/28806927>

Created attachment 293750 [details] WIP This patch is most of the task, and the included layout test that does a lot of IDBKey creation and inspection actually works. Final bit is getting binary keys to actually work in the 2 backing stores which shouldn't actually be too difficult.

Created attachment 293794 [details] Patch

Comment on attachment 293794 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293794&action=review r=me with comments Could we add two tests? 1) Use an array as a key to put a value in the database, change the original array, make sure that using the original array as a key returns no value to verify that the array was copied. 2) Use an array as a key to put a value in the database, use a different type of array or string with the same binary values. Such as [97, 98, 99, 100], "abcd", and [1633837924] with different types (Uint32Array/Uint8Array). I'm not sure if the type is supposed to be part of the key, or if the same bytes in different typed arrays (does endianness matter?) should be the same key. > Source/WTF/wtf/Hasher.h:248 > + size_t remainder = length % sizeof(UChar); > + for (size_t i = 0; i < remainder; ++i) Can this be just if (length % sizeof(UChar)) since sizeof(UChar) is 2? > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:111 > + if (!data) > + return jsNull(); Is this correct? No exception? Can this be reached? > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:116 > + if (!structure) > + return jsNull(); Ditto.

(In reply to comment #4) > Comment on attachment 293794 [details] > > > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:111 > > + if (!data) > > + return jsNull(); > > Is this correct? No exception? Can this be reached? Shouldn't happen, code that calls in doesn't expect for an exception. Can add an ASSERT. > > > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:116 > > + if (!structure) > > + return jsNull(); > > Ditto. Out of my wheelhouse, I have no idea.

Created attachment 293798 [details] Patch

This patch won't pass tests, forgot to regenerate results. *sigh* New one coming

The test failures on the bots are the results I forgot to update in this patch. Proper results landed with the patch in https://trac.webkit.org/changeset/208349

This change appears to have introduced an API test failure: https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK1%20(Tests)/builds/19500 FAIL WTF.StringHasher_hashMemory /Volumes/Data/slave/yosemite-release/build/Tools/TestWebKitAPI/Tests/WTF/StringHasher.cpp:430 Value of: StringHasher::hashMemory(0, 0) Actual: 82610334 Expected: emptyStringHash & 0xFFFFFF Which is: 15501470

(In reply to comment #9) > This change appears to have introduced an API test failure: > > https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK1%20(Tests)/ > builds/19500 > > FAIL WTF.StringHasher_hashMemory > > /Volumes/Data/slave/yosemite-release/build/Tools/TestWebKitAPI/Tests/WTF/ > StringHasher.cpp:430 > Value of: StringHasher::hashMemory(0, 0) > Actual: 82610334 > Expected: emptyStringHash & 0xFFFFFF > Which is: 15501470 This is a behavior change, but it's unclear to me whether it's an important one. The test appears to be asserting that the hash of "'\0', length 1" should be equal to the hash of "nullptr, length 0" And I'm not sure why that assertion should hold.

(In reply to comment #10) > (In reply to comment #9) > > This change appears to have introduced an API test failure: > > > > https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK1%20(Tests)/ > > builds/19500 > > > > FAIL WTF.StringHasher_hashMemory > > > > /Volumes/Data/slave/yosemite-release/build/Tools/TestWebKitAPI/Tests/WTF/ > > StringHasher.cpp:430 > > Value of: StringHasher::hashMemory(0, 0) > > Actual: 82610334 > > Expected: emptyStringHash & 0xFFFFFF > > Which is: 15501470 > > This is a behavior change, but it's unclear to me whether it's an important > one. > > The test appears to be asserting that the hash of "'\0', length 1" should be > equal to the hash of "nullptr, length 0" > > And I'm not sure why that assertion should hold. Filed https://bugs.webkit.org/show_bug.cgi?id=164390 to fix this