Bug 45118 – Update TypedArrays to throw RangeError or similar instead of INDEX_SIZE_ERR

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kenneth Russell]

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.

[arno.]

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.]

Created attachment 156183 [details]
patch proposal: change INDEX_SIZE_ERR to RangeError and throws TypeError when calling set with invalid arguments

[Kentaro Hara]

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.]

(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]

(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.]

> (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.]

Created attachment 156216 [details]
updated patch to take comments into account

[Kentaro Hara]

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.]

Created attachment 156404 [details]
updated patch

[Kentaro Hara]

Comment on attachment 156404 [details]
updated patch

LGTM. kbr@ might want to review the patch.

[Kentaro Hara]

kbr: review please?

[Kenneth Russell]

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 on attachment 156404 [details]
updated patch

conformance tests all pass correctly

[WebKit Review Bot]

Comment on attachment 156404 [details]
updated patch

Clearing flags on attachment: 156404

Committed r124668: <http://trac.webkit.org/changeset/124668>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 93167

[arno.]

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 on attachment 158676 [details]
updated patch to build on mac

Looks fine. r=me again

[WebKit Review Bot]

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]

All reviewed patches have been landed.  Closing bug.