Bug 164359 – IndexedDB 2.0: Support binary keys

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

[Brady Eidson]

IndexedDB 2.0: Support binary keys

[Brady Eidson]

<rdar://problem/28806927>

[Brady Eidson]

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.

[Brady Eidson]

Created attachment 293794 [details]
Patch

[Alex Christensen]

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.

[Brady Eidson]

(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.

[Brady Eidson]

Created attachment 293798 [details]
Patch

[Brady Eidson]

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

New one coming

[Brady Eidson]

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

[Ryan Haddad]

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

[Brady Eidson]

(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.

[Brady Eidson]

(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