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.
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.
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.
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.
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.
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.
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
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
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
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
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
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
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
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
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.
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.
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.
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
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
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
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
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
Comment on attachment 224204 [details] Patch v4 Clearing flags on attachment: 224204 Committed r164819: <http://trac.webkit.org/changeset/164819>
All reviewed patches have been landed. Closing bug.
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!
(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.
(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.
And my real point is that this patch introduced a regression!
(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.
(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.
(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.
(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