Bug 123661

Summary: EnforceRange doesn't enforce range of a short
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: BindingsAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cdumez, commit-queue, heycam, michael, rniwa, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 122679    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
ap: review+
Improved patch for landing none

Description Alexey Proskuryakov 2013-11-02 01:00:08 PDT
In WebIDL, short has a range of  [−32768, 32767], and unsigned short has a range of [0, 65535]. But our code generator just does a 32-bit conversion:

    return "toInt32(exec, $value, $intConversion)" if $type eq "long" or $type eq "short";
    return "toUInt32(exec, $value, $intConversion)" if $type eq "unsigned long" or $type eq "unsigned short";
Comment 1 Chris Dumez 2013-11-02 16:41:07 PDT
Created attachment 215835 [details]
Patch
Comment 2 Build Bot 2013-11-02 17:20:20 PDT
Comment on attachment 215835 [details]
Patch

Attachment 215835 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/19668405
Comment 3 Build Bot 2013-11-02 17:26:10 PDT
Comment on attachment 215835 [details]
Patch

Attachment 215835 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/19558427
Comment 4 Chris Dumez 2013-11-02 17:27:35 PDT
Created attachment 215839 [details]
Patch
Comment 5 Chris Dumez 2013-11-02 17:33:12 PDT
Created attachment 215840 [details]
Patch
Comment 6 Alexey Proskuryakov 2013-11-02 19:28:54 PDT
This conflicts with my patch in bug 123669 a little, which has a partial implementation in JSDOMBinding. But it's fine to land this first.
Comment 7 Alexey Proskuryakov 2013-11-02 19:36:13 PDT
Comment on attachment 215840 [details]
Patch

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

Some of the comments below definitely don't need to be addressed in this patch. Some probably need to be addressed one way or another.

> Source/WebCore/ChangeLog:17
> +        No new tests, covered by js/dom/webidl-type-mapping.html.

I'd have said "added test cases to js/dom/webidl-type-mapping.html"

> Source/WebCore/bindings/js/JSDOMBinding.cpp:328
> +struct IntTypeLimits<short> {

It's a strange mixup that we use int8_t, but don't use int16_t. Is possible to make this consistent?

> Source/WebCore/bindings/js/JSDOMBinding.cpp:386
> +        return static_cast<T>(d % LimitsTrait::numberOfValues);

I don't understand why taking a remainder is necessary. Why can't we simply cast?

> Source/WebCore/bindings/js/JSDOMBinding.cpp:416
> +short toInt16(ExecState* exec, JSValue value, IntegerConversionConfiguration configuration)

Same comment about int16_t vs. short.

> LayoutTests/js/dom/webidl-type-mapping-expected.txt:746
> +PASS converter.testShort is 0

It's unfortunate that these results are unreadable, they don't tell what the value being converted was.

> LayoutTests/js/dom/webidl-type-mapping.html:426
> +convert(type, "0");

Would be nice to test negative zero everywhere.
Comment 8 Chris Dumez 2013-11-02 20:02:00 PDT
Comment on attachment 215840 [details]
Patch

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

>> Source/WebCore/ChangeLog:17
>> +        No new tests, covered by js/dom/webidl-type-mapping.html.
> 
> I'd have said "added test cases to js/dom/webidl-type-mapping.html"

Ok, I will fix this.

>> Source/WebCore/bindings/js/JSDOMBinding.cpp:328
>> +struct IntTypeLimits<short> {
> 
> It's a strange mixup that we use int8_t, but don't use int16_t. Is possible to make this consistent?

Ok, I will switch to int16_t / uint_16t for consistency.

>> Source/WebCore/bindings/js/JSDOMBinding.cpp:386
>> +        return static_cast<T>(d % LimitsTrait::numberOfValues);
> 
> I don't understand why taking a remainder is necessary. Why can't we simply cast?

Yes, we can simply cast. I will get rid of the modulo.

>> Source/WebCore/bindings/js/JSDOMBinding.cpp:416
>> +short toInt16(ExecState* exec, JSValue value, IntegerConversionConfiguration configuration)
> 
> Same comment about int16_t vs. short.

Will fix.

>> LayoutTests/js/dom/webidl-type-mapping-expected.txt:746
>> +PASS converter.testShort is 0
> 
> It's unfortunate that these results are unreadable, they don't tell what the value being converted was.

Actually it does. The results are on 2 lines. Based on the line before, we know 0 was assigned.

>> LayoutTests/js/dom/webidl-type-mapping.html:426
>> +convert(type, "0");
> 
> Would be nice to test negative zero everywhere.

Hmm. The thing is that I am not 100% sure what the result is supposed to be when assigning -0. Is is supposed to become 0 in all cases? (it seems to be what the current code gives as a result).
I was wondering if assigning -0 on to an unsigned short with [EnforceRange] should throw for example. Currently it does not.
Comment 9 Chris Dumez 2013-11-02 20:09:09 PDT
Created attachment 215843 [details]
Improved patch for landing

I took feedback into consideration, thanks.

The only thing is that I am not 100% sure about the results for -0 but at least it is covered by layout test now if we want to change the behavior.
Comment 10 WebKit Commit Bot 2013-11-02 21:11:54 PDT
Comment on attachment 215843 [details]
Improved patch for landing

Clearing flags on attachment: 215843

Committed r158521: <http://trac.webkit.org/changeset/158521>
Comment 11 WebKit Commit Bot 2013-11-02 21:11:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Alexey Proskuryakov 2013-11-02 22:15:29 PDT
Thank you for dressing this so quickly!

> The only thing is that I am not 100% sure about the results for -0 but at least it is covered by layout test now if we want to change the behavior.

I think that it's a mistake in WebIDL - it should have a special case for zeroes, because using sign(x) on either +0 or -0 is undefined in ECMAScript.