The TypedArray specification was just updated to remove references to the DOM INDEX_SIZE_ERR exception when certain out-of-range accesses are performed. The WebKit implementation needs to be updated to throw RangeError (most likely), and array-unit-tests.html updated on the Khronos site and synced back down to WebKit to no longer expect INDEX_SIZE_ERR to be thrown.
Currently, there is a mix of dom INDEX_SIZE_ERR exceptions, and of RangeError. For example, if you build a typed array from a too large object (array of typed array), a dom INDEX_SIZE_ERR is thrown. But if you build a too typed array from a too large number, a RangeError is thrown. Another inconsistency: when passing invalid arguments to set method, a dom SYNTAX_ERR exception is thrown in v8. But in JavasScriptCore, it's a JavaScript SyntaxError. On the other browsers I tested (firefox and opera), javascript exception are thrown (Error for firefox, TypeError for opera).
Created attachment 156183 [details] patch proposal: change INDEX_SIZE_ERR to RangeError and throws TypeError when calling set with invalid arguments
Comment on attachment 156183 [details] patch proposal: change INDEX_SIZE_ERR to RangeError and throws TypeError when calling set with invalid arguments View in context: https://bugs.webkit.org/attachment.cgi?id=156183&action=review > Source/WebCore/ChangeLog:11 > + Update TypedArrays to throw JavaScript RangeError instead of dom > + INDEX_SIZE_ERR exceptions. Also, update TypedArrays to throw TypeError > + instead of JavaScript SyntaxError or dom exceptions SYNTAX_ERR when > + calling set method with invalid arguments. Would you add the spec link to the ChangeLog? > Source/WebCore/bindings/js/JSArrayBufferViewHelper.h:129 > + return JSC::throwTypeError(exec); Let's output some descriptive message. > Source/WebCore/bindings/js/JSArrayBufferViewHelper.h:156 > + return JSC::throwTypeError(exec); Ditto. > Source/WebCore/bindings/v8/custom/V8ArrayBufferViewCustom.h:208 > + return V8Proxy::throwTypeError(0, args.GetIsolate()); Ditto. > Source/WebCore/bindings/v8/custom/V8ArrayBufferViewCustom.h:244 > + return V8Proxy::throwTypeError(0, args.GetIsolate()); Ditto. > LayoutTests/fast/canvas/webgl/array-set-invalid-arguments.html:21 > + shouldThrow("typedArray.set()"); > + shouldThrow("typedArray.set('hello world')"); Shall we add more tests so that they cover all your changes?
(In reply to comment #3) > (From update of attachment 156183 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156183&action=review > > > Source/WebCore/ChangeLog:11 > > + Update TypedArrays to throw JavaScript RangeError instead of dom > > + INDEX_SIZE_ERR exceptions. Also, update TypedArrays to throw TypeError > > + instead of JavaScript SyntaxError or dom exceptions SYNTAX_ERR when > > + calling set method with invalid arguments. > > Would you add the spec link to the ChangeLog? The spec does not specify the kind of exception to raise. So, should I still put a link to it ?
(In reply to comment #4) > The spec does not specify the kind of exception to raise. So, should I still put a link to it ? (In reply to comment #0) > The TypedArray specification was just updated to remove references to the DOM INDEX_SIZE_ERR exception when certain out-of-range accesses are performed. Where is the "TypedArray specification"? > Another inconsistency: when passing invalid arguments to set method, a dom SYNTAX_ERR exception is thrown in v8. But in JavasScriptCore, it's a JavaScript SyntaxError. > > On the other browsers I tested (firefox and opera), javascript exception are thrown (Error for firefox, TypeError for opera). I think it looks reasonable to raise JavaScript errors for cross-browser compatibility (So your patch looks good to me). Would you also describe other browsers' behavior to the ChangeLog? (We want to describe the rationale for our change to ChangeLog.)
> (In reply to comment #0) > > The TypedArray specification was just updated to remove references to the DOM INDEX_SIZE_ERR exception when certain out-of-range accesses are performed. > > Where is the "TypedArray specification"? > http://www.khronos.org/registry/typedarray/specs/latest/
Created attachment 156216 [details] updated patch to take comments into account
Comment on attachment 156216 [details] updated patch to take comments into account View in context: https://bugs.webkit.org/attachment.cgi?id=156216&action=review > Source/WebCore/ChangeLog:14 > + Other browsers raise JavaScript errors, so those changes will improve > + compatibility. Let's also describe that the spec does not specify the kind of exception with the spec link. > Source/WebCore/bindings/js/JSArrayBufferViewHelper.h:43 > +static const char* notSmallEnoughSize = "ArrayBufferView size is not a small enough positive integer."; "not a small enough positive integer" is a bit confusing. This might be better: static const char* notEnoughSizeErrorMessage = "ArrayBufferView size is not enough."; > Source/WebCore/bindings/js/JSArrayBufferViewHelper.h:44 > +static const char* outOfRangeLengthAndOffset = "Length and offset are out of range."; This might be better: static const char* outOfRangeErrorMessage = "Index is out of range."; > Source/WebCore/bindings/js/JSArrayBufferViewHelper.h:129 > + return JSC::throwTypeError(exec, "Not enough arguments"); throwError(exec, createNotEnoughArgumentsError(exec)) > Source/WebCore/bindings/v8/custom/V8ArrayBufferViewCustom.h:208 > + return V8Proxy::throwTypeError("Not enough arguments", args.GetIsolate()); V8Proxy::throwNotEnoughArgumentsError(args.GetIsolate());
Created attachment 156404 [details] updated patch
Comment on attachment 156404 [details] updated patch LGTM. kbr@ might want to review the patch.
kbr: review please?
Comment on attachment 156404 [details] updated patch Looks good. Could you please make sure the tests in http://www.khronos.org/registry/webgl/sdk/tests/conformance/typedarrays/ pass with these changes? (I suspect they should since they don't have test expectations like WebKit, and the majority of the code is checked in as layout tests.) r=me
Comment on attachment 156404 [details] updated patch conformance tests all pass correctly
Comment on attachment 156404 [details] updated patch Clearing flags on attachment: 156404 Committed r124668: <http://trac.webkit.org/changeset/124668>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by 93167
Created attachment 158676 [details] updated patch to build on mac I tried to separate JSArrayBufferViewHelper and JSTypedArrayBufferViewHelper to not have an unneeeded ooutOfRangeLengthAndOffset static variable. But it made a lot of modifications, and at the end, I could not share the variable tooLargeSize and had to duplicate it anyway. So the best way I could find to fix the build failure on mac is to duplicate the error string. Also, v8 part of the patch is updated to build on top of bug #93792
Comment on attachment 158676 [details] updated patch to build on mac Looks fine. r=me again
Comment on attachment 158676 [details] updated patch to build on mac Clearing flags on attachment: 158676 Committed r125828: <http://trac.webkit.org/changeset/125828>