bindings: Add validation for enum Sequence or Array

bindings: Add validation for enum Sequence or Array

We should throw a TypeError when an invalid enum value is passed
as an element of Sequence or Array.

BUG=471967
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192987

[bashi, 2015-03-31 04:32:25]

bashi@chromium.org changed reviewers:
+ haraken@chromium.org

[bashi, 2015-03-31 04:32:25]

What do you think about this fix? This is not an ideal fix but should work.
Better approach would be to generate V8NativeValueTraits::nativeValue() for each
enum types. However, it requires bunch of code generator changes (collecting all
enums from all IDL files, adding new code generator like CodeGeneratorEnum in
code_generator_v8.py, etc)

I'll update layout tests when this seems reasonable.

[haraken, 2015-03-31 06:11:17]

The approach looks good.

https://codereview.chromium.org/1047993002/diff/1/Source/bindings/templates/a...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/1047993002/diff/1/Source/bindings/templates/a...
Source/bindings/templates/attributes.cpp:337: if (!isValidEnum(cppValue,
validValues, {{attribute.enum_values|length}})) {

Space around '|'?

https://codereview.chromium.org/1047993002/diff/1/Source/bindings/templates/c...
File Source/bindings/templates/conversions.cpp (right):

https://codereview.chromium.org/1047993002/diff/1/Source/bindings/templates/c...
Source/bindings/templates/conversions.cpp:35: const char* validValues[] = {

Add static?

https://codereview.chromium.org/1047993002/diff/1/Source/bindings/templates/d...
File Source/bindings/templates/dictionary_v8.cpp (right):

https://codereview.chromium.org/1047993002/diff/1/Source/bindings/templates/d...
Source/bindings/templates/dictionary_v8.cpp:68: if
(!isValidEnum({{member.name}}, validValues, {{member.enum_values|length}})) {

Space around '|'?

https://codereview.chromium.org/1047993002/diff/1/Source/bindings/templates/m...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/1047993002/diff/1/Source/bindings/templates/m...
Source/bindings/templates/methods.cpp:202: if (!isValidEnum({{argument.name}},
validValues, {{argument.enum_values|length}})) {

Space around '|'?

https://codereview.chromium.org/1047993002/diff/1/Source/bindings/tests/resul...
File Source/bindings/tests/results/core/UnionTypesCore.cpp (right):

https://codereview.chromium.org/1047993002/diff/1/Source/bindings/tests/resul...
Source/bindings/tests/results/core/UnionTypesCore.cpp:767: if
(!isValidEnum(value, validValues, 4)) {

Or you can use WTF_ARRAY_LENGTH(validValues).

https://codereview.chromium.org/1047993002/diff/1/Source/bindings/tests/resul...
File Source/bindings/tests/results/core/V8TestObject.cpp (right):

https://codereview.chromium.org/1047993002/diff/1/Source/bindings/tests/resul...
Source/bindings/tests/results/core/V8TestObject.cpp:1733:
currentExecutionContext(info.GetIsolate())->addConsoleMessage(ConsoleMessage::create(JSMessageSource,
WarningMessageLevel, "The provided value is not a valid value of type
'TestEnum'."));

It's a shame that we lose the |string| information from the error message. Can
we add it back somehow?

[bashi, 2015-04-01 00:17:31]

Thanks!

https://codereview.chromium.org/1047993002/diff/1/Source/bindings/templates/c...
File Source/bindings/templates/conversions.cpp (right):

https://codereview.chromium.org/1047993002/diff/1/Source/bindings/templates/c...
Source/bindings/templates/conversions.cpp:35: const char* validValues[] = {
On 2015/03/31 06:11:16, haraken wrote:
> 
> Add static?

Done.

https://codereview.chromium.org/1047993002/diff/1/Source/bindings/tests/resul...
File Source/bindings/tests/results/core/UnionTypesCore.cpp (right):

https://codereview.chromium.org/1047993002/diff/1/Source/bindings/tests/resul...
Source/bindings/tests/results/core/UnionTypesCore.cpp:767: if
(!isValidEnum(value, validValues, 4)) {
On 2015/03/31 06:11:17, haraken wrote:
> 
> Or you can use WTF_ARRAY_LENGTH(validValues).

Done.

https://codereview.chromium.org/1047993002/diff/1/Source/bindings/tests/resul...
File Source/bindings/tests/results/core/V8TestObject.cpp (right):

https://codereview.chromium.org/1047993002/diff/1/Source/bindings/tests/resul...
Source/bindings/tests/results/core/V8TestObject.cpp:1733:
currentExecutionContext(info.GetIsolate())->addConsoleMessage(ConsoleMessage::create(JSMessageSource,
WarningMessageLevel, "The provided value is not a valid value of type
'TestEnum'."));
On 2015/03/31 06:11:17, haraken wrote:
> 
> It's a shame that we lose the |string| information from the error message. Can
> we add it back somehow?

I dropped this because it requires a Vector<String> -> String conversion
function or a function which converts any V8 value to String. Added latter as
toCoreString.

[haraken, 2015-04-01 00:29:46]

V8StringResource has an implicit conversion to a WTF::String, so we might want
to use the mechanism instead of calling toCoreString().

https://codereview.chromium.org/1047993002/diff/20001/Source/bindings/tests/r...
File Source/bindings/tests/results/core/V8TestDictionary.cpp (right):

https://codereview.chromium.org/1047993002/diff/20001/Source/bindings/tests/r...
Source/bindings/tests/results/core/V8TestDictionary.cpp:140:
exceptionState.throwTypeError("member enumMember ('" + toCoreString(isolate,
enumMemberValue) + "') is not a valid enum value.");

Can we just use enumMember (not toCoreString(enumMemberValue))?

https://codereview.chromium.org/1047993002/diff/20001/Source/bindings/tests/r...
File Source/bindings/tests/results/core/V8TestInterface.cpp (right):

https://codereview.chromium.org/1047993002/diff/20001/Source/bindings/tests/r...
Source/bindings/tests/results/core/V8TestInterface.cpp:291:
currentExecutionContext(info.GetIsolate())->addConsoleMessage(ConsoleMessage::create(JSMessageSource,
WarningMessageLevel, "The provided value '" + toCoreString(info.GetIsolate(),
v8Value) + "' is not a valid value of type 'TestEnum'."));

Ditto. Can we use cppValue instead of toCoreString(v8Value)?

[bashi, 2015-04-01 00:50:57]

> V8StringResource has an implicit conversion to a WTF::String, so we might want
> to use the mechanism instead of calling toCoreString().

Using implicit conversion makes binding code complicated (as described below).

https://codereview.chromium.org/1047993002/diff/20001/Source/bindings/tests/r...
File Source/bindings/tests/results/core/V8TestDictionary.cpp (right):

https://codereview.chromium.org/1047993002/diff/20001/Source/bindings/tests/r...
Source/bindings/tests/results/core/V8TestDictionary.cpp:140:
exceptionState.throwTypeError("member enumMember ('" + toCoreString(isolate,
enumMemberValue) + "') is not a valid enum value.");
On 2015/04/01 00:29:45, haraken wrote:
> 
> Can we just use enumMember (not toCoreString(enumMemberValue))?

I don't think we want to do it because it requires two branches in Jinja
templates. A member may be a string, but also can be a vector. That's why I use
toCoreString here.

Added a sequence<TestEnum> in TestDictionary.idl to explain this. (I thought I
added it, but it seems I didn't...)

https://codereview.chromium.org/1047993002/diff/20001/Source/bindings/tests/r...
File Source/bindings/tests/results/core/V8TestInterface.cpp (right):

https://codereview.chromium.org/1047993002/diff/20001/Source/bindings/tests/r...
Source/bindings/tests/results/core/V8TestInterface.cpp:291:
currentExecutionContext(info.GetIsolate())->addConsoleMessage(ConsoleMessage::create(JSMessageSource,
WarningMessageLevel, "The provided value '" + toCoreString(info.GetIsolate(),
v8Value) + "' is not a valid value of type 'TestEnum'."));
On 2015/04/01 00:29:45, haraken wrote:
> 
> Ditto. Can we use cppValue instead of toCoreString(v8Value)?

Ditto. An attribute can be a vector.

[haraken, 2015-04-01 01:27:25]

https://codereview.chromium.org/1047993002/diff/40001/Source/bindings/tests/r...
File Source/bindings/tests/results/core/V8TestDictionary.cpp (right):

https://codereview.chromium.org/1047993002/diff/40001/Source/bindings/tests/r...
Source/bindings/tests/results/core/V8TestDictionary.cpp:164:
exceptionState.throwTypeError("member enumSequenceMember ('" +
toCoreString(isolate, enumSequenceMemberValue) + "') is not a valid enum
value.");

Shall we move the for loop from isValidEnum to the caller side (although I agree
that it will complicate the template file a bit)? Then we can just print one
String that is judged not to be valid here, instead of all Strings including
valid ones. Also we won't need toCoreString().

What do you think?

[bashi, 2015-04-01 01:48:42]

On 2015/04/01 01:27:25, haraken wrote:
>
https://codereview.chromium.org/1047993002/diff/40001/Source/bindings/tests/r...
> File Source/bindings/tests/results/core/V8TestDictionary.cpp (right):
> 
>
https://codereview.chromium.org/1047993002/diff/40001/Source/bindings/tests/r...
> Source/bindings/tests/results/core/V8TestDictionary.cpp:164:
> exceptionState.throwTypeError("member enumSequenceMember ('" +
> toCoreString(isolate, enumSequenceMemberValue) + "') is not a valid enum
> value.");
> 
> Shall we move the for loop from isValidEnum to the caller side (although I
agree
> that it will complicate the template file a bit)? Then we can just print one
> String that is judged not to be valid here, instead of all Strings including
> valid ones. Also we won't need toCoreString().
> 
> What do you think?

The same thing can be said when we want to do that. It will make template files
even more complicated. To to that,
- we need to pass a flag to indicate the thing (attribute, dictionary member,
argument, whatever) in question is a string or a vector because we need to
different checks for strings and vectors.
- then, we need to have two branches (one for string, the other for vector)
everywhere we need to check enums (attributes, method arguments, dictionary,
union, etc).

[haraken, 2015-04-01 02:02:54]

On 2015/04/01 01:48:42, bashi1 wrote:
> On 2015/04/01 01:27:25, haraken wrote:
> >
>
https://codereview.chromium.org/1047993002/diff/40001/Source/bindings/tests/r...
> > File Source/bindings/tests/results/core/V8TestDictionary.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1047993002/diff/40001/Source/bindings/tests/r...
> > Source/bindings/tests/results/core/V8TestDictionary.cpp:164:
> > exceptionState.throwTypeError("member enumSequenceMember ('" +
> > toCoreString(isolate, enumSequenceMemberValue) + "') is not a valid enum
> > value.");
> > 
> > Shall we move the for loop from isValidEnum to the caller side (although I
> agree
> > that it will complicate the template file a bit)? Then we can just print one
> > String that is judged not to be valid here, instead of all Strings including
> > valid ones. Also we won't need toCoreString().
> > 
> > What do you think?
> 
> The same thing can be said when we want to do that. It will make template
files
> even more complicated. To to that,
> - we need to pass a flag to indicate the thing (attribute, dictionary member,
> argument, whatever) in question is a string or a vector because we need to
> different checks for strings and vectors.
> - then, we need to have two branches (one for string, the other for vector)
> everywhere we need to check enums (attributes, method arguments, dictionary,
> union, etc).

Ah, got it; thanks for the clarification. It seems your approach is a good
balancing point.

A slightly tidier way to implement the approach might be to pass in the
exceptionState to isValidEnum and let isValidEnum throw a type error. Then we
can print the exact invalid string for Vector<String>. Also we won't need
toCoreString().

[bashi, 2015-04-01 02:19:31]

On 2015/04/01 02:02:54, haraken wrote:
> On 2015/04/01 01:48:42, bashi1 wrote:
> > On 2015/04/01 01:27:25, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/1047993002/diff/40001/Source/bindings/tests/r...
> > > File Source/bindings/tests/results/core/V8TestDictionary.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1047993002/diff/40001/Source/bindings/tests/r...
> > > Source/bindings/tests/results/core/V8TestDictionary.cpp:164:
> > > exceptionState.throwTypeError("member enumSequenceMember ('" +
> > > toCoreString(isolate, enumSequenceMemberValue) + "') is not a valid enum
> > > value.");
> > > 
> > > Shall we move the for loop from isValidEnum to the caller side (although I
> > agree
> > > that it will complicate the template file a bit)? Then we can just print
one
> > > String that is judged not to be valid here, instead of all Strings
including
> > > valid ones. Also we won't need toCoreString().
> > > 
> > > What do you think?
> > 
> > The same thing can be said when we want to do that. It will make template
> files
> > even more complicated. To to that,
> > - we need to pass a flag to indicate the thing (attribute, dictionary
member,
> > argument, whatever) in question is a string or a vector because we need to
> > different checks for strings and vectors.
> > - then, we need to have two branches (one for string, the other for vector)
> > everywhere we need to check enums (attributes, method arguments, dictionary,
> > union, etc).
> 
> Ah, got it; thanks for the clarification. It seems your approach is a good
> balancing point.
> 
> A slightly tidier way to implement the approach might be to pass in the
> exceptionState to isValidEnum and let isValidEnum throw a type error. Then we
> can print the exact invalid string for Vector<String>. Also we won't need
> toCoreString().

It sounds good, but here is a pitfall. We shouldn't throw a type error when we
are handling attribute setter.
http://heycam.github.io/webidl/#idl-enums

So, on attribute setter, code will look like:

if (!isValidEnum(..., exceptionState)) {
  addConsoleMessage(..., exceptionState.message());
  exceptionState.clearException();
  return;
}

Does this sound reasonable? Will try this if it's fine.

[haraken, 2015-04-01 02:25:00]

On 2015/04/01 02:19:31, bashi1 wrote:
> On 2015/04/01 02:02:54, haraken wrote:
> > On 2015/04/01 01:48:42, bashi1 wrote:
> > > On 2015/04/01 01:27:25, haraken wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1047993002/diff/40001/Source/bindings/tests/r...
> > > > File Source/bindings/tests/results/core/V8TestDictionary.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1047993002/diff/40001/Source/bindings/tests/r...
> > > > Source/bindings/tests/results/core/V8TestDictionary.cpp:164:
> > > > exceptionState.throwTypeError("member enumSequenceMember ('" +
> > > > toCoreString(isolate, enumSequenceMemberValue) + "') is not a valid enum
> > > > value.");
> > > > 
> > > > Shall we move the for loop from isValidEnum to the caller side (although
I
> > > agree
> > > > that it will complicate the template file a bit)? Then we can just print
> one
> > > > String that is judged not to be valid here, instead of all Strings
> including
> > > > valid ones. Also we won't need toCoreString().
> > > > 
> > > > What do you think?
> > > 
> > > The same thing can be said when we want to do that. It will make template
> > files
> > > even more complicated. To to that,
> > > - we need to pass a flag to indicate the thing (attribute, dictionary
> member,
> > > argument, whatever) in question is a string or a vector because we need to
> > > different checks for strings and vectors.
> > > - then, we need to have two branches (one for string, the other for
vector)
> > > everywhere we need to check enums (attributes, method arguments,
dictionary,
> > > union, etc).
> > 
> > Ah, got it; thanks for the clarification. It seems your approach is a good
> > balancing point.
> > 
> > A slightly tidier way to implement the approach might be to pass in the
> > exceptionState to isValidEnum and let isValidEnum throw a type error. Then
we
> > can print the exact invalid string for Vector<String>. Also we won't need
> > toCoreString().
> 
> It sounds good, but here is a pitfall. We shouldn't throw a type error when we
> are handling attribute setter.
> http://heycam.github.io/webidl/#idl-enums
> 
> So, on attribute setter, code will look like:
> 
> if (!isValidEnum(..., exceptionState)) {
>   addConsoleMessage(..., exceptionState.message());
>   exceptionState.clearException();
>   return;
> }
> 
> Does this sound reasonable? Will try this if it's fine.

I personally think that it is reasonable, because the setter wants to ignore the
exception and the above code is doing exactly what the setter should do.

I guess you won't need clearException though. You can just ignore the exception.

[bashi, 2015-04-01 03:08:30]

Done. Using exceptionState without adding extra handling also drops some
information. If this looks OK, I'll update all test expectations.

[haraken, 2015-04-01 03:38:03]

LGTM

[bashi, 2015-04-01 08:25:34]

Updated test expectations.

https://codereview.chromium.org/1047993002/diff/100001/Source/bindings/tests/...
File Source/bindings/tests/results/core/V8TestInterface.cpp (right):

https://codereview.chromium.org/1047993002/diff/100001/Source/bindings/tests/...
Source/bindings/tests/results/core/V8TestInterface.cpp:280: ExceptionState
exceptionState(ExceptionState::SetterContext, "testEnumAttribute",
"TestInterface", holder, info.GetIsolate());
Changed to use actual ExceptionState.

[haraken, 2015-04-01 08:28:33]

On 2015/04/01 08:25:34, bashi1 wrote:
> Updated test expectations.
> 
>
https://codereview.chromium.org/1047993002/diff/100001/Source/bindings/tests/...
> File Source/bindings/tests/results/core/V8TestInterface.cpp (right):
> 
>
https://codereview.chromium.org/1047993002/diff/100001/Source/bindings/tests/...
> Source/bindings/tests/results/core/V8TestInterface.cpp:280: ExceptionState
> exceptionState(ExceptionState::SetterContext, "testEnumAttribute",
> "TestInterface", holder, info.GetIsolate());
> Changed to use actual ExceptionState.

LGTM

[bashi, 2015-04-02 01:27:25]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2015-04-02 01:28:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1047993002/100001

[commit-bot: I haz the power, 2015-04-02 02:39:04]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192987