Bug 129874 – Math.{max, min}() must return after first Exception value

• Status: NEW
• Component: JavaScriptCore

[Tibor Mészáros]

If there is more exception, it will not stop at the first, but the return value will be the first exception.

[Tibor Mészáros]

Created attachment 226105 [details]
Patch v1

If I add an if statement before the first if, we can check that there were any exception. If it was, drop a break, and the result will be this exception.

[Oliver Hunt]

It may be worth investigating implementing min/max in JS

[Tibor Mészáros]

(In reply to comment #2)
> It may be worth investigating implementing min/max in JS

I had tested that the original method will not stop at the first exception, but the return value will be the first. So the patch will fix this problem.

[Darin Adler]

Comment on attachment 226105 [details]
Patch v1

This fix looks just right. But we need to add a test case too, so someone doesn’t break it in the future.

[Tibor Mészáros]

(In reply to comment #4)
> (From update of attachment 226105 [details])
> This fix looks just right. But we need to add a test case too, so someone doesn’t break it in the future.

Hi!

I'm sorry, but there is no way to show sideEffect, because the toNumber will return 0.0, after an exception, so complex objects will return 0.0 after an exception...

references are there:

http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/runtime/JSObject.cpp?rev=164904&order=name#L1562

http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/runtime/JSObject.cpp?rev=164904&order=name#L1350

[Darin Adler]

(In reply to comment #5)
> I'm sorry, but there is no way to show sideEffect, because the toNumber will return 0.0, after an exception, so complex objects will return 0.0 after an exception...

That’s wrong. The issue is not about a return value. It’s about whether the valueOf code runs at all.

[Oliver Hunt]

Created attachment 226977 [details]
Patch

[Michael Saboff]

Comment on attachment 226977 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        This required fleching out a bit more of our builtins logic

*fleching*?

> Source/JavaScriptCore/builtins/Math.js:30
> +        value = +value;

Won't this mean that min(-0, +0) will return +0?  I think the spec says it should return -0.

> Source/JavaScriptCore/builtins/Math.js:47
> +        value = +value;

Won't this mean that min(-0, -1) will return +0?  I think the spec says it should return -0.

[Build Bot]

Comment on attachment 226977 [details]
Patch

Attachment 226977 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5475724035096576

New failing tests:
js/math.html

[Build Bot]

Created attachment 226998 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 226977 [details]
Patch

Attachment 226977 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4663751877853184

New failing tests:
js/math.html

[Build Bot]

Created attachment 227000 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 226977 [details]
Patch

Attachment 226977 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6182673236099072

New failing tests:
js/math.html

[Build Bot]

Created attachment 227002 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 226977 [details]
Patch

Attachment 226977 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4975311221424128

New failing tests:
js/math.html

[Build Bot]

Created attachment 227004 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Brent Fulgham]

Oliver: Is this something you plan on completing? Or should we close the bug as NTBF?

[Michael Catanzaro]

Comment on attachment 226977 [details]
Patch

Hi,

Apologies that your patch was not reviewed in a timely manner. Since it's now quite old, I am removing it from the review request queue. Please consider rebasing it on trunk and resubmitting.

To increase the chances of getting a review, consider using 'Tools/Scripts/webkit-patch upload --suggest-reviewers' to CC reviewers who might be interested in this bug.