WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
45118
Update TypedArrays to throw RangeError or similar instead of INDEX_SIZE_ERR
https://bugs.webkit.org/show_bug.cgi?id=45118
Summary
Update TypedArrays to throw RangeError or similar instead of INDEX_SIZE_ERR
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
Details
Formatted Diff
Diff
updated patch to take comments into account
(19.83 KB, patch)
2012-08-02 17:32 PDT
,
arno.
no flags
Details
Formatted Diff
Diff
updated patch
(20.31 KB, patch)
2012-08-03 10:01 PDT
,
arno.
no flags
Details
Formatted Diff
Diff
updated patch to build on mac
(20.06 KB, patch)
2012-08-15 18:11 PDT
,
arno.
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug