DerivedSources/WebKitTestRunner/JSAccessibilityUIElement.cpp:802:72: runtime error: -1 is outside the range of representable values of type 'unsigned int'

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

DerivedSources/WebKitTestRunner/JSAccessibilityUIElement.cpp:802:72: runtime error: -1 is outside the range of representable values of type 'unsigned int'.

[Chris Dumez]

Created attachment 424049 [details]
Patch

[Chris Dumez]

   1 Release/DerivedSources/WebKitTestRunner/JSEventSendingController.cpp:274:45: runtime error: nan is outside the range of representable values of type 'int'
   1 Release/DerivedSources/WebKitTestRunner/JSEventSendingController.cpp:130:23: runtime error: nan is outside the range of representable values of type 'int'
   1 Release/DerivedSources/WebKitTestRunner/JSAccessibilityUIElement.cpp:802:72: runtime error: -1 is outside the range of representable values of type 'unsigned int'
   1 Release/DerivedSources/WebKitTestRunner/JSAccessibilityUIElement.cpp:802:64: runtime error: -1 is outside the range of representable values of type 'unsigned int'
   1 Release/DerivedSources/WebKitTestRunner/JSAccessibilityUIElement.cpp:1571:63: runtime error: 1e+11 is outside the range of representable values of type 'int'
   1 Release/DerivedSources/WebKitTestRunner/JSAccessibilityUIElement.cpp:1221:67: runtime error: -1 is outside the range of representable values of type 'unsigned int'

[Darin Adler]

What does clampTo do for NaN? Is there any downside to silently clamping instead of doing something like raising an exception?

[Chris Dumez]

(In reply to Darin Adler from comment #3)
> What does clampTo do for NaN? Is there any downside to silently clamping
> instead of doing something like raising an exception?

I will check what it does for NaN, I am not sure. Previously, I believe it was undefined behavior to cast NaN to an integer type.

About clamping vs throwing an exception, I guess we can do whatever we decide to, since this is used only by layout tests. I went this way because it was a bit less work and basically as permissive as it used to be.

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> (In reply to Darin Adler from comment #3)
> > What does clampTo do for NaN? Is there any downside to silently clamping
> > instead of doing something like raising an exception?
> 
> I will check what it does for NaN, I am not sure. Previously, I believe it
> was undefined behavior to cast NaN to an integer type.

We seem to be calling:
// Floating point source.
template<typename TargetType, typename SourceType>
typename std::enable_if<!std::is_same<TargetType, SourceType>::value
    && std::is_floating_point<SourceType>::value
    && !(std::is_floating_point<TargetType>::value && sizeof(TargetType) > sizeof(SourceType)), TargetType>::type
clampTo(SourceType value, TargetType min = defaultMinimumForClamp<TargetType>(), TargetType max = defaultMaximumForClamp<TargetType>())
{
    if (value >= static_cast<SourceType>(max))
        return max;
    if (value <= static_cast<SourceType>(min))
        return min;
    return static_cast<TargetType>(value);
}

If value is NaN, then I'd expect both if checks to fail and we'd static_cast<> to the destination type, which I believe would still be undefined behavior for integer destination types.
We may need to update std::clampTo() to handle NaN. Looks like the default behavior for WebIDL is:
"If x is NaN, +0, +∞, or −∞, then return +0."

[Chris Dumez]

Created attachment 424067 [details]
Patch

[Darin Adler]

We should consider fixing clampTo so that it returns "min" when NaN is passed instead of returning "value". I think we can do that efficiently. Could be as simple as:

    if (!(value > static_cast<SourceType>(min)))
        return min;

Instead of the "<=" version. That might fix it.

[Chris Dumez]

(In reply to Darin Adler from comment #7)
> We should consider fixing clampTo so that it returns "min" when NaN is
> passed instead of returning "value". I think we can do that efficiently.
> Could be as simple as:
> 
>     if (!(value > static_cast<SourceType>(min)))
>         return min;
> 
> Instead of the "<=" version. That might fix it.

I had a slight preference for 0 but I guess it doesn't really matter. At least your way avoids an extra branch. I'll update the patch accordingly and add API tests.

[Darin Adler]

Yes, I see your point. 0 would be nicer.

[Darin Adler]

But maybe 0 isn’t even between min and max, so we’d want to convert to 0 before doing the min/max checks.

[Chris Dumez]

(In reply to Darin Adler from comment #10)
> But maybe 0 isn’t even between min and max, so we’d want to convert to 0
> before doing the min/max checks.

Yes, that is what my patch was doing:
```
if (std::isnan(value))
   value = 0;
if (value >= max)
   return max;
[...]
```

[Chris Dumez]

Created attachment 424074 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #12)
> Created attachment 424074 [details]
> Patch

This version returns min as you initially suggested and adds API test coverage. I can easily switch to 0 (then clamped to range) if you'd prefer.

[EWS]

Committed r274919: <https://commits.webkit.org/r274919>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424074 [details].

[Radar WebKit Bug Importer]

<rdar://problem/75763224>