Bug 50081 – Clean up IDBDatabase.transaction and add checks to IDBTransaction.objectStore

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeremy Orlow]

Clean up IDBDatabase.transaction and add checks to IDBTransaction.objectStore

[Jeremy Orlow]

Created attachment 74891 [details]
Patch

[Hans Wennborg]

Comment on attachment 74891 [details]
Patch

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

> WebCore/bindings/v8/OptionsObject.cpp:30
> +#include <limits>

I think this should come last in the list of includes?

> WebCore/bindings/v8/OptionsObject.cpp:96
> +String OptionsObject::getKeyString(const String& key) const

Are we sure we don't want to make this return a bool that can be false to signal an option not being found, like the bool and integer getKey functions?

> WebCore/storage/IDBDatabase.cpp:29
> +#include <limits>

I think this should be last.

[Jeremy Orlow]

(In reply to comment #2)
> (From update of attachment 74891 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74891&action=review
> 
> > WebCore/bindings/v8/OptionsObject.cpp:30
> > +#include <limits>
> 
> I think this should come last in the list of includes?
> 
> > WebCore/bindings/v8/OptionsObject.cpp:96
> > +String OptionsObject::getKeyString(const String& key) const
> 
> Are we sure we don't want to make this return a bool that can be false to signal an option not being found, like the bool and integer getKey functions?

I went back and forth, but I found the usage of this cleaner.  If something cares to distinguish between a key being null and/or undefined vs a string, they should probably do that themself anyway.

I think the ideal would actually be for primitives to be boxed in some sort of object so I can return a null object to signal nothing.  But since not, I somewhat think this is best....but like I said, I went back and forth.

> > WebCore/storage/IDBDatabase.cpp:29
> > +#include <limits>
> 
> I think this should be last.

I'll try it, but I think the style bot was yelling at me.

[Jeremy Orlow]

Comment on attachment 74891 [details]
Patch

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

> WebCore/bindings/v8/OptionsObject.cpp:108
> +RefPtr<DOMStringList> OptionsObject::getKeyDOMStringList(const String& key) const

Should be passRefPtr

[Jeremy Orlow]

Created attachment 74912 [details]
Patch

[Steve Block]

Comment on attachment 74912 [details]
Patch

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

> WebCore/bindings/v8/OptionsObject.cpp:76
> +bool OptionsObject::getKeyInteger(const String& key, int64_t& value) const

Does JSC have an equivalent Integer type? If not, you might want to call this getKeyInt64 for compatibility.

> WebCore/bindings/v8/OptionsObject.cpp:82
> +    if (!v8Value->IsNumber())

Shouldn't you be testing for int64/Integer, rather than Number?

> WebCore/bindings/v8/OptionsObject.cpp:96
> +String OptionsObject::getKeyString(const String& key) const

I agree with Hans that this should return a boolean and have the string as an out param. I think that wanting to distinguish between a non-string object and an empty string is a common use case.

At the very least, if you want the API to return a default value (here the empty string) if the type is incorrect, we should be consistent in doing so for all methods.

> WebCore/bindings/v8/OptionsObject.h:49
> +    PassRefPtr<DOMStringList> getKeyDOMStringList(const String& key) const;

It looks like all of these methods don't do any coercion and will fail if the property doesn't have the expected type. This isn't what I'd expect for JS, so might be worth a comment.

> WebCore/storage/IDBDatabase.cpp:44
> +const unsigned long defaultTimeout = 0; // Infinite.

I think the accepted style is to avoid 'magic values' like this. See https://bugs.webkit.org/show_bug.cgi?id=27254#c5

[Andrei Popescu]

> result = evalAndLog("webkitIndexedDB.open('name', 'description')");

No need to pass description anymore.

>  if (!v8Value->IsNumber())

I wonder if we should just let the VM give us a number instead? You could do:

v8::Local<v8::Number> number = v8value->ToNumber();
if (number.IsEmpty()) {
  // Could not get a number out of the JS value.
  return false;
}


Same for the other methods?

[Andrei Popescu]

(In reply to comment #7)
> > result = evalAndLog("webkitIndexedDB.open('name', 'description')");
> 
> No need to pass description anymore.
> 
> >  if (!v8Value->IsNumber())
> 
> I wonder if we should just let the VM give us a number instead? You could do:
> 
> v8::Local<v8::Number> number = v8value->ToNumber();
> if (number.IsEmpty()) {
>   // Could not get a number out of the JS value.
>   return false;
> }
> 
> 
> Same for the other methods?

To clarify, looking at the spec, I don't think the language in the spec document calls for this strict type checking. 

Also, we had a similar discussion for Geolocation and here's what Darin had to say about the same issue:

https://bugs.webkit.org/show_bug.cgi?id=27254#c7

[Jeremy Orlow]

(In reply to comment #6)
> (From update of attachment 74912 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74912&action=review
> 
> > WebCore/bindings/v8/OptionsObject.cpp:76
> > +bool OptionsObject::getKeyInteger(const String& key, int64_t& value) const
> 
> Does JSC have an equivalent Integer type? If not, you might want to call this getKeyInt64 for compatibility.

Apparently not.

> > WebCore/bindings/v8/OptionsObject.cpp:82
> > +    if (!v8Value->IsNumber())
> 
> Shouldn't you be testing for int64/Integer, rather than Number?

There is no such thing.  I guess the portable solution is probably to just extract the double and do all the rest of the work myself.

> > WebCore/bindings/v8/OptionsObject.cpp:96
> > +String OptionsObject::getKeyString(const String& key) const
> 
> I agree with Hans that this should return a boolean and have the string as an out param. I think that wanting to distinguish between a non-string object and an empty string is a common use case.

Not sure I agree, but OK, I"ll do it.

> At the very least, if you want the API to return a default value (here the empty string) if the type is incorrect, we should be consistent in doing so for all methods.

I think this would be the ideal, but it's clearly not possible for non pointer or string types.

> > WebCore/bindings/v8/OptionsObject.h:49
> > +    PassRefPtr<DOMStringList> getKeyDOMStringList(const String& key) const;
> 
> It looks like all of these methods don't do any coercion and will fail if the property doesn't have the expected type. This isn't what I'd expect for JS, so might be worth a comment.

In what cases do you think we're doing the wrong thing?  I think null/undefined should be treated as no value present.  Doing something like {unique: someObject} evaluating to true rather than the default doesn't seem that useful.  I did think about this, but I couldn't think of a case where coercion is better than what I have here.

> > WebCore/storage/IDBDatabase.cpp:44
> > +const unsigned long defaultTimeout = 0; // Infinite.
> 
> I think the accepted style is to avoid 'magic values' like this. See https://bugs.webkit.org/show_bug.cgi?id=27254#c5

This is not a change.  Just moving what was there before.  There's a lot of existing code that uses this and the spec doesn't actually say (as far as I can tell) how to specify "infinite" so I'd like to punt this until we figure it out at the spec side.  I can add a FIXME.



Please do review the others while we nail this down as none of these issues should prohibit a review.

[Jeremy Orlow]

(In reply to comment #9)
> (In reply to comment #6)
> > (From update of attachment 74912 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=74912&action=review
> > 
> > > WebCore/bindings/v8/OptionsObject.cpp:76
> > > +bool OptionsObject::getKeyInteger(const String& key, int64_t& value) const
> > 
> > Does JSC have an equivalent Integer type? If not, you might want to call this getKeyInt64 for compatibility.
> 
> Apparently not.

Switched things over to using int32, which both support and which should meet the needs for now.  In the future, timeout should use a double.  I've added a FIXME for this.
 
> > > WebCore/bindings/v8/OptionsObject.cpp:82
> > > +    if (!v8Value->IsNumber())
> > 
> > Shouldn't you be testing for int64/Integer, rather than Number?
> 
> There is no such thing.  I guess the portable solution is probably to just extract the double and do all the rest of the work myself.
> 
> > > WebCore/bindings/v8/OptionsObject.cpp:96
> > > +String OptionsObject::getKeyString(const String& key) const
> > 
> > I agree with Hans that this should return a boolean and have the string as an out param. I think that wanting to distinguish between a non-string object and an empty string is a common use case.
> 
> Not sure I agree, but OK, I"ll do it.

done

> > At the very least, if you want the API to return a default value (here the empty string) if the type is incorrect, we should be consistent in doing so for all methods.
> 
> I think this would be the ideal, but it's clearly not possible for non pointer or string types.
> 
> > > WebCore/bindings/v8/OptionsObject.h:49
> > > +    PassRefPtr<DOMStringList> getKeyDOMStringList(const String& key) const;
> > 
> > It looks like all of these methods don't do any coercion and will fail if the property doesn't have the expected type. This isn't what I'd expect for JS, so might be worth a comment.
> 
> In what cases do you think we're doing the wrong thing?  I think null/undefined should be treated as no value present.  Doing something like {unique: someObject} evaluating to true rather than the default doesn't seem that useful.  I did think about this, but I couldn't think of a case where coercion is better than what I have here.

As discussed, yeah, this is the right thing to do.  Made much more liberal.

> > > WebCore/storage/IDBDatabase.cpp:44
> > > +const unsigned long defaultTimeout = 0; // Infinite.
> > 
> > I think the accepted style is to avoid 'magic values' like this. See https://bugs.webkit.org/show_bug.cgi?id=27254#c5
> 
> This is not a change.  Just moving what was there before.  There's a lot of existing code that uses this and the spec doesn't actually say (as far as I can tell) how to specify "infinite" so I'd like to punt this until we figure it out at the spec side.  I can add a FIXME.

added

[Jeremy Orlow]

Created attachment 74936 [details]
Patch

[Jeremy Orlow]

Created attachment 74937 [details]
diff between patch 2 and 3

[Andrei Popescu]

LGTM

[Eric Seidel (no email)]

Attachment 74936 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6357066

[Steve Block]

Comment on attachment 74936 [details]
Patch

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

In getKey(), are you sure that Has(key) will return false if the caller explicitly passes undefined? Geolocation checks for an undefined value even if Has(key)/Get(key) returns a non-empty value. It looks like one of the two cases is wrong.

> WebCore/bindings/v8/OptionsObject.cpp:68
>      ASSERT(!v8Bool.IsEmpty());

I don't think this is safe. If the JavaScript property's valueOf() method throws an exception, v8Bool will be empty. See fast/dom/Geolocation/script-tests/argument-types.js.

On a similar note, are you sure that calling Has(key) then ASSERT(!Get(key).IsEmpty()) is safe in getKey() in the case where the options object's property getter throws?

> WebCore/bindings/v8/OptionsObject.cpp:109
> +            return 0;

Shouldn't you just call v8ValueToWebCoreString(), as in getKeyString(), rather than checking IsString()?

[Jeremy Orlow]

(In reply to comment #15)
> (From update of attachment 74936 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74936&action=review
> 
> In getKey(), are you sure that Has(key) will return false if the caller explicitly passes undefined? Geolocation checks for an undefined value even if Has(key)/Get(key) returns a non-empty value. It looks like one of the two cases is wrong.

I don't know for sure, and it's not easy to look at the code.  Will add extra check and fixme to look into it.
 
> > WebCore/bindings/v8/OptionsObject.cpp:68
> >      ASSERT(!v8Bool.IsEmpty());
> 
> I don't think this is safe. If the JavaScript property's valueOf() method throws an exception, v8Bool will be empty. See fast/dom/Geolocation/script-tests/argument-types.js.

made these if, return false's.

> On a similar note, are you sure that calling Has(key) then ASSERT(!Get(key).IsEmpty()) is safe in getKey() in the case where the options object's property getter throws?

no, will make it more conservative.

> > WebCore/bindings/v8/OptionsObject.cpp:109
> > +            return 0;
> 
> Shouldn't you just call v8ValueToWebCoreString(), as in getKeyString(), rather than checking IsString()?

done

[Jeremy Orlow]

Created attachment 74940 [details]
Patch

[Jeremy Orlow]

Created attachment 74942 [details]
Patch

[Jeremy Orlow]

Committed r72765: <http://trac.webkit.org/changeset/72765>