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

RESOLVED FIXED in mozilla20

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: anba, Assigned: jkitch)

Tracking

Trunk
mozilla20
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t][lang=c++][mentor=jwalden])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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

6 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [js:t][lang=c++][mentor=jwalden]
(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.
(Assignee)

Comment 3

6 years ago
Created attachment 690009 [details] [diff] [review]
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.
Assignee: general → jkitch.bug
Attachment #690009 - Flags: review?(jwalden+bmo)
I may have a better solution but I don't know if I can attach it !

Comment 5

6 years ago
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;
Attachment #690009 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 6

6 years ago
Created attachment 692911 [details] [diff] [review]
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%
Attachment #690009 - Attachment is obsolete: true
Attachment #692911 - Flags: review?(jwalden+bmo)

Comment 7

6 years ago
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.
Attachment #692911 - Flags: review?(jwalden+bmo) → review+

Comment 8

6 years ago
Could use a testcase, too.
(Assignee)

Comment 9

6 years ago
Created attachment 693815 [details] [diff] [review]
testcase

Comment 10

6 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/b6e9ba8a6a20
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
(Assignee)

Comment 12

6 years ago
Created attachment 695463 [details] [diff] [review]
testcase v2

now uses assertThrowsInstanceOf
Attachment #693815 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.