RESOLVED FIXED 158302
Creating a TouchList with non Touch items should throw an exception
https://bugs.webkit.org/show_bug.cgi?id=158302
Summary Creating a TouchList with non Touch items should throw an exception
Nael Ouedraogo
Reported 2016-06-02 05:41:34 PDT
Creating a TouchList with non Touch items should throw an exception.
Attachments
Patch (7.11 KB, patch)
2016-06-02 06:34 PDT, Nael Ouedraogo
no flags
Patch (9.17 KB, patch)
2016-06-02 08:41 PDT, Nael Ouedraogo
no flags
Patch (9.77 KB, patch)
2016-06-06 09:48 PDT, Nael Ouedraogo
no flags
Patch (9.71 KB, patch)
2016-06-07 01:28 PDT, Nael Ouedraogo
no flags
Nael Ouedraogo
Comment 1 2016-06-02 06:34:02 PDT
Nael Ouedraogo
Comment 2 2016-06-02 08:41:50 PDT
Nael Ouedraogo
Comment 3 2016-06-02 08:49:29 PDT
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 4 2016-06-02 09:25:50 PDT
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 5 2016-06-02 09:29:24 PDT
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 6 2016-06-02 09:31:03 PDT
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 7 2016-06-02 09:34:26 PDT
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 8 2016-06-02 09:37:09 PDT
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
Comment 9 2016-06-06 09:48:03 PDT
Nael Ouedraogo
Comment 10 2016-06-06 09:52:53 PDT
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 11 2016-06-06 12:19:31 PDT
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
Comment 12 2016-06-07 01:28:37 PDT
youenn fablet
Comment 13 2016-06-07 01:59:32 PDT
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
Comment 14 2016-06-07 02:41:27 PDT
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 15 2016-06-07 03:05:27 PDT
Comment on attachment 280683 [details] Patch Clearing flags on attachment: 280683 Committed r201747: <http://trac.webkit.org/changeset/201747>
WebKit Commit Bot
Comment 16 2016-06-07 03:05:34 PDT
All reviewed patches have been landed. Closing bug.
Chris Dumez
Comment 17 2016-06-07 08:58:51 PDT
Comment on attachment 280683 [details] Patch Darn, Youenn stole my review :) Anyway, belated lgtm.
Darin Adler
Comment 18 2016-06-09 20:31:36 PDT
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 19 2016-06-09 20:32:33 PDT
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 20 2016-06-09 20:34:20 PDT
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
Comment 21 2016-06-10 08:58:20 PDT
For a regular binding only NewObject attribute is needed for createTouchList. Thus to remove the custom binding we need just to delete "Custom" word.
Note You need to log in before you can comment on or make changes to this bug.