Bug 104147 – Math.{max, min}() must not return after first NaN value

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[André Bargull]

Steps to reproduce:

Test case:
---
js> Math.max(NaN, {valueOf:function(){throw "err"}})
NaN
js> Math.max(NaN, NaN, {valueOf:function(){throw "err"}})
NaN
---



Expected results:

Both calls should have thrown an uncaught exception, cf. [ES5.1 - 15.8.2, 15.8.2.11, 15.8.2.12]:
---
15.8.2  Function Properties of the Math Object
Each of the following Math object functions applies the ToNumber abstract operator to each of its arguments (in left-to-right order if there is more than one) and then performs a computation on the resulting Number value(s).

15.8.2.11  max ( [ value1 [ , value2 [ , … ] ] ] )
Given zero or more arguments, calls ToNumber on each of the arguments and returns the largest of the resulting values.

15.8.2.12  min ( [ value1 [ , value2 [ , … ] ] ] )
Given zero or more arguments, calls ToNumber on each of the arguments and returns the smallest of the resulting values.
---

So according to the spec, ToNumber needs to be called on each argument even if a `NaN` value was already found.

[Tibor Mészáros]

Created attachment 223868 [details]
Patch

According to the spec, ToNumber going to be called on each argument even if a `NaN` value was already found.

[Darin Adler]

Comment on attachment 223868 [details]
Patch

Code change looks great. We need to also add a test case; bug fixes for WebKit require regression tests go to with them.

[Filip Pizlo]

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

Your change needs a test.  Please include a test in your patch.

> Source/JavaScriptCore/runtime/MathObject.cpp:193
>          double val = exec->uncheckedArgument(k).toNumber(exec);
>          if (std::isnan(val)) {
>              result = QNaN;
> -            break;
>          }
>          if (val > result || (!val && !result && !std::signbit(val)))
>              result = val;

I wouldn't write this code this way.  You're relying on the comparison in the next 'if' statement falling through when result is NaN.  It's true that this will work, but it's subtle.  Subtle code just leads to more bugs later.

You could fix this with a comment above the second 'if' statement saying that it's written in a way that guarantees that it will fall through when result is NaN.

[Tibor Mészáros]

Created attachment 223974 [details]
Patch v2

I added tests into this patch, and an "else" statement. In this way it will falling through when result is NaN, and not necessary a description.

[WebKit Commit Bot]

Attachment 223974 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/MathObject.cpp:192:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/MathObject.cpp:207:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 223974 [details]
Patch v2

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

New failing tests:
js/math.html

[Build Bot]

Created attachment 223979 [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 223974 [details]
Patch v2

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

New failing tests:
js/math.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 223974 [details]
Patch v2

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

New failing tests:
js/math.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 223974 [details]
Patch v2

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

New failing tests:
js/math.html

[Build Bot]

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

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

[Darin Adler]

Comment on attachment 223974 [details]
Patch v2

The tests added look like fine minimal coverage of this change, but we also need to update the expected results in LayoutTests/js/math-expected.txt; please upload a new patch with that.

I also think the code change is incorrect. It looks like this code will continue to evaluate values even if ToNumber raises an exception. I don’t think that’s right. I think that once a ToNumber raises an exception, we should stop iterating through the rest of the arguments. The old code incorrectly had this behavior any time there is a NaN involved, but the new code incorrectly continues after an exception.

To test this we probably need test cases that involve side effects, not just exceptions. Here is one example (a bit clumsily written) that I think will show we have it wrong:

    sideEffect = 0;
    shouldThrow("Math.min({valueOf:function(){throw 'error1'}}, valueOf:function(){sideEffect = 1}})", "error1")
    shouldBe('sideEffect', '0');

I believe sideEffect will be 1.

[Tibor Mészáros]

Created attachment 224066 [details]
Patch v3

I added the tests that you mentioned, and all were passed, the SideEffect was 0, so I think it's working fine.

[WebKit Commit Bot]

Attachment 224066 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/MathObject.cpp:192:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/MathObject.cpp:207:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 224066 [details]
Patch v3

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

New failing tests:
js/math.html

[Build Bot]

Created attachment 224078 [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 224066 [details]
Patch v3

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

New failing tests:
js/math.html

[Build Bot]

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

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

[Tibor Mészáros]

Created attachment 224204 [details]
Patch v4

Style Fix:
Source/JavaScriptCore/runtime/MathObject.cpp:192: removed the newline before else
Source/JavaScriptCore/runtime/MathObject.cpp:207: removed the newline before else

Fix for expected:
math-expected.txt:112: added whitespace

[WebKit Commit Bot]

Comment on attachment 224204 [details]
Patch v4

Clearing flags on attachment: 224204

Committed r164819: <http://trac.webkit.org/changeset/164819>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

I still think this caused a regression.

It seems to me that we will keep calling toNumber on subsequent arguments even if an exception occurred when calling toNumber on the first argument. I am not sure why the regression test did not cover this case, but I see no way the code could be doing the right thing. We need to exit the loop when an exception occurs, and we used to do that as a side effect because we would exit the loop any time a NAN was returned.

I see the test case that is supposed to test this, and I don’t understand why it doesn’t fail. Someone should look in the debugger and find out!

[Oliver Hunt]

(In reply to comment #24)
> I still think this caused a regression.
> 
> It seems to me that we will keep calling toNumber on subsequent arguments even if an exception occurred when calling toNumber on the first argument. I am not sure why the regression test did not cover this case, but I see no way the code could be doing the right thing. We need to exit the loop when an exception occurs, and we used to do that as a side effect because we would exit the loop any time a NAN was returned.
> 
> I see the test case that is supposed to test this, and I don’t understand why it doesn’t fail. Someone should look in the debugger and find out!

Indeed, i suspect that we're short circuiting vm entry when there's an exception set.

That said at this point it occurs to me that we should just implement min/max as builtins.

[Darin Adler]

(In reply to comment #25)
> Indeed, i suspect that we're short circuiting vm entry when there's an exception set.

Got it.

> That said at this point it occurs to me that we should just implement min/max as builtins.

I suppose I’m more interested in constructing suitable test cases to demonstrate this bug that I spotted, rather than rushing to fix this implementation, which we might swiftly replace with a more efficient one.

[Darin Adler]

And my real point is that this patch introduced a regression!

[Tibor Mészáros]

(In reply to comment #27)
> And my real point is that this patch introduced a regression!

I had tested that problem that you talking about. What I found:

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

Solution: 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.

I hope this will be good solution.

[Darin Adler]

(In reply to comment #28)
> I had tested that problem that you talking about. What I found:
> 
> - If there is more exception, it will not stop at the first, but the return value will be the first exception.

That sounds right.

> Solution: 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.

Yes, this is the solution I had in mind.

[Darin Adler]

(In reply to comment #29)
> (In reply to comment #28)
> > I had tested that problem that you talking about. What I found:
> > 
> > - If there is more exception, it will not stop at the first, but the return value will be the first exception.
> 
> That sounds right.

I mean that it sounds like a correct description of test results that would demonstrate the problem that we introduced.

[Tibor Mészáros]

(In reply to comment #29)
> (In reply to comment #28)
> > I had tested that problem that you talking about. What I found:
> > 
> > - If there is more exception, it will not stop at the first, but the return value will be the first exception.
> 
> That sounds right.
> 
> > Solution: 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.
> 
> Yes, this is the solution I had in mind.

The patch for it: https://bugs.webkit.org/show_bug.cgi?id=129874