Bug 52768 – Construction of Uint8Array from JS Array (and possibly others) incorrectly clamps values

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

[Kenneth Russell]

Tim Johansson from Opera has pointed out that in the WebGL test more/functions/drawElementsBadArgs.html in the Khronos repository, construction of Uint8Array (and possibly other types) taking a JS array is clamping values rather than performing modulo arithmetic. In other words, construction with an array containing the value 256 is resulting in a Uint8Array containing 255 rather than 0. It isn't clear yet whether this bug is in both the JSC and V8 bindings.

[Kenneth Russell]

The bug is in the common C++ code, specifically IntegralTypedArrayBase.h. It's explicitly clamping the incoming double value to the element type's minimum and maximum values, where it should be using casting behavior.

[Zhenyao Mo]

Also the conformance test array-unit-tests.html is failing due to 0.6 is mapped to 1 instead of 0.  According to Web IDL type mapping, floor(abs(x)) * sign(x) is the correct behavior.  Maybe it could be fixed in one patch as the code is in the same function.

[Kenneth Russell]

The rounding mode failure in array-unit-tests.html in Chromium is a different problem. That fix has been committed to V8 and we are waiting for it to be rolled into Chromium.

[Chris Marrin]

(In reply to comment #0)
> Tim Johansson from Opera has pointed out that in the WebGL test more/functions/drawElementsBadArgs.html in the Khronos repository, construction of Uint8Array (and possibly other types) taking a JS array is clamping values rather than performing modulo arithmetic. In other words, construction with an array containing the value 256 is resulting in a Uint8Array containing 255 rather than 0. It isn't clear yet whether this bug is in both the JSC and V8 bindings.

Looking at the typed array spec, it could be more clear about these conversions. There is one statement in the "omittable setter" part of section 5 that gives a link to the WebIDL spec. But I think it would be better call this out into its own section.

I've opened a spec bug about this: https://www.khronos.org/bugzilla/show_bug.cgi?id=420. It's in the WebGL area, which I suppose is an appropriate place for it for now.

[Kenneth Russell]

(In reply to comment #4)
> (In reply to comment #0)
> > Tim Johansson from Opera has pointed out that in the WebGL test more/functions/drawElementsBadArgs.html in the Khronos repository, construction of Uint8Array (and possibly other types) taking a JS array is clamping values rather than performing modulo arithmetic. In other words, construction with an array containing the value 256 is resulting in a Uint8Array containing 255 rather than 0. It isn't clear yet whether this bug is in both the JSC and V8 bindings.
> 
> Looking at the typed array spec, it could be more clear about these conversions. There is one statement in the "omittable setter" part of section 5 that gives a link to the WebIDL spec. But I think it would be better call this out into its own section.
> 
> I've opened a spec bug about this: https://www.khronos.org/bugzilla/show_bug.cgi?id=420. It's in the WebGL area, which I suppose is an appropriate place for it for now.

For the record, the spec bug has been fixed; the typed array spec is much more clear about rounding mode and type conversion behavior.

[Kenneth Russell]

Created attachment 82729 [details]
Patch

[Chris Marrin]

Comment on attachment 82729 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82729&action=review

Generally fine, one nit, and one question about the necessity of LargestIntType in the template. r=me, after settling those.

> Source/WebCore/ChangeLog:13
> +        required template parameter.

I don't understand the second parameter to IntegralTypedArrayBase. It's always a 64 bit int, which it would have to be to hold a any possible double. I don't think having it be a uint64 in some places is useful because a signed 64 bit number is plenty to hold all possible double values. I think JS requires numbers to be doubles so why not just eliminate it? Seems like unnecessary future-proofing

> Source/WebCore/bindings/js/JSArrayBufferViewHelper.h:137
> +        unsigned length = srcArray->get(exec, JSC::Identifier(exec, "length")).toUInt32(exec);

It would be more consistent to use uint32_t instead of unsigned here?

[Kenneth Russell]

(In reply to comment #7)
> (From update of attachment 82729 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82729&action=review
> 
> Generally fine, one nit, and one question about the necessity of LargestIntType in the template. r=me, after settling those.
> 
> > Source/WebCore/ChangeLog:13
> > +        required template parameter.
> 
> I don't understand the second parameter to IntegralTypedArrayBase. It's always a 64 bit int, which it would have to be to hold a any possible double. I don't think having it be a uint64 in some places is useful because a signed 64 bit number is plenty to hold all possible double values. I think JS requires numbers to be doubles so why not just eliminate it? Seems like unnecessary future-proofing

I didn't experiment enough, but you're right; the intermediate cast can always be to int64_t, so the LargestIntType template parameter can be eliminated. This simplifies the patch.

> > Source/WebCore/bindings/js/JSArrayBufferViewHelper.h:137
> > +        unsigned length = srcArray->get(exec, JSC::Identifier(exec, "length")).toUInt32(exec);
> 
> It would be more consistent to use uint32_t instead of unsigned here?

OK, I'll make this change.

[Kenneth Russell]

Committed r78919: <http://trac.webkit.org/changeset/78919>