Bug 150743 - IDB: Date objects don't work as keys or values
Summary: IDB: Date objects don't work as keys or values
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Safari 9
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks: 149117
  Show dependency treegraph
 
Reported: 2015-10-30 17:21 PDT by Brady Eidson
Modified: 2015-11-01 14:50 PST (History)
2 users (show)

See Also:


Attachments
Patch v1 (36.33 KB, patch)
2015-10-30 22:47 PDT, Brady Eidson
darin: review+
beidson: commit-queue?
Details | Formatted Diff | Diff
Patch for landing. (35.94 KB, patch)
2015-10-31 20:47 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2015-10-30 17:21:21 PDT
IDB: Date objects don't work as keys

This appears to affect Legacy IDB in addition to Modern IDB.

If I can fix it at the bindings, it'll be fixed in both.
If not, I'll only fix it for Modern IDB.
Comment 1 Brady Eidson 2015-10-30 17:22:13 PDT
In addition to a test that deals solely with dates-as-keys, the test landed in https://trac.webkit.org/changeset/191832 can be augmented with dates.
Comment 2 Brady Eidson 2015-10-30 22:47:13 PDT
Created attachment 264465 [details]
Patch v1
Comment 3 Brady Eidson 2015-10-30 22:48:01 PDT
(In reply to comment #1)
> In addition to a test that deals solely with dates-as-keys, the test landed
> in https://trac.webkit.org/changeset/191832 can be augmented with dates.

This patch includes a new targeted test as well as adding Dates to the r191832 test
Comment 4 Darin Adler 2015-10-31 14:57:12 PDT
Comment on attachment 264465 [details]
Patch v1

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

All the function arguments should be just JSValue instead of const JSValue&, because a JSValue is a small scalar and better to pass by value.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.idl:37
> +    [CallWith=ScriptExecutionContext, Custom, ImplementedAs=putRecord, RaisesException] IDBRequest put(any value, optional any key);

The ImplementedAs is awkward. I assume there's a name conflict, but I don’t understand why put has a conflict and add does not.

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:310
> +        Vector<RefPtr<IDBKey>> result;

result.reserveInitialCapacity(array.size());

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:312
> +        for (size_t i = 0; i < array.size(); i++) {

Use a modern for loop here:

    for (auto& string : array) {

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:316
> +            result.append(key);

WTF::move(key)

    result.uncheckedAppend(WTF::move(key));

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:318
> +        return IDBKey::createArray(result);

WTF::move(result) maybe? Not sure it’s helpful.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:59
> +    size_t argsCount = state.argumentCount();
> +    if (UNLIKELY(argsCount < 1))
> +        return JSValue::decode(throwVMError(&state, createNotEnoughArgumentsError(&state)));

As I understand it, we should almost never be checking argument count. Modern DOM bindings check for undefined to see if an argument is missing, and never have to check the argument count itself.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:61
> +    auto value = state.argument(0);

If we retain the argumentCount check, then this should be uncheckedArgument(0). If we get rid of it, then we’ll likely be checking for undefined at some point.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:63
> +    if (UNLIKELY(state.hadException()))
> +        return jsUndefined();

This hadException check is not needed; extracting an argument from ExecState cannot cause an exception.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:72
> +        if (overwrite)
> +            result = toJS(&state, castedThis->globalObject(), WTF::getPtr(impl.put(state, value, ec)));
> +        else
> +            result = toJS(&state, castedThis->globalObject(), WTF::getPtr(impl.add(state, value, ec)));

Hand written code should not need to use WTF::getPtr. Instead we can either put in a .get() or not depending on the specific type. The automatically generated code uses getPtr to be generic, but that’s not important for hand-written code.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:77
> +        if (UNLIKELY(state.hadException()))
> +            return jsUndefined();
> +        return result;

The hadException check is not needed; the result will be ignored if an exception is raised; it’s OK to return result.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:80
> +    auto key = state.argument(1);

If we retain the argumentCount check, then this should be uncheckedArgument(1), because we’ve already determined that the argument count is not < 1 and it’s not 1, so it’s at least 2. If we get rid of the argumentCount check, then we’ll likely be checking for undefined at some point.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:82
> +    if (UNLIKELY(state.hadException()))
> +        return jsUndefined();

This hadException check is not needed; extracting an argument from ExecState cannot cause an exception.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:94
> +    if (UNLIKELY(state.hadException()))
> +        return jsUndefined();
> +
> +    return result;

The hadException check is not needed; the result will be ignored if an exception is raised; it’s OK to return result.
Comment 5 Brady Eidson 2015-10-31 15:26:48 PDT
(In reply to comment #4)
> Comment on attachment 264465 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264465&action=review
> 
> All the function arguments should be just JSValue instead of const JSValue&,
> because a JSValue is a small scalar and better to pass by value.

I did not know that about JSValue! Great!

> > Source/WebCore/Modules/indexeddb/IDBObjectStore.idl:37
> > +    [CallWith=ScriptExecutionContext, Custom, ImplementedAs=putRecord, RaisesException] IDBRequest put(any value, optional any key);
> 
> The ImplementedAs is awkward. I assume there's a name conflict, but I don’t
> understand why put has a conflict and add does not.

There's a name conflict with the JS object structures but only on put.

> > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:310
> > +        Vector<RefPtr<IDBKey>> result;
> 
> result.reserveInitialCapacity(array.size());

Yup.

> > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:312
> > +        for (size_t i = 0; i < array.size(); i++) {
> 
> Use a modern for loop here:
> 
>     for (auto& string : array) {

Yup.

> > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:316
> > +            result.append(key);
> 
> WTF::move(key)
> 
>     result.uncheckedAppend(WTF::move(key));

Cool.

> > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:318
> > +        return IDBKey::createArray(result);
> 
> WTF::move(result) maybe? Not sure it’s helpful.

Will look into it.

> > Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:59
> > +    size_t argsCount = state.argumentCount();
> > +    if (UNLIKELY(argsCount < 1))
> > +        return JSValue::decode(throwVMError(&state, createNotEnoughArgumentsError(&state)));
> 
> As I understand it, we should almost never be checking argument count.
> Modern DOM bindings check for undefined to see if an argument is missing,
> and never have to check the argument count itself.

While I'll trust you on the argsCount < 1 part of this, on the argsCount < 2 part it still seems necessary since it's an optional argument.

> 
> > Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:61
> > +    auto value = state.argument(0);
> 
> If we retain the argumentCount check, then this should be
> uncheckedArgument(0). If we get rid of it, then we’ll likely be checking for
> undefined at some point.
> 
> > Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:63
> > +    if (UNLIKELY(state.hadException()))
> > +        return jsUndefined();
> 
> This hadException check is not needed; extracting an argument from ExecState
> cannot cause an exception.

I pulled that from auto-gen code, but it appears it is only relevant WRT making Deprecated::ScriptValues in the VM.

Will remove!

> 
> > Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:72
> > +        if (overwrite)
> > +            result = toJS(&state, castedThis->globalObject(), WTF::getPtr(impl.put(state, value, ec)));
> > +        else
> > +            result = toJS(&state, castedThis->globalObject(), WTF::getPtr(impl.add(state, value, ec)));
> 
> Hand written code should not need to use WTF::getPtr. Instead we can either
> put in a .get() or not depending on the specific type. The automatically
> generated code uses getPtr to be generic, but that’s not important for
> hand-written code.

Totally makes sense (You can probably tell by now I started with the auto-gen'ed impl)

> > Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:77
> > +        if (UNLIKELY(state.hadException()))
> > +            return jsUndefined();
> > +        return result;
> 
> The hadException check is not needed; the result will be ignored if an
> exception is raised; it’s OK to return result.

Yup.

> > Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:80
> > +    auto key = state.argument(1);
> 
> If we retain the argumentCount check, then this should be
> uncheckedArgument(1), because we’ve already determined that the argument
> count is not < 1 and it’s not 1, so it’s at least 2. If we get rid of the
> argumentCount check, then we’ll likely be checking for undefined at some
> point.

Makes sense.

> 
> > Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:82
> > +    if (UNLIKELY(state.hadException()))
> > +        return jsUndefined();
> 
> This hadException check is not needed; extracting an argument from ExecState
> cannot cause an exception.
> 
> > Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:94
> > +    if (UNLIKELY(state.hadException()))
> > +        return jsUndefined();
> > +
> > +    return result;
> 
> The hadException check is not needed; the result will be ignored if an
> exception is raised; it’s OK to return result.

Got it and got it!
Comment 6 Brady Eidson 2015-10-31 20:47:00 PDT
Created attachment 264501 [details]
Patch for landing.
Comment 7 WebKit Commit Bot 2015-10-31 21:35:41 PDT
Comment on attachment 264501 [details]
Patch for landing.

Clearing flags on attachment: 264501

Committed r191850: <http://trac.webkit.org/changeset/191850>