818620 - Math.{max, min}() must not return after first NaN value

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0
Build ID: 20121119183901

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.

Comment 1

[Jeff Walden [:Waldo]]

Good call.  Should be a pretty easy first bug for anyone interested in it.

There's a small chance there's some JIT-magic that might need to be changed here, but I suspect we're only optimizing in JITs (away from just calling the actual C++ function) when Math.min/max is called with all-numeric arguments.  Might be best to add a feedback? from some JIT person as a double-check.

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #1)
> There's a small chance there's some JIT-magic that might need to be changed
> here, but I suspect we're only optimizing in JITs (away from just calling
> the actual C++ function) when Math.min/max is called with all-numeric
> arguments.  Might be best to add a feedback? from some JIT person as a
> double-check.

Yeah, the JIT's inline Math.max/Math.min with exactly two arguments, and both arguments must be known to be int32 or double.

Comment 3

[James Kitchener (:jkitch)]

Attachment: patch

This still performs redundant comparisons on non-NaN values should a NaN be present, but adding a test to avoid this made it slightly slower on SunSpider.

Comment 4

[Alexandre BM (:rednaks)]

I may have a better solution but I don't know if I can attach it !

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 690009 [details] [diff] [review]
patch

Review of attachment 690009 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay here, been heads-down on my own patches, and on trying to keep up with other people's patches...  :-(

So, semantically, I think your changes are correct.  But whenever you're making changes to an algorithm, it's always worth looking to see if it can be improved.  Sometimes exceptional cases have to be exceptions building upon the existing algorithm.  But other times, they can be folded into the rest of it.

Spurred on by your patch's ideas, I've scribbled out reimplementations that I think address the bug here, and simplify things.  Any chance you could test these algorithms and see if they work, and/or make suggestions for improvements if you see them?

(The CallArgs bit is definitely tangential: it's a new abstraction we're slowly introducing into the engine, with the goal of getting rid of argc/vp eventually.  Newer code tends to use it; older code doesn't, unless it's been touched recently.  This code's older because it doesn't use it and because it uses JS_TRUE and JS_FALSE rather than C++ true/false.)

::: js/src/jsmath.cpp
@@ +390,1 @@
>      return JS_TRUE;

CallArgs args = CallArgsFromVp(argc, vp);

double x;
double maxval = MOZ_DOUBLE_NEGATIVE_INFINITY();
for (unsigned i = 0; i < args.length(); i++) {
    if (!ToNumber(cx, args[i], &x))
        return false;
    // Math.max(num, NaN) => NaN, Math.max(-0, +0) => +0
    if (x > maxval || MOZ_DOUBLE_IS_NaN(x) || (x == maxval && MOZ_DOUBLE_IS_NEGATIVE(maxval)))
        maxval = x;
}
args.rval().setNumber(maxval);
return true;

@@ +414,5 @@
>                  z = x;
>          } else {
>              z = (x < z) ? x : z;
>          }
>      }

CallArgs args = CallArgsFromVp(argc, vp);

double x;
double minval = MOZ_DOUBLE_POSITIVE_INFINITY();
for (unsigned i = 0; i < args.length(); i++) {
    if (!ToNumber(cx, args[i], &x))
        return false;
    // Math.min(num, NaN) => NaN, Math.min(-0, +0) => -0
    if (x < minval || MOZ_DOUBLE_IS_NaN(x) || (x == minval && MOZ_DOUBLE_IS_NEGATIVE_ZERO(x)))
        minval = x;
}
args.rval().setNumber(minval);
return true;

Comment 6

[James Kitchener (:jkitch)]

Attachment: patch v2

The functions have been replaced as specified, with one minor whitespace change to avoid >80 char lines.

I've checked that it passes all tests and compared it in SunSpider.
prior patch: 267.5ms +/- 1.3%
this patch : 265.2ms */- 0.8%

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 692911 [details] [diff] [review]
patch v2

Review of attachment 692911 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, modulo a style nit.  Thanks!  Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6e9ba8a6a20

::: js/src/jsmath.cpp
@@ +386,5 @@
> +        if (!ToNumber(cx, args[i], &x))
> +            return false;
> +        // Math.max(num, NaN) => NaN, Math.max(-0, +0) => +0
> +        if (x > maxval || MOZ_DOUBLE_IS_NaN(x) ||
> +            (x == maxval && MOZ_DOUBLE_IS_NEGATIVE(maxval)))

I assume this was the one whitespace change to avoid 80+-character lines?

The rule in JS isn't actually 80ch at this point -- it's 99ch for code (not 100, because some deficient editors, in a 100ch terminal, will show a line-continuation character at the last position and spill a 100ch line to two lines).  It's still 79ch, or maybe 80ch, for comments, but this isn't a comment.  So this should be on one line, because it's 98ch.

(If this were to spill over to two lines, we would add braces around the if-body, opening '{' on a new line aligned with the 'i' in 'if'.  We add braces when either the condition or body is multiple lines.  Just for future reference.  :-) )

@@ +405,5 @@
> +        if (!ToNumber(cx, args[i], &x))
> +            return false;
> +        // Math.min(num, NaN) => NaN, Math.min(-0, +0) => -0
> +        if (x < minval || MOZ_DOUBLE_IS_NaN(x) ||
> +            (x == minval && MOZ_DOUBLE_IS_NEGATIVE_ZERO(x)))

Same here.

Comment 8

[:Benjamin Peterson]

Could use a testcase, too.

Comment 9

[James Kitchener (:jkitch)]

Attachment: testcase

Comment 10

[:Benjamin Peterson]

Comment on attachment 693815 [details] [diff] [review]
testcase

Review of attachment 693815 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/basic/maxConvertAllArgs.js
@@ +4,5 @@
> +try {
> +    Math.max(NaN, {valueOf:function(){throw "err"}})
> +} catch (e) {
> +    caught = true;
> +}

If you put

load(libdir + "asserts.js");

at the top, then you can use a function called assertThrowsInstanceOf, which does this for you.

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b6e9ba8a6a20

Comment 12

[James Kitchener (:jkitch)]

Attachment: testcase v2

now uses assertThrowsInstanceOf

Comment 13

[:Benjamin Peterson]

That looks good. Thank you.

https://hg.mozilla.org/integration/mozilla-inbound/rev/978ca52298f0

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/978ca52298f0