[webkit-reviews] review canceled: [Bug 182984] Crash under JSC::JSCell::toNumber(JSC::ExecState*) : [Attachment 334301] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has canceled Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 182984: Crash under JSC::JSCell::toNumber(JSC::ExecState*)
https://bugs.webkit.org/show_bug.cgi?id=182984

Attachment 334301: Patch

https://bugs.webkit.org/attachment.cgi?id=334301&action=review




--- Comment #4 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 334301
  --> https://bugs.webkit.org/attachment.cgi?id=334301
Patch

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

>> Source/WebCore/ChangeLog:15
>> +	    No new tests, extended new test.
> 
> Did you mean "extended existing test"?

Yep. Will fix.

>> Source/WebCore/bindings/js/JSDOMConvertNumbers.h:388
>>	}
> 
> If you look in JSCJSValue.h, you'll see that jsNumber() can also take integer
values.  Is IDLUnrestrictedDouble::ImplementationType guaranteed to be a
double?  If not, then maybe you should have 2 versions of convert: one for
integral types and one for double.

AFAICT IDLUnrestrictedDouble::ImplementationType has to be a double.

>> Source/WebCore/testing/TypeConversions.h:147
>> +	double testQuietNaNUnrestrictedDouble() const { return
std::numeric_limits<double>::quiet_NaN(); }
> 
> Maybe you should add a case for PureNaN as well just to make sure that it is
also working as expected.

Good point, will add.