Bug 96798 – [V8] Binding: Implement EnforceRange IDL Attribute for long long conversions

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Joshua Bell]

From the WebIDL spec: http://dev.w3.org/2006/webapi/WebIDL/#es-long-long

1. If x is NaN, +∞, or −∞, then throw a TypeError.
2. Set x to sign(x) * floor(abs(x)).
3. If x < −(2^53 − 1) or x > 2^53 − 1, then throw a TypeError.
4. Return the IDL long long value that represents the same numeric value as x.

Note that there are similar cases for unsigned long long.

The binding code generator says:

return "toInt64($value)" if $type eq "unsigned long long" or $type eq "long long";

and toInt64 defined in bindings/v8/V8Binding.h is:

    inline long long toInt64(v8::Local<v8::Value> value)
    {
        return static_cast<long long>(value->IntegerValue());
    }

... so there are no guard checks. 

This causes IndexedDB to fail the test http://w3c-test.org/webapps/IndexedDB/tests/submissions/Ms2ger/idbfactory_open9.htm

We could special case this in the IDB code, or change toInt64 to incorporate the checks from WebIDL and call throwTypeError().

[David Grogan]

Adding negative sign

[Joshua Bell]

Note that this should only apply if the IDL has [EnforceRange] extended attribute.

[Joshua Bell]

Debugging through this, NaN, Infinity and -Infinity are being coerced to -0x8000000000000000 via V8's IntegerValue(). It's IndexedDB that's generating the type error for those.

So basically we need to implement all these tests.

[Joshua Bell]

Huh, I'm a bit stumped.

This should be a matter of dumping the appropriate tests into V8Binding.h's toInt64 (and then refactoring), and having toInt64() call throwTypeError(). However, when this is done the exception is not caught/rethrown by the v8::TryCatch block in EXCEPTION_BLOCK, so the script never sees the exception. 

Maybe v8 change r11894 is to blame?

Calling throwTypeError() winds its way into api.cc's ThrowException which calls Isolate::ScheduleThrow(). That method calls into Isolate::Throw(), then immediately moves the exception from "pending" to "scheduled" and clears the pending exception. Tracing through, nothing causes the "scheduled" exception to be "promoted" back to pending before the TryCatch block concludes.

v8 r11894 monkeys with the contents of Isolate::ScheduleThrow, removing a call to PropagatePendingExceptionToExternalTryCatch() which seems to be needed. If I undo that change then throwTypeError() works as expected.

[Joshua Bell]

Created attachment 164990 [details]
Patch

[Joshua Bell]

(In reply to comment #5)
> Created an attachment (id=164990) [details]
> Patch

Uploaded a patch, but it won't work until we sort out the V8 issue. (If I revert http://code.google.com/p/v8/source/diff?spec=svn11894&old=11882&r=11894&format=unidiff&path=%2Ftrunk%2Fsrc%2Fisolate.cc locally it works!)

Also:

* Doesn't implement EnforceRange for "long" types, just "long long"
* The logic should probably move from V8Binding.h to V8Binding.cpp
* I didn't touch JSC at all

[Joshua Bell]

(In reply to comment #4)
> This should be a matter of dumping the appropriate tests into V8Binding.h's toInt64 (and then refactoring), and having toInt64() call throwTypeError(). However, when this is done the exception is not caught/rethrown by the v8::TryCatch block in EXCEPTION_BLOCK, so the script never sees the exception. 

haraken@ points out this is http://code.google.com/p/v8/issues/detail?id=2166 - in summary, TryCatch is not nestable.

[Joshua Bell]

(In reply to comment #7)
> (In reply to comment #4)
> > This should be a matter of dumping the appropriate tests into V8Binding.h's toInt64 (and then refactoring), and having toInt64() call throwTypeError(). However, when this is done the exception is not caught/rethrown by the v8::TryCatch block in EXCEPTION_BLOCK, so the script never sees the exception. 
> 
> haraken@ points out this is http://code.google.com/p/v8/issues/detail?id=2166 - in summary, TryCatch is not nestable.

Once Chromium rolls v8 to r13130 this should be unblocked on the v8 side.

[Erik Arvidsson]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #4)
> > > This should be a matter of dumping the appropriate tests into V8Binding.h's toInt64 (and then refactoring), and having toInt64() call throwTypeError(). However, when this is done the exception is not caught/rethrown by the v8::TryCatch block in EXCEPTION_BLOCK, so the script never sees the exception. 
> > 
> > haraken@ points out this is http://code.google.com/p/v8/issues/detail?id=2166 - in summary, TryCatch is not nestable.
> 
> Once Chromium rolls v8 to r13130 this should be unblocked on the v8 side.

I don't understand why we need V8 changes for this. See bug 101783 for a WIP patch that worked (to some extent). Maybe it is cleaner to wait for V8 to allow anything to throw but we can keep track of the success state as we do the conversions.

[Joshua Bell]

(In reply to comment #9)
> I don't understand why we need V8 changes for this. See bug 101783 for a WIP patch that worked (to some extent). Maybe it is cleaner to wait for V8 to allow anything to throw but we can keep track of the success state as we do the conversions.

That's true, it's not technically blocked. I was favoring having the toIntXXX() methods throw for readability but passing in an bool& obviously works as well.

The throwing approach works, apart from a v8 bug which has now been resolved, so we can choose either approach. I wasn't aware of the bug when I started this patch and as it wasn't urgent and the v8 fix seemed imminent I left it blocked rather than exploring other approaches.

[Erik Arvidsson]

(In reply to comment #10)
> (In reply to comment #9)
> > I don't understand why we need V8 changes for this. See bug 101783 for a WIP patch that worked (to some extent). Maybe it is cleaner to wait for V8 to allow anything to throw but we can keep track of the success state as we do the conversions.
> 
> That's true, it's not technically blocked. I was favoring having the toIntXXX() methods throw for readability but passing in an bool& obviously works as well.

I'm not sure which approach is better. Today, where we do not handle any type conversion failures, I think using a throw is less invasive, but given that we really should handle all type conversion failures I'm not sure throw is a better solution?

[Joshua Bell]

(In reply to comment #11)
> I'm not sure which approach is better. Today, where we do not handle any type conversion failures, I think using a throw is less invasive, but given that we really should handle all type conversion failures I'm not sure throw is a better solution?

I was imagining we'd end up with a repeated pattern like this anyway in the API:

XXX toXXX(x, options..., bool& ok);
XXX toXXXMaybeThrow(x, options...) { bool ok; XXX r = toXXX(x, ok); if (!ok) throwTypeError(); return r; }

So eliminating the toXXXMaybeThrow and relying on the generated code to throw seems like it will result in less code overall.

[Kentaro Hara]

(In reply to comment #9)
> > > haraken@ points out this is http://code.google.com/p/v8/issues/detail?id=2166 - in summary, TryCatch is not nestable.

BTW, this bug is going to be fixed shortly.

[Joshua Bell]

Created attachment 180821 [details]
Patch

[Joshua Bell]

Latest patch drops dependency on nested v8 try/catch blocks and implements this in the style of bug 101783 - thanks arv@!

To match the IDL it is more lenient for u/int32 conversions in some cases when [EnforceRange] is not specified.

This does NOT YET implement the JSC version, only the V8 version, so it's not ready for landing but I wanted to see what the bots think and get feedback.

[WebKit Review Bot]

Comment on attachment 180821 [details]
Patch

Attachment 180821 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15549644

New failing tests:
fast/js/select-options-add.html
fast/dom/non-numeric-values-numeric-parameters.html

[Erik Arvidsson]

Comment on attachment 180821 [details]
Patch

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

> Source/WebCore/bindings/v8/V8Binding.cpp:136
> +        x = (x < 0 ? -1 : 1) * floor(fabs(x));

Can't this just be (int) x?

> Source/WebCore/bindings/v8/V8Binding.cpp:147
> +    if (isnan(numberValue) || isinf(numberValue))

I'm concerned about this change. Why do we silently want to treat NaN and Infinity as 0?

[Joshua Bell]

Comment on attachment 180821 [details]
Patch

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

>> Source/WebCore/bindings/v8/V8Binding.cpp:136
>> +        x = (x < 0 ? -1 : 1) * floor(fabs(x));
> 
> Can't this just be (int) x?

After reviewing the C standard: yes. "When a value of floating type is converted to integral type, the fractional part is discarded."

(Unsurprising, as WebIDL is likely just specifying existing behavior of implementations.)

>> Source/WebCore/bindings/v8/V8Binding.cpp:147
>> +    if (isnan(numberValue) || isinf(numberValue))
> 
> I'm concerned about this change. Why do we silently want to treat NaN and Infinity as 0?

WebIDL does not specify that an exception should be raised in non-finite cases if [EnforceRange] is not specified. That's the point of the attribute - to allow interfaces to turn on throwing behavior.

I agree that this is the scariest part of the patch, as it changes the behavior of existing binding code. I will look more carefully at where toIntXX(value, ok) is called explicitly, since I think generated code never uses that form unless [EnforceRange] is specified.

[Erik Arvidsson]

(In reply to comment #18)
> WebIDL does not specify that an exception should be raised in non-finite cases if [EnforceRange] is not specified. That's the point of the attribute - to allow interfaces to turn on throwing behavior.

My assumption was wrong. I just double checked.

I wonder if we could test these behaviors? One idea would be to have add an object to testRunner where we have all of these. I'll file a bug.

[Joshua Bell]

(In reply to comment #18)
> (From update of attachment 180821 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180821&action=review
> 
> >> Source/WebCore/bindings/v8/V8Binding.cpp:136
> >> +        x = (x < 0 ? -1 : 1) * floor(fabs(x));
> > 
> > Can't this just be (int) x?
> 
> After reviewing the C standard: yes. "When a value of floating type is converted to integral type, the fractional part is discarded."

Hrm, well, trickier than that as the subsequent tests look for values outside the range of the destination type. The C++ standard says "The behavior is undefined if the truncated value cannot be represented in the destination type". So... I'm not sure we can take a shortcut here.

[Joshua Bell]

Details on the two test failures:

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

Both are probing the behavior around:

document.createElement('select').options.add(document.createElement('option'), x);

... for various values of X.

x = Infinity  ==> Chrome 23 throws, Safari 6 throws, FF17 throws, IE10 throws.
x = -Infinity ==> Chrome 23 throws, Safari 6 throws, FF17 throws, IE10 throws.
x = NaN       ==> Chrome 23 throws, Safari 6 throws, FF17 ok,     IE10 throws.
x = -2        ==> Chrome 23 throws, Safari 6 throws, FF17 ok,     IE10 throws.
x = -1        ==> Chrome 23 ok,     Safari 6 ok,     FF17 ok,     IE10 ok.
x = 100       ==> Chrome 23 ok,     Safari 6 ok,     FF17 ok,     IE10 ok.

The HTML spec has:

void add((HTMLOptionElement or HTMLOptGroupElement) element, optional (HTMLElement or long)? before = null);

... and the prose reads: "If before is omitted, null, or a number out of range, then element will be added at the end of the list" - therefore, I would not expect the throwing behavior in any of these cases.

...

The fast/dom/non-numeric-values-numeric-parameters.html test actually has a bug in the test where it is setting NaNAllowed but testing nanAllowed, so it is not correctly probing NaN support for this method, which may explain the divergence between WebKit and Gecko.

[Joshua Bell]

Per discussion w/ Moz folks on #whatwg, this bit of Gecko code has crufty old custom binding code not based on the IDL, so the behavior there is not intentional. jwalden agrees that per-spec none of these should throw.

[Joshua Bell]

Filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=20553 against the HTML spec.

[Joshua Bell]

Created attachment 181764 [details]
Patch

[Joshua Bell]

(In reply to comment #24)
> Created an attachment (id=181764) [details]
> Patch

Updated patch still a WIP. Handles attribute setters via another macro (since return type is void). Adds more attributes to the TypeConversions object and corresponding tests.

Still V8 only.

[Joshua Bell]

Created attachment 187960 [details]
Patch

[Joshua Bell]

Rebased. Still V8 only.

* Passes throwTypeError() the required isolate argument.
* Introduces a ConversionAttributes enum of flags in V8Binding.h so that eventually [Clamp] can be supported without adding yet another boolean arg.

I think this is good-to-go on the V8 side, but the JSC side needs some luv.

[Adam Barth]

Would you like me to review your patch?

[Joshua Bell]

Comment on attachment 187960 [details]
Patch

abarth@ - Sure, feedback at this point is welcome. r?

[Adam Barth]

Comment on attachment 187960 [details]
Patch

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

> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:202
> -    int v = toInt32(value);
> +    V8TRYCATCH_FOR_TYPECHECK_VOID(int, v, toInt32(value, false, ok));

It looks like we're now always creating a try-catch block.  Is that necessary?  Does that impact performance when the [EnforceRange] attribute isn't specified?

[Joshua Bell]

Comment on attachment 187960 [details]
Patch

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

> Source/WebCore/bindings/v8/V8BindingMacros.h:77
> +            throwTypeError(0, info.GetIsolate());                \

This should just be |isolate|

[Joshua Bell]

Comment on attachment 187960 [details]
Patch

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

>> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:202
>> +    V8TRYCATCH_FOR_TYPECHECK_VOID(int, v, toInt32(value, false, ok));
> 
> It looks like we're now always creating a try-catch block.  Is that necessary?  Does that impact performance when the [EnforceRange] attribute isn't specified?

Thanks - no, shouldn't be necessary; probably residue from earlier iterations. In the ...TYPECHECK_VOID case there shouldn't need to be a try-catch block. This can reduce to a TYPECHECK macro.

[Build Bot]

Comment on attachment 187960 [details]
Patch

Attachment 187960 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16547013

New failing tests:
fast/dom/non-numeric-values-numeric-parameters.html
fast/js/webidl-type-mapping.html

[Build Bot]

Comment on attachment 187960 [details]
Patch

Attachment 187960 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16563012

New failing tests:
fast/dom/non-numeric-values-numeric-parameters.html
fast/js/webidl-type-mapping.html

[Joshua Bell]

Created attachment 188184 [details]
Patch

[Joshua Bell]

Let's give that another whirl:

* Minimized code generator output differences - callers to JSValueToNative call a helper to determine if type checking is necessary and only use type checking macro wrappers if necessary. Ugly, but safer.
* Only need TYPECHECK_VOID and V8TRYCATCH_WITH_TYPECHECK macros. With more extensive refactoring could probably compose those. Alternately, these could be inlined but I think these will eventually end up being used in 1-2 more places (e.g. Dictionary value access)
* Actually ran the tests, which turned up stupid logic bugs with the flags. Derp.
* Updated the test for HTMLOptionsCollection to match the HTML spec, which matches the new behavior of the code (see comment #21)

[Kentaro Hara]

Comment on attachment 188184 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3928
> +sub ConversionThrowsTypeError

Will this method be useful for other purpose? Otherwise, I would hard-code $signature->extendedAttributes->{"EnforceRange"} in call sites and remove the method.

> Source/WebCore/bindings/v8/V8Binding.cpp:115
>      v8::Local<v8::Number> numberObject = value->ToNumber();

This can throw. You need a try-catch block.

> Source/WebCore/bindings/v8/V8Binding.cpp:121
> +    if (attributes & FEnforceRange) {

[EnforgeRange] must not be in conjunction with [Clamp]:
http://www.w3.org/TR/WebIDL/#EnforceRange

So it would be useless to use a bit-masking approach.

> Source/WebCore/bindings/v8/V8Binding.cpp:128
> +        if (x < -0x8000000 || x > 0x7fffffff) {

Nit: Shall we use a constant variable for these magic numbers?

> Source/WebCore/bindings/v8/V8Binding.cpp:166
> +    if (attributes & FEnforceRange) {

Ditto.

> Source/WebCore/bindings/v8/V8Binding.cpp:173
> +        if (x < 0 || x > 0xffffffff) {

Nit: Use a constant variable for the magic number.

> Source/WebCore/bindings/v8/V8Binding.cpp:198
> +    v8::Local<v8::Number> numberObject = value->ToNumber();

This can throw. You need a try-catch block.

> Source/WebCore/bindings/v8/V8Binding.cpp:206
> +    if (attributes & FEnforceRange) {

Ditto.

> Source/WebCore/bindings/v8/V8Binding.cpp:237
> +        if (result >= 0)
> +            return result;

Can we also add a fast path for result < 0 ?

> Source/WebCore/bindings/v8/V8Binding.cpp:241
> +    v8::Local<v8::Number> numberObject = value->ToNumber();

This can throw. You need a try-catch block.

> Source/WebCore/bindings/v8/V8Binding.cpp:249
> +    if (attributes & FEnforceRange) {

Ditto.

> Source/WebCore/bindings/v8/V8Binding.h:348
> +    enum ConversionAttributes {

Nit: IntergerConversionConfiguration?

> Source/WebCore/bindings/v8/V8Binding.h:351
> +        FNone = 0x0000,
> +        FEnforceRange = 0x0001,
> +        // FIXME: Implement FClamp = 0x0002

Nit: Are these hard-coded values (e.g. 0x0001) meaningful? Otherwise, we can drop them.

Nit: Also is "FEnforceRange" a speced name somewhere? Otherwise, we can simply say "EnforceRange".

> Source/WebCore/bindings/v8/V8BindingMacros.h:46
> +        bool ok = true;                           \
> +        var = (value);                            \
> +        if (UNLIKELY(!ok)) {                      \

Where is |ok| updated?

> Source/WebCore/bindings/v8/V8BindingMacros.h:71
> +        if (UNLIKELY(!ok))                                   \

Ditto.

[Kentaro Hara]

Comment on attachment 188184 [details]
Patch

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

> LayoutTests/fast/js/webidl-type-mapping.html:45
> +function testNonNumericToNumericEnforceRange(attribute)

You might want to add more test cases for non numeric values, especially a case where an exception is thrown during value conversion.

[Joshua Bell]

(In reply to comment #37)
> (From update of attachment 188184 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188184&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3928
> > +sub ConversionThrowsTypeError
> 
> Will this method be useful for other purpose? Otherwise, I would hard-code $signature->extendedAttributes->{"EnforceRange"} in call sites and remove the method.

I was imagining that it would be, but I also wasn't really paying attention to what [Clamp] meant. I can inline it.
 
> > Source/WebCore/bindings/v8/V8Binding.cpp:115
> >      v8::Local<v8::Number> numberObject = value->ToNumber();
> 
> This can throw. You need a try-catch block.

Bleah. This is also in the existing code, which means existing conversions are buggy. To abarth@'s earlier point this probably means all numeric conversions will need to get wrapped in try-catch blocks, not just those with [EnforceRange] specified.

> 
> > Source/WebCore/bindings/v8/V8Binding.cpp:121
> > +    if (attributes & FEnforceRange) {
> 
> [EnforgeRange] must not be in conjunction with [Clamp]:
> http://www.w3.org/TR/WebIDL/#EnforceRange
> 
> So it would be useless to use a bit-masking approach.

Thanks. I should have looked more closely at [Clamp]. That simplifies things.

> 
> > Source/WebCore/bindings/v8/V8Binding.cpp:128
> > +        if (x < -0x8000000 || x > 0x7fffffff) {
> 
> Nit: Shall we use a constant variable for these magic numbers?

Sure. kMinInt32, kMaxUint32, etc?

> > Source/WebCore/bindings/v8/V8Binding.cpp:237
> > +        if (result >= 0)
> > +            return result;
> 
> Can we also add a fast path for result < 0 ?

Yep. Will depend on the attributes so will have multiple cases, but probably worth it.

> > Source/WebCore/bindings/v8/V8Binding.h:348
> > +    enum ConversionAttributes {
> 
> Nit: IntergerConversionConfiguration?

Sure.
 
> > Source/WebCore/bindings/v8/V8Binding.h:351
> > +        FNone = 0x0000,
> > +        FEnforceRange = 0x0001,
> > +        // FIXME: Implement FClamp = 0x0002
> 
> Nit: Are these hard-coded values (e.g. 0x0001) meaningful? Otherwise, we can drop them.
> 
> Nit: Also is "FEnforceRange" a speced name somewhere? Otherwise, we can simply say "EnforceRange".

Since [Clamp] and [EnforceRange] are exclusive, the values and 'F' prefixes can definitely be dropped. Thanks again, I should have looked more closely at the spec for [Clamp].

> > Source/WebCore/bindings/v8/V8BindingMacros.h:46
> > +        bool ok = true;                           \
> > +        var = (value);                            \
> > +        if (UNLIKELY(!ok)) {                      \
> 
> Where is |ok| updated?

In the cases where these macros are used, the expression generated by JSValueToNative() passes ok into the ToIntXX functions, that expression is what ends up as the |value| macro parameter. (This was arv@'s suggestion.)

[Kentaro Hara]

> > > Source/WebCore/bindings/v8/V8Binding.cpp:115
> > >      v8::Local<v8::Number> numberObject = value->ToNumber();
> > 
> > This can throw. You need a try-catch block.
> 
> Bleah. This is also in the existing code, which means existing conversions are buggy. To abarth@'s earlier point this probably means all numeric conversions will need to get wrapped in try-catch blocks, not just those with [EnforceRange] specified.

Yeah, I think you can fix it in a separate patch. (Actually we have similar bugs in a lot of places in custom bindings... not only integer conversion but also string conversions.)

> > Nit: Shall we use a constant variable for these magic numbers?
> 
> Sure. kMinInt32, kMaxUint32, etc?

Sounds good!

[Joshua Bell]

Created attachment 188229 [details]
Patch

[Kentaro Hara]

Comment on attachment 188229 [details]
Patch

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

LGTM.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1252
> +            my $value = JSValueToNative($attribute->signature, "value", "info.GetIsolate()", 1);

1 is unclear. Let's pass a string "canThrowException".

[Kentaro Hara]

(In reply to comment #40)
> Yeah, I think you can fix it in a separate patch. (Actually we have similar bugs in a lot of places in custom bindings... not only integer conversion but also string conversions.)

Please add more test cases for non numeric values (exception cases) when you work on the follow-up patch.

[Joshua Bell]

(In reply to comment #42)
> 
> 1 is unclear. Let's pass a string "canThrowException".

Done. (We should do this for the other methods that take optional bools, too.)

So... thoughts on the JSC side here? Tackle it now, handle it with TestExpectations, ...?

[Joshua Bell]

Created attachment 190095 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 190095 [details]
Patch for landing

Rejecting attachment 190095 [details] from commit-queue.

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

Last 500 characters of output:
LayoutTests/fast/dom/script-tests/non-numeric-values-numeric-parameters.js
patching file LayoutTests/fast/js/webidl-type-mapping-expected.txt
patching file LayoutTests/fast/js/webidl-type-mapping.html
patching file LayoutTests/storage/indexeddb/intversion-bad-parameters-expected.txt
patching file LayoutTests/storage/indexeddb/resources/intversion-bad-parameters.js

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

Full output: http://queues.webkit.org/results/16760403

[Joshua Bell]

Created attachment 190301 [details]
Patch for landing

[Build Bot]

Comment on attachment 190301 [details]
Patch for landing

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

New failing tests:
fast/dom/non-numeric-values-numeric-parameters.html

[WebKit Review Bot]

Comment on attachment 190301 [details]
Patch for landing

Attachment 190301 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16781400

New failing tests:
fast/js/select-options-add.html
fast/js/webidl-type-mapping.html

[Build Bot]

Comment on attachment 190301 [details]
Patch for landing

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

New failing tests:
fast/dom/non-numeric-values-numeric-parameters.html

[Peter Beverloo (cr-android ews)]

Comment on attachment 190301 [details]
Patch for landing

Attachment 190301 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/16797184

[Joshua Bell]

Created attachment 192258 [details]
Patch

[Joshua Bell]

Created attachment 192267 [details]
Patch

[Peter Beverloo (cr-android ews)]

Comment on attachment 192267 [details]
Patch

Attachment 192267 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17016293

[WebKit Review Bot]

Comment on attachment 192267 [details]
Patch

Attachment 192267 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17110231

New failing tests:
fast/dom/non-numeric-values-numeric-parameters.html

[WebKit Review Bot]

Comment on attachment 192267 [details]
Patch

Attachment 192267 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17016336

New failing tests:
fast/dom/non-numeric-values-numeric-parameters.html

[Joshua Bell]

Created attachment 192526 [details]
Patch

[Joshua Bell]

Needed the passing expectation for chromium on that test.

...

Sorry about the patch churn. It's all about the previously mentioned case:

document.createElement('select').options.add(document.createElement('option'), x);

... which per spec (and Firefox) shouldn't throw for non-finite x, but previously did. This patch will make it not throw in V8, but it will continue to throw in JSC. Therefore, I've updated the tests to match the spec, which leaves them with FAIL expectations in JSC and requires different expectations in V8.

Do any JSC experts want to weigh in before landing?

haraken@ - can you give it one last look?

[Peter Beverloo (cr-android ews)]

Comment on attachment 192526 [details]
Patch

Attachment 192526 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17011690

[Kentaro Hara]

Comment on attachment 192526 [details]
Patch

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

Getting closer!

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3994
> +    my $intConversion = $signature->extendedAttributes->{"EnforceRange"} ? "EnforceRange" : "NormalConversion";
> +    die "Can't handle [EnforceRange] unless called from context that handles failures." if $intConversion eq "EnforceRange" and !$mayFail;

Maybe you can simply remove $mayFail because you want to throw anyway if $signature->extendedAttributes->{"EnforceRange"} is true. If you remove $mayFail, call sites of JSValueToNative() become much simpler.

> Source/WebCore/bindings/v8/V8Binding.cpp:267
> +        if (x < 0 || x > (kJSMaxInteger - 1)) {

Is this correct?

c.f. You are doing 'if (x < -(kJSMaxInteger - 1) || x > (kJSMaxInteger - 1))' in toInt64().

> Source/WebCore/bindings/v8/V8Binding.h:59
> +    const long long kJSMaxInteger = 0x20000000000000LL;

You can put it in V8Binding.cpp. Also this should be 0x1fffffffffffffLL. (Then you can remove '- 1' from V8Binding.cpp.)

> Source/WebCore/bindings/v8/V8BindingMacros.h:45
> +#define TYPECHECK_VOID(type, var, value, isolate) \
> +    type var;                                     \
> +    {                                             \
> +        bool ok = true;                           \
> +        var = (value);                            \
> +        if (UNLIKELY(!ok)) {                      \
> +            throwTypeError(0, isolate);           \
> +            return;                               \
> +        }                                         \
> +    }

'var = (value)' can throw, so you need to protect it with a TryCatch block. You can rename the macro to V8TRYCATCH_WITH_TYPECHECK_VOID().

[Joshua Bell]

(In reply to comment #60)
> Maybe you can simply remove $mayFail because you want to throw anyway if $signature->extendedAttributes->{"EnforceRange"} is true. If you remove $mayFail, call sites of JSValueToNative() become much simpler.

This was added to ensure that callers of JSValueToNative() were "opting in" to the new behavior and would then use the correct macro wrappers around the conversion. But I agree it's not required, so will remove it.

> > Source/WebCore/bindings/v8/V8Binding.cpp:267
> > +        if (x < 0 || x > (kJSMaxInteger - 1)) {
> 
> Is this correct?
> 
> c.f. You are doing 'if (x < -(kJSMaxInteger - 1) || x > (kJSMaxInteger - 1))' in toInt64().

Yes. Unless there's a typo I'm not seeing.

Spec has, for unsigned long long: If x < 0 or x > 2^53 − 1, then throw a TypeError.
Spec has, for long long: If x < −(2^53 − 1) or x > 2^53 − 1, then throw a TypeError.

Are you asking because it seems unusual for the upper bounds to be the same for both int64 and uint64 conversion? If so, the reason is that the maximum integer value exactly representable (due to the limited mantissa range) in an ECMAScript Number (a.k.a. a 64-bit IEEE754 float) is 2^53-1, which is less than either 2^64-1 (max uint64) or 2^63-1 (max int64). So the WebIDL conversion logic is not "is this number in the int64/uint64 range?" but "if I put the floor of this exact number into an int64/uint64 will I get the same number out again?"

If you're asking for another reason then I'm probably missing something, so please point it out!

> > Source/WebCore/bindings/v8/V8Binding.h:59
> > +    const long long kJSMaxInteger = 0x20000000000000LL;
> 
> You can put it in V8Binding.cpp. Also this should be 0x1fffffffffffffLL. (Then you can remove '- 1' from V8Binding.cpp.)

Done. The WebIDL spec uses "2^53 - 1" hence that style, but I'll move it and clarify the constant.

> > Source/WebCore/bindings/v8/V8BindingMacros.h:45
> > +#define TYPECHECK_VOID(type, var, value, isolate) \
> > +    type var;                                     \
> > +    {                                             \
> > +        bool ok = true;                           \
> > +        var = (value);                            \
> > +        if (UNLIKELY(!ok)) {                      \
> > +            throwTypeError(0, isolate);           \
> > +            return;                               \
> > +        }                                         \
> > +    }
> 
> 'var = (value)' can throw, so you need to protect it with a TryCatch block. You can rename the macro to V8TRYCATCH_WITH_TYPECHECK_VOID().

That's actually what I had prior to comment #30 from abarth@. But now that the call sites inspect EnforceRange I can revert - thanks!

[Joshua Bell]

Created attachment 192784 [details]
Patch

[Joshua Bell]

(In reply to comment #60)
> 
> 'var = (value)' can throw, so you need to protect it with a TryCatch block. You can rename the macro to V8TRYCATCH_WITH_TYPECHECK_VOID().

Fixing this macro also revealed a flaw in the previous patch where conversions that threw (e.g. { valueOf: function() { throw new Error('foo'); }) were eating the thrown error and throwing a TypeError instead. Updated the tests to double-check that the error thrown during "var = (value)" are what makes it out.

[Kentaro Hara]

Thanks for all the fixes!

(In reply to comment #61)
> Spec has, for unsigned long long: If x < 0 or x > 2^53 − 1, then throw a TypeError.
> Spec has, for long long: If x < −(2^53 − 1) or x > 2^53 − 1, then throw a TypeError.
> 
> Are you asking because it seems unusual for the upper bounds to be the same for both int64 and uint64 conversion?

Yes, I was asking for the reason. Thanks for the detailed clarification.

[Kentaro Hara]

Comment on attachment 192784 [details]
Patch

LGTM

[Erik Arvidsson]

Comment on attachment 192784 [details]
Patch

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

This looks great

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1904
> +            if ($parameter->extendedAttributes->{"EnforceRange"}) {
> +                my $value = JSValueToNative($parameter, $optional && $optional eq "DefaultIsNullString" ? "argumentOrNull(args, $paramIndex)" : "args[$paramIndex]", "args.GetIsolate()");
> +                $parameterCheckString .= "    V8TRYCATCH_WITH_TYPECHECK($nativeType, $parameterName, $value, args.GetIsolate());\n";
> +            } else {
> +                my $value = JSValueToNative($parameter, $optional && $optional eq "DefaultIsNullString" ? "argumentOrNull(args, $paramIndex)" : "args[$paramIndex]", "args.GetIsolate()");
> +                $parameterCheckString .= "    V8TRYCATCH($nativeType, $parameterName, $value);\n";
> +            }

Can you move the "my $value = ..." line out of the if to reduce code duplication?

[Joshua Bell]

(In reply to comment #66)
> Can you move the "my $value = ..." line out of the if to reduce code duplication?

Nice catch, done.

[Joshua Bell]

Created attachment 193338 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 193338 [details]
Patch for landing

Clearing flags on attachment: 193338

Committed r145929: <http://trac.webkit.org/changeset/145929>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Russell]

FYI: this caused a build failure on Windows which was fixed in http://trac.webkit.org/changeset/145975 .

[Joshua Bell]

(In reply to comment #71)
> FYI: this caused a build failure on Windows which was fixed in http://trac.webkit.org/changeset/145975 .

Thanks for the fix, sorry about the breakage.