Bug 158302 – Creating a TouchList with non Touch items should throw an exception

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nael Ouedraogo]

Creating a TouchList with non Touch items should throw an exception.

[Nael Ouedraogo]

Created attachment 280330 [details]
Patch

[Nael Ouedraogo]

Created attachment 280331 [details]
Patch

[Nael Ouedraogo]

This patch would also permit to fix an issue related to a discussion in bug 157985. It will make it possible to pass a Ref&& instead of RefPtr&& in TouchList::append().

[Chris Dumez]

Comment on attachment 280331 [details]
Patch

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

> Source/WebCore/bindings/js/JSDocumentCustom.cpp:136
> +    for (size_t i = 0; i < state.argumentCount(); i++) {

++i

> Source/WebCore/bindings/js/JSDocumentCustom.cpp:143
>      return toJS(&state, globalObject(), touchList);

return toJSNewlyCreated(&state, globalObject(), WTFMove(touchList));

> LayoutTests/fast/events/touch/script-tests/document-create-touch-list-crash.js:19
>  

Can you add a test to make sure we do not throw if createTouchList() is called without parameter and no make sure that the list length is 0?

[Chris Dumez]

Comment on attachment 280331 [details]
Patch

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

> LayoutTests/fast/events/touch/script-tests/document-create-touch-list-crash.js:8
> +shouldThrow('document.createTouchList(undefined).item(0)');

I don't believe document.createTouchList(undefined) should throw. A variadic parameter is considered optional in Web IDL. So passing undefined should be equivalent to not passing any parameter in this case (since there is no default value for the parameter).

[Chris Dumez]

Comment on attachment 280331 [details]
Patch

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

>> LayoutTests/fast/events/touch/script-tests/document-create-touch-list-crash.js:8
>> +shouldThrow('document.createTouchList(undefined).item(0)');
> 
> I don't believe document.createTouchList(undefined) should throw. A variadic parameter is considered optional in Web IDL. So passing undefined should be equivalent to not passing any parameter in this case (since there is no default value for the parameter).

See:
https://heycam.github.io/webidl/#es-overloads (step 10.4)

Also from the IDL spec:
An argument is considered to be an optional argument if it is declared with the optional keyword. The final argument of a variadic operation is also considered to be an optional argument. Declaring an argument to be optional indicates that the argument value can be omitted when the operation is invoked. The final argument in an operation must not explicitly be declared to be optional if the operation is variadic.

[Chris Dumez]

Comment on attachment 280331 [details]
Patch

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

> Source/WebCore/bindings/js/JSDocumentCustom.cpp:137
> +        auto* item = JSTouch::toWrapped(state.argument(i));

Should be state.uncheckedArgument(i)

[Chris Dumez]

Comment on attachment 280331 [details]
Patch

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

>> Source/WebCore/bindings/js/JSDocumentCustom.cpp:137
>> +        auto* item = JSTouch::toWrapped(state.argument(i));
> 
> Should be state.uncheckedArgument(i)

Maybe something like this:
JSValue argument = state.uncheckedArgument(I);
If (argument.isUndefined())
    continue;
auto* touch = JSTouch::toWrapped(argument);
...

[Nael Ouedraogo]

Created attachment 280602 [details]
Patch

[Nael Ouedraogo]

Thanks for the review.

Ok I modified tests in uploaded patch. It is checked that no exception is thrown when createTouchList() is called without parameter. The list length is also checked.

Concerning support of "undefined" arguments, it seems that they should be converted to the type of the variadic as would be done for non-optional arguments (c.f. http://heycam.github.io/webidl/#es-overloads. step 16. last sentence of the note).
Conversion of Touch objects from undefined should thus throw a TypeError exception as per section 4.2.19 (http://heycam.github.io/webidl/#es-object).

I also tried to remove the custom binding since probably not necessary here but a compilation error is observed in GObject binding code.

[Chris Dumez]

Comment on attachment 280602 [details]
Patch

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

> Source/WebCore/bindings/js/JSDocumentCustom.cpp:137
> +        if (!state.uncheckedArgument(i).inherits(JSTouch::info()))

Why not null check the value returned by JSTouch::toWrapped() instead? JSTouch::toWrapped() already does a inherits(JSTouch::info()) check internally.

> Source/WebCore/bindings/js/JSDocumentCustom.cpp:138
> +            return JSValue::decode(throwArgumentTypeError(state, i, "touches", "Document", "createTouchList", "TouchList"));

The last parameter should probably be "Touch".

> Source/WebCore/bindings/js/JSDocumentCustom.cpp:140
> +        auto* item = JSTouch::toWrapped(state.argument(i));

uncheckedArgument(i)

> Source/WebCore/bindings/js/JSDocumentCustom.cpp:142
> +        touchList->append(item);

In a follow-up patch, we should update append() to take a reference instead of a raw pointer.

> LayoutTests/fast/events/touch/document-create-touch-list-crash-expected.txt:14
> +PASS document.createTouchList(t, document, t2); threw exception TypeError: Argument 2 ('touches') to Document.createTouchList must be an instance of TouchList.

The message should probably says "... Must be an instance of Touch.", not TouchList.

[Nael Ouedraogo]

Created attachment 280683 [details]
Patch

[youenn fablet]

Comment on attachment 280683 [details]
Patch

r=me
Might be worth looking at updating the binding generated code for variadics and applying it for touch lists and more.

[Nael Ouedraogo]

Thanks for reviews. 

I made the corrections suggested by Chris in the uploaded patch. 
I file also a new bug (bug 158469) to pass a Ref instead of RefPtr in TouchList::append() method.

[WebKit Commit Bot]

Comment on attachment 280683 [details]
Patch

Clearing flags on attachment: 280683

Committed r201747: <http://trac.webkit.org/changeset/201747>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Comment on attachment 280683 [details]
Patch

Darn, Youenn stole my review :) Anyway, belated lgtm.

[Darin Adler]

Comment on attachment 280683 [details]
Patch

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

> Source/WebCore/bindings/js/JSDocumentCustom.cpp:143
>      auto touchList = TouchList::create();
>  
> -    for (size_t i = 0; i < state.argumentCount(); i++)
> -        touchList->append(JSTouch::toWrapped(state.argument(i)));
> +    for (size_t i = 0; i < state.argumentCount(); ++i) {
> +        auto* item = JSTouch::toWrapped(state.uncheckedArgument(i));
> +        if (!item)
> +            return JSValue::decode(throwArgumentTypeError(state, i, "touches", "Document", "createTouchList", "Touch"));
>  
> -    return toJS(&state, globalObject(), touchList);
> +        touchList->append(item);
> +    }
> +    return toJSNewlyCreated(&state, globalObject(), WTFMove(touchList));

Paragraphing here is a little peculiar. If there’s a space before the loop, then I suggest a space after the loop too.

> Source/WebCore/dom/Document.idl:304
> -    [NewObject, Custom, RaisesException] TouchList createTouchList();
> +    [NewObject, Custom] TouchList createTouchList(Touch... touches);

What was the rationale for removing RaisesException?

[Darin Adler]

Comment on attachment 280683 [details]
Patch

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

>> Source/WebCore/dom/Document.idl:304
>> +    [NewObject, Custom] TouchList createTouchList(Touch... touches);
> 
> What was the rationale for removing RaisesException?

Oh, I understand now. The only exception that can occur is in argument handling, not in the function itself.

[Chris Dumez]

Comment on attachment 280683 [details]
Patch

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

>>> Source/WebCore/dom/Document.idl:304
>>> +    [NewObject, Custom] TouchList createTouchList(Touch... touches);
>> 
>> What was the rationale for removing RaisesException?
> 
> Oh, I understand now. The only exception that can occur is in argument handling, not in the function itself.

Well, it is marked as [Custom] so [NewObject] and [RaisesException] have no impact anyway.

[Nael Ouedraogo]

For a regular binding only NewObject attribute is needed for createTouchList. 
Thus to remove the custom binding we need just to delete "Custom" word.