Bug 45118

Summary: Update TypedArrays to throw RangeError or similar instead of INDEX_SIZE_ERR
Product: WebKit Reporter: Kenneth Russell <kbr>
Component: WebGLAssignee: arno. <a.renevier>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, a.renevier, cmarrin, enne, haraken, japhet, kbr, psawaya, webkit.review.bot, zmo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 93167    
Bug Blocks:    
Attachments:
Description Flags
patch proposal: change INDEX_SIZE_ERR to RangeError and throws TypeError when calling set with invalid arguments
none
updated patch to take comments into account
none
updated patch
none
updated patch to build on mac none

Kenneth Russell
Reported 2010-09-02 11:16:45 PDT
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.
Attachments
patch proposal: change INDEX_SIZE_ERR to RangeError and throws TypeError when calling set with invalid arguments (18.74 KB, patch)
2012-08-02 15:34 PDT, arno.
no flags
updated patch to take comments into account (19.83 KB, patch)
2012-08-02 17:32 PDT, arno.
no flags
updated patch (20.31 KB, patch)
2012-08-03 10:01 PDT, arno.
no flags
updated patch to build on mac (20.06 KB, patch)
2012-08-15 18:11 PDT, arno.
no flags
arno.
Comment 1 2012-08-02 14:55:13 PDT
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).
arno.
Comment 2 2012-08-02 15:34:44 PDT
Created attachment 156183 [details] patch proposal: change INDEX_SIZE_ERR to RangeError and throws TypeError when calling set with invalid arguments
Kentaro Hara
Comment 3 2012-08-02 17:03:10 PDT
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?
arno.
Comment 4 2012-08-02 17:15:40 PDT
(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 ?
Kentaro Hara
Comment 5 2012-08-02 17:24:29 PDT
(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.)
arno.
Comment 6 2012-08-02 17:29:32 PDT
> (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/
arno.
Comment 7 2012-08-02 17:32:38 PDT
Created attachment 156216 [details] updated patch to take comments into account
Kentaro Hara
Comment 8 2012-08-02 17:53:53 PDT
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());
arno.
Comment 9 2012-08-03 10:01:03 PDT
Created attachment 156404 [details] updated patch
Kentaro Hara
Comment 10 2012-08-03 10:04:26 PDT
Comment on attachment 156404 [details] updated patch LGTM. kbr@ might want to review the patch.
Kentaro Hara
Comment 11 2012-08-03 10:04:51 PDT
kbr: review please?
Kenneth Russell
Comment 12 2012-08-03 11:30:56 PDT
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
arno.
Comment 13 2012-08-03 14:26:39 PDT
Comment on attachment 156404 [details] updated patch conformance tests all pass correctly
WebKit Review Bot
Comment 14 2012-08-03 16:03:33 PDT
Comment on attachment 156404 [details] updated patch Clearing flags on attachment: 156404 Committed r124668: <http://trac.webkit.org/changeset/124668>
WebKit Review Bot
Comment 15 2012-08-03 16:03:37 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 16 2012-08-03 16:21:53 PDT
Re-opened since this is blocked by 93167
arno.
Comment 17 2012-08-15 18:11:19 PDT
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
Kenneth Russell
Comment 18 2012-08-16 15:25:07 PDT
Comment on attachment 158676 [details] updated patch to build on mac Looks fine. r=me again
WebKit Review Bot
Comment 19 2012-08-16 16:53:40 PDT
Comment on attachment 158676 [details] updated patch to build on mac Clearing flags on attachment: 158676 Committed r125828: <http://trac.webkit.org/changeset/125828>
WebKit Review Bot
Comment 20 2012-08-16 16:53:46 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.