818617 - Number.prototype.{toPrecision,toExponential} should accept out-of-range input for NaN/Infinity

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> Number.prototype.toExponential.call(NaN, 555)
typein:63: RangeError: precision 555 out of range
js> Number.prototype.toPrecision.call(NaN, 555)
typein:64: RangeError: precision 555 out of range
---



Expected results:

According to [15.7.4.6  Number.prototype.toExponential (fractionDigits)] step 4 resp. step 6 and according to [15.7.4.7  Number.prototype.toPrecision (precision)] step 4 resp. step 7, toExponential()/toPrecision() return "NaN" (or "Infinity") if the this-argument is `NaN` (or `Infinity`). The actual range check on the input argument happens after (!) handling `NaN`/`Infinity`.

Comment 1

[Jeff Walden [:Waldo]]

Yup, the steps clearly say if |this| is NaN, return "NaN".

If I remember correctly, toPrecision and toExponential (and probably toFixed as well) all have crazy non-spec-aligned implementations.  (In other words, they're not broken down into /* Step 1. */, /* Steps 2-4. */, etc. parts.)  We should probably fix that in the process of fixing this -- get out all the dry rot at once.

Comment 2

[André Bargull [:anba]]

Since you've mentioned `toFixed()`: toFixed() is spec'ed to throw a RangeError if the `fractionDigits` argument is out of range, even if the |this| value is NaN.

Comment 3

[Rajinder Yadav]

Hello I would like to work on this as my first bug, can I be assigned a mentor to help me get going.

Comment 4

[Tooru Fujisawa [:arai]]

Adding some links.

This bug is about the implementation of num_toExponential_impl [1] and num_toPrecision_impl [2], that corresponds to Number.prototype.toExponential [3] and Number.prototype.toPrecision [4] in the spec.

[1] https://dxr.mozilla.org/mozilla-central/rev/23dc78b7b57e9f91798ea44c242a04e112c37db0/js/src/jsnum.cpp#941
[2] https://dxr.mozilla.org/mozilla-central/rev/23dc78b7b57e9f91798ea44c242a04e112c37db0/js/src/jsnum.cpp#967
[3] https://tc39.github.io/ecma262/#sec-number.prototype.toexponential
[4] https://tc39.github.io/ecma262/#sec-number.prototype.toprecision

Comment 5

[Rajinder Yadav]

Attachment: bug818617-01.patch

Please review bug818617-01.patch

Comment 6

[Tooru Fujisawa [:arai]]

Comment on attachment 8769457 [details] [diff] [review]
bug818617-01.patch

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

Thank you for your patch :D

Please address the following comments, and post updated patch.

::: js/src/jsnum.cpp
@@ +941,5 @@
>  num_toExponential_impl(JSContext* cx, const CallArgs& args)
>  {
>      MOZ_ASSERT(IsNumber(args.thisv()));
>  
> +    double d = Extract(args.thisv());

while you're here, can you add spec step comments?
like
  // Step 1.
before the above line, and same for other steps you added?
Also, please add the following comment before the function definition:

// ES 2017 draft rev f8a9be8ea4bd97237d176907a1e3080dce20c68f 20.1.3.2.

(f8a9be8ea4bd97237d176907a1e3080dce20c68f corresponds to the revision of the draft spec, that is used for generating the spec HTML file)

That will help us finding missed/swapped steps, and also helps when the spec is updated :)

@@ +943,5 @@
>      MOZ_ASSERT(IsNumber(args.thisv()));
>  
> +    double d = Extract(args.thisv());
> +
> +    if (mozilla::IsNaN(d)) {

So, before this line, please add:
  // Step 4.

and you'll notice that we don't have step 2 and step 3 between 1 and 4.

> 2. Let f be ? ToInteger(fractionDigits).
> 3. Assert: f is 0, when fractionDigits is undefined.

Step 3 is just an assertion, so the order doesn't matter for actual execution, but we'd better adding assertion here:

  // Step 3.
  MOZ_ASSERT_IF(pre-condition here, assertion condition here);


step 2 should be done before step 4, as the operation `ToInteger` has side effect, and may throw an exception.

for example, the following code should throw "hello", instead of returning "NaN":

  Number.prototype.toExponential.call(NaN, { valueOf() { throw "hello"; } })

another case, the following code should print 20:

  var x = 10;
  Number.prototype.toExponential.call(NaN, { valueOf() { x = 20; return 1; } });
  console.log(x);

ToInteger is performed in `ComputePrecisionInRange` [1], so you'll need to move the code out of the function, and pass `double` variable instead of `HandleValue`.

> static bool
> ComputePrecisionInRange(JSContext* cx, int minPrecision, int maxPrecision, HandleValue v,
>                         int* precision)
> {
>     double prec;
>     if (!ToInteger(cx, v, &prec))
>         return false;


Also, please add corresponding testcase.

[1] https://dxr.mozilla.org/mozilla-central/rev/23dc78b7b57e9f91798ea44c242a04e112c37db0/js/src/jsnum.cpp#884

@@ +950,5 @@
> +    }
> +
> +    if (mozilla::IsInfinite(d)) {
> +        StringBuffer sb(cx);
> +        if (d < 0) {

We could flip this condition and just pass cx->names().Infinity to setString, and early return here.
So we don't need to allocate StringBuffer for positive infinity case.

@@ +951,5 @@
> +
> +    if (mozilla::IsInfinite(d)) {
> +        StringBuffer sb(cx);
> +        if (d < 0) {
> +            if (!sb.append("-")) return false;

Please move "return false;" to next line.
same for other "return false;" too.

@@ +954,5 @@
> +        if (d < 0) {
> +            if (!sb.append("-")) return false;
> +        }
> +
> +        if (!sb.append(cx->names().Infinity.get())) return false;

You don't need ".get()".
ImmutablePropertyNamePtr can be converted to JSLinearString* implicitly.

@@ +985,5 @@
>      return CallNonGenericMethod<IsNumber, num_toExponential_impl>(cx, args);
>  }
>  
>  MOZ_ALWAYS_INLINE bool
>  num_toPrecision_impl(JSContext* cx, const CallArgs& args)

basically, please apply same fix as num_toExponential_impl for num_toPrecision_impl ("return false;" position, step numbers, etc)

@@ +991,5 @@
>      MOZ_ASSERT(IsNumber(args.thisv()));
>  
>      double d = Extract(args.thisv());
>  
> +    if (mozilla::IsNaN(d)) {

this is step 4, and we don't have step 2 and step 3 before.

> 2. If precision is undefined, return ! ToString(x).
> 3. Let p be ? ToInteger(precision).

step 2 is done below in same function, so please move the lines before this:

>     if (!args.hasDefined(0)) {
>         JSString* str = NumberToStringWithBase<CanGC>(cx, d, 10);
>         if (!str) {
>             JS_ReportOutOfMemory(cx);
>             return false;
>         }
>         args.rval().setString(str);
>         return true;
>     }

(step 2 may throw only for "out of memory" tho)

step 3 is done in `ComputePrecisionInRange` here too, so please apply same fix as toExponential's case.

::: js/src/tests/ecma_3/Number/15.7.4.6-1.js
@@ -48,5 @@
>  actual = Number.NaN.toExponential(-3);
>  expect = 'NaN';
>  captureThis();
>  
> -

this line is accidentally removed.
can you revert this change?

::: js/src/tests/ecma_6/Number/20.1.3.2-toExponential.js
@@ +3,5 @@
> +
> +//-----------------------------------------------------------------------------
> +
> +var BUGNUMBER = 818617;
> +var summary = "ECMAScript 2017 Draft ECMA-262 Section 20.1.3.2: Number.prototype.toExponentia";

typo: toExponentia => toExponential

@@ +11,5 @@
> +/**************
> + * BEGIN TEST *
> + **************/
> +
> +// ECMAScript 2017 Draft ECMA-262 Section 20.1.3.2: Number.prototype.toExponential with NaN fractionDigits

you could remove redundant part "ECMAScript 2017 Draft ECMA-262 Section 20.1.3.2: Number.prototype.toExponential" from each comment, as it's already in summary string.
Also, we're using 79-chars per line for comment, and 99-chars per line for code.

@@ +13,5 @@
> + **************/
> +
> +// ECMAScript 2017 Draft ECMA-262 Section 20.1.3.2: Number.prototype.toExponential with NaN fractionDigits
> +// Out of range
> +assertEq('NaN', Number.prototype.toExponential.call(NaN, 555));

can you swap 1st and 2nd arguments?
1st is "actual value" and 2nd is "expected value".

https://dxr.mozilla.org/mozilla-central/rev/23dc78b7b57e9f91798ea44c242a04e112c37db0/js/src/shell/js.cpp#5306
> "assertEq(actual, expected[, msg])",

same for other tests.

::: js/src/tests/ecma_6/Number/20.1.3.2-toPrecision.js
@@ +13,5 @@
> + **************/
> +
> +// ECMAScript 2017 Draft ECMA-262 Section 20.1.3.5: Number.prototype.toPrecision with NaN fractionDigits
> +// Out of range
> +assertEq('NaN', Number.prototype.toPrecision.call(NaN, 555));

please swap arguments in this file too

Comment 7

[Rajinder Yadav]

Attachment: bug818617-01.patch

Please review patch with feedback updates.

Comment 8

[Tooru Fujisawa [:arai]]

Comment on attachment 8769466 [details] [diff] [review]
bug818617-01.patch

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

Thank you again!
It's almost there :)

please address the following comment, mostly cosmetic change.

::: js/src/jsnum.cpp
@@ +884,1 @@
>                          int* precision)

Can you unwrap these arguments to fit to 99-characters per line?

@@ +923,5 @@
>      if (args.length() == 0) {
>          precision = 0;
>      } else {
> +      double prec = 0;
> +      if (!ToInteger(cx, args[0], &prec)) return false;

here 2-spaces indent is used.
please add 2 more spaces for above 2 lines.

Also, please move "return false;" to next line.

@@ +944,4 @@
>  MOZ_ALWAYS_INLINE bool
>  num_toExponential_impl(JSContext* cx, const CallArgs& args)
>  {
> +    // Step 1: Let d be ? thisNumberValue(this value).

It's not necessary to add spec step text ("Let d be ...") if it's clear from the code.
can you remove them and replace with "// Step 1." etc?

@@ +966,5 @@
> +        return true;
> +    }
> +
> +    // Steps 5 - 7: If d is +Infinity or -Infinity
> +    //              return String "Infinity" or "-Infinity".

Can you use "// Steps 5-7." instead to follow other places?

@@ +968,5 @@
> +
> +    // Steps 5 - 7: If d is +Infinity or -Infinity
> +    //              return String "Infinity" or "-Infinity".
> +    if (mozilla::IsInfinite(d)) {
> +        if (d >= 0) {

"=" can be removed for infinity.

@@ +988,5 @@
> +        args.rval().setString(str);
> +        return true;
> +    }
> +
> +    // Steps 8 - 15

Here too.
  // Steps 8-15.

Comment 9

[Rajinder Yadav]

Attachment: bug818617-01.patch

Please review patch with feedback updates.

Comment 10

[Tooru Fujisawa [:arai]]

Comment on attachment 8769469 [details] [diff] [review]
bug818617-01.patch

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

Thanks!

::: js/src/jsnum.cpp
@@ +876,5 @@
>  
>  static const unsigned MAX_PRECISION = 100;
>  
>  static bool
> +ComputePrecisionInRange(JSContext* cx, int minPrecision, int maxPrecision, double prec, int* precision)

Sorry, my comment was unclear.
this line is now 103 chars.
the last "int* precision" should be in next line.

Comment 11

[Rajinder Yadav]

Attachment: bug818617-01.patch

Patch corrected, please review.

Comment 12

[Tooru Fujisawa [:arai]]

Comment on attachment 8769472 [details] [diff] [review]
bug818617-01.patch

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

Excellent!  thank you!

Here's try run (build + automated tests) for your patch:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3e7f8c2d91b

after it passed all tests, this patch can be landed.

Comment 13

[Tooru Fujisawa [:arai]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3e7f8c2d91b&selectedJob=23604584
> REFTEST TEST-UNEXPECTED-PASS | file:///home/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=ecma_3/Number/15.7.4.6-1.js | Section A of test: no error intended! item 1
> REFTEST TEST-UNEXPECTED-PASS | file:///home/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=ecma_3/Number/15.7.4.6-1.js | Section B of test: Infinity.toExponential() with out-of-range fractionDigits item 2
> REFTEST TEST-UNEXPECTED-PASS | file:///home/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=ecma_3/Number/15.7.4.6-1.js | Section C of test: -Infinity.toExponential() with out-of-range fractionDigits item 3
> REFTEST TEST-UNEXPECTED-PASS | file:///home/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=ecma_3/Number/15.7.4.6-1.js | Section D of test: NaN.toExponential() with out-of-range fractionDigits item 4
> REFTEST TEST-UNEXPECTED-PASS | file:///home/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=ecma_3/Number/15.7.4.7-1.js | Section A of test: no error intended! item 1
> REFTEST TEST-UNEXPECTED-PASS | file:///home/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=ecma_3/Number/15.7.4.7-1.js | Section B of test: Infinity.toPrecision() with out-of-range fractionDigits item 2
> REFTEST TEST-UNEXPECTED-PASS | file:///home/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=ecma_3/Number/15.7.4.7-1.js | Section C of test: -Infinity.toPrecision() with out-of-range fractionDigits item 3
> REFTEST TEST-UNEXPECTED-PASS | file:///home/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=ecma_3/Number/15.7.4.7-1.js | Section D of test: NaN.toPrecision() with out-of-range fractionDigits item 4

Apparently, we already have testcases for this bug, and they're marked as `|reftest| fails`, but your patch fixed all of them :)
(and they're missed in jstests because of the behavior difference between jstests and jsreftest...)

  https://dxr.mozilla.org/mozilla-central/source/js/src/tests/ecma_3/Number/15.7.4.6-1.js
  https://dxr.mozilla.org/mozilla-central/source/js/src/tests/ecma_3/Number/15.7.4.7-1.js

So, can you remove the following line from those 2 files?

  // |reftest| fails

about many `TypeError: Assertion failed: got "false", expected "true: must be a current function to examine` in the log, they're not related to this patch, so please ignore them.

Comment 14

[Rajinder Yadav]

Attachment: bug818617-01.patch

Nice to see my first bug fix about to land :D
I've updated the patch, please review!

Comment 15

[Tooru Fujisawa [:arai]]

Comment on attachment 8769507 [details] [diff] [review]
bug818617-01.patch

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

Thank you again!

here's try run.
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1a83be853f8

Comment 16

[Tooru Fujisawa [:arai]]

try run is all green :)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78aede3bf18d
Number.prototype.{toPrecision,toExponential} should accept out-of-range input for NaN/Infinity. r=arai

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/78aede3bf18d