Bug 45118 - Update TypedArrays to throw RangeError or similar instead of INDEX_SIZE_ERR
Summary: Update TypedArrays to throw RangeError or similar instead of INDEX_SIZE_ERR
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: arno.
URL:
Keywords:
Depends on: 93167
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-02 11:16 PDT by Kenneth Russell
Modified: 2012-08-16 16:53 PDT (History)
10 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 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.
Comment 1 arno. 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).
Comment 2 arno. 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
Comment 3 Kentaro Hara 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?
Comment 4 arno. 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 ?
Comment 5 Kentaro Hara 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.)
Comment 6 arno. 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/
Comment 7 arno. 2012-08-02 17:32:38 PDT
Created attachment 156216 [details]
updated patch to take comments into account
Comment 8 Kentaro Hara 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());
Comment 9 arno. 2012-08-03 10:01:03 PDT
Created attachment 156404 [details]
updated patch
Comment 10 Kentaro Hara 2012-08-03 10:04:26 PDT
Comment on attachment 156404 [details]
updated patch

LGTM. kbr@ might want to review the patch.
Comment 11 Kentaro Hara 2012-08-03 10:04:51 PDT
kbr: review please?
Comment 12 Kenneth Russell 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
Comment 13 arno. 2012-08-03 14:26:39 PDT
Comment on attachment 156404 [details]
updated patch

conformance tests all pass correctly
Comment 14 WebKit Review Bot 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>
Comment 15 WebKit Review Bot 2012-08-03 16:03:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 WebKit Review Bot 2012-08-03 16:21:53 PDT
Re-opened since this is blocked by 93167
Comment 17 arno. 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
Comment 18 Kenneth Russell 2012-08-16 15:25:07 PDT
Comment on attachment 158676 [details]
updated patch to build on mac

Looks fine. r=me again
Comment 19 WebKit Review Bot 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>
Comment 20 WebKit Review Bot 2012-08-16 16:53:46 PDT
All reviewed patches have been landed.  Closing bug.