Bug 112506 – [JSC] Implement EnforceRange IDL attribute for integer conversions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Michael Pruett]

The EnforceRange IDL attribute specifies conditions under which exceptions are thrown when converting ECMAScript numbers to IDL integer types.

http://www.w3.org/TR/WebIDL/#EnforceRange

The EnforceRange attribute has been implemented for V8 in bug 96798. This bug addresses implementation of EnforceRange for JSC.

[Michael Pruett]

Created attachment 193456 [details]
Patch

[Build Bot]

Comment on attachment 193456 [details]
Patch

Attachment 193456 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17112368

[Build Bot]

Comment on attachment 193456 [details]
Patch

Attachment 193456 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17212336

[Build Bot]

Comment on attachment 193456 [details]
Patch

Attachment 193456 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17206368

[Build Bot]

Comment on attachment 193456 [details]
Patch

Attachment 193456 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17179249

[Zan Dobersek]

Comment on attachment 193456 [details]
Patch

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

> Source/WebCore/bindings/js/JSIntegerConversion.cpp:34
> +#include <runtime/JSCJSValueInlines.h>

The Apple build fails due to missing forwarding header for JSCJSValueInlines.h. If required it should be added in Source/WebCore/ForwardingHeaders.

> Tools/GNUmakefile.am:57
> +	Source/WebCore/bindings/js/JSIntegerConversion.h \

Is compiling the new files into libWebCoreInternals.la really required?

[Kentaro Hara]

Comment on attachment 193456 [details]
Patch

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

Thank you very much for making JSC and V8 consistent!

> Source/WebCore/ChangeLog:33
> +        * bindings/js/JSIntegerConversion.cpp: Added.
> +        (enforceRange):
> +        (WebCore):
> +        (WebCore::toInt32):
> +        (WebCore::toUInt32):
> +        (WebCore::toInt64):
> +        (WebCore::toUInt64):
> +        * bindings/js/JSIntegerConversion.h: Added.
> +        (WebCore):

It looks like we have toXXX() helpers in JSDOMBinding.{h,cpp}. How about just adding integer conversion methods to JSDOMBindings ?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3189
> +    if ($type eq "short" or
> +        $type eq "unsigned short" or
> +        $type eq "long" or
> +        $type eq "unsigned long" or
> +        $type eq "long long" or
> +        $type eq "unsigned long long") {

Why do you need the if statement ?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3199
> +            return "$value.toInt32(exec)" if $type eq "long" or $type eq "short";
> +            return "$value.toUInt32(exec)" if $type eq "unsigned long" or $type eq "unsigned short";
> +

Shall we define toInt32(exec, value, NormalConversion) and toUInt32(exec, value, NormalConversion) and use them here? (They can just redirect to value.toInt32() or value.toUInt32().)

Then you will be able to clean up the code like this:

  my $intConversion = $signature->extendedAttributes->{"EnforceRange"} ? "EnforceRange" : "NormalConversion";
  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";
  ...;

[Joshua Bell]

Yes, thanks for taking this on!

If this is implemented correctly on the JSC side I would expect the following tests to need updating in addition to webidl-type-mapping.html:

* fast/dom/non-numeric-values-numeric-parameters.html
* fast/js/select-options-add.html

In bug 96798 I added platform-specific -expected.txt files and updated the main -expected.txt files to have FAIL lines as well.

[Michael Pruett]

Created attachment 193686 [details]
Patch

[Michael Pruett]

(In reply to comment #8)
> If this is implemented correctly on the JSC side I would expect the following tests to need updating in addition to webidl-type-mapping.html:
> 
> * fast/dom/non-numeric-values-numeric-parameters.html
> * fast/js/select-options-add.html
> 
> In bug 96798 I added platform-specific -expected.txt files and updated the main -expected.txt files to have FAIL lines as well.

I've addressed the issue of HTMLOptionsCollection.add() throwing on infinity- and NaN-valued indices in bug 112612.

[Build Bot]

Comment on attachment 193686 [details]
Patch

Attachment 193686 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17188584

[Michael Pruett]

Created attachment 193708 [details]
Patch

[Build Bot]

Comment on attachment 193708 [details]
Patch

Attachment 193708 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17222471

[Build Bot]

Comment on attachment 193708 [details]
Patch

Attachment 193708 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17230171

[Michael Pruett]

Created attachment 193745 [details]
Patch

[Build Bot]

Comment on attachment 193745 [details]
Patch

Attachment 193745 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17220352

[Build Bot]

Comment on attachment 193745 [details]
Patch

Attachment 193745 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17238260

[Build Bot]

Comment on attachment 193745 [details]
Patch

Attachment 193745 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17213588

[Michael Pruett]

Created attachment 193832 [details]
Patch

[Michael Pruett]

Created attachment 193841 [details]
Patch

[Kentaro Hara]

Comment on attachment 193841 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMBinding.cpp:268
> +static double enforceRange(ExecState* exec, double x, double minimum, double maximum)

This helper method looks great; it is easy to understand that the implementation is conformed to the spec. I'd be happy if you could clean up V8Binding.cpp similarly in a follow-up patch:)

> Source/WebCore/bindings/js/JSDOMBinding.h:278
> +    inline int32_t toInt32(JSC::ExecState* exec, JSC::JSValue value, IntegerConversionConfiguration configuration)
> +    {
> +        if (configuration == EnforceRange)
> +            return toInt32EnforceRange(exec, value);
> +        return value.toInt32(exec);
> +    }
> +
> +    inline uint32_t toUInt32(JSC::ExecState* exec, JSC::JSValue value, IntegerConversionConfiguration configuration)
> +    {
> +        if (configuration == EnforceRange)
> +            return toUInt32EnforceRange(exec, value);
> +        return value.toUInt32(exec);
> +    }
> +
> +    int64_t toInt64(JSC::ExecState*, JSC::JSValue, IntegerConversionConfiguration);
> +    uint64_t toUInt64(JSC::ExecState*, JSC::JSValue, IntegerConversionConfiguration);

In a follow-up patch, let's make IntegerConversionConfiguration an optional parameter and set NormalConversion by default (in both CodeGeneratorJS.pm and CodeGeneratorV8.pm). Then you don't need to care about the parameter in caller sites in common cases.

[WebKit Review Bot]

Comment on attachment 193841 [details]
Patch

Rejecting attachment 193841 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 193841, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
TestTypedefs.cpp
patching file Source/WebKit/win/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/win/WebKit.vcproj/WebKitExports.def.in
Hunk #1 FAILED at 414.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit/win/WebKit.vcproj/WebKitExports.def.in.rej
patching file Source/autotools/symbols.filter

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Kentaro Hara']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://webkit-commit-queue.appspot.com/results/17125733

[Michael Pruett]

Created attachment 194159 [details]
Patch

I've rebased the previous patch.

[WebKit Review Bot]

Comment on attachment 194159 [details]
Patch

Clearing flags on attachment: 194159

Committed r146430: <http://trac.webkit.org/changeset/146430>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

This patch may have caused https://bugs.webkit.org/show_bug.cgi?id=112875.

[Ryosuke Niwa]

Fixed Windows build in http://trac.webkit.org/changeset/146445.