897027 - Missing ToNumber conversion for Math.pow()/Math.atan() when passing only one argument

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

Test case:
---
Math.pow({valueOf: () => print("hello")})
Math.atan2({valueOf: () => print("hello")})
---

Expected:
Both calls invoke the valueOf() function

Actual:
The valueOf() function is not invoked

Somewhat related to bug 896264, also see bug 896264 comment 4 for an explanation why ToNumber needs to be executed.

Comment 1

[Jeff Walden [:Waldo]]

David, you have any interest in fixing this perhaps?

Comment 2

[David Caabeiro [:dcaabeiro]]

Sure, feel free to assign me. Will try doing some work over this weekend.

Comment 3

[Jeff Walden [:Waldo]]

Great!  We probably should audit all the Math methods while we're at it -- might be more than just these two that need fixing.

Comment 4

[David Caabeiro [:dcaabeiro]]

Attachment: bug-897027-fix.patch

Apologies for the delay to get back to this. Please find a proposed fix attached, which should fix the bug.

The Math methods reported by André are the only ones having this problem (performed a check over the rest)

Any further comment or feedback would be appreciated.

Comment 5

[Till Schneidereit [:till]]

Comment on attachment 813138 [details] [diff] [review]
bug-897027-fix.patch

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

Stealing ...

Looks good, but I think we should have tests added for this. Can you create them from André's initial report? Ask me for review with those.

Comment 6

[David Caabeiro [:dcaabeiro]]

Attachment: bug-897027-fix.patch

This new patch includes the requested test cases.

Comment 7

[Till Schneidereit [:till]]

Comment on attachment 815216 [details] [diff] [review]
bug-897027-fix.patch

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

Nice!

r=me with a license header added to the test. Not all tests have them, but all should. Personally, I'd go with
// Any copyright is dedicated to the Public Domain.
// http://creativecommons.org/licenses/publicdomain/

::: js/src/tests/ecma_6/Math/20.2.2.ToNumber.js
@@ +2,5 @@
> + *   20.2.2 Function Properties of the Math Object
> + *
> + *   Each of the following Math object functions applies the ToNumber abstract operation 
> + *   to each of its arguments (in left-to-right order if there is more than one). 
> + *   If ToNumber returns an abrupt completion, that completion record is immediately returned. 

Nit: remove stray spaces at the last three lines' ends.

Comment 8

[David Caabeiro [:dcaabeiro]]

Attachment: bug-897027-fix.patch

New version with requested corrections.

Comment 9

[Till Schneidereit [:till]]

Comment on attachment 815315 [details] [diff] [review]
bug-897027-fix.patch

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

Perfect, thanks. Do you have checkin rights? If not, just add the checkin-needed keyword on the bug and the sheriffs will do their magic.

(Just for future reference, if you get an r+ with a request to do some small changes, you can set the r+ yourself on the new version and add something like "carrying r+ from tschneidereit" to the comment.)

Comment 10

[David Caabeiro [:dcaabeiro]]

Thanks for the tips! Will do. It was my understanding that checkin rights were given after some few contributions. Maybe the time has come :-)

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/774d6d582f40

In the future, please make sure your commit message has the bug number and a brief description of what the patch is doing (preferably not just the bug title). Thanks!

Comment 12

[Ed Morley [:emorley]]

Backed out for assertions:
eg https://tbpl.mozilla.org/php/getParsedLog.php?id=28934657&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/45f35cc52ceb

Were the jit-tests run locally on a debug build before requesting checkin-needed..? :-)

Comment 13

[Till Schneidereit [:till]]

Urgh, I'm utterly embarrassed. David, I take full responsibility for this one and am sorry for not doing a better job and catching it.

Can you debug things and attach a new version of the patch?

Comment 14

[André Bargull [:anba]]

(In reply to Till Schneidereit [:till] from comment #13)
> Can you debug things and attach a new version of the patch?

`ToNumber(cx, args[1], &y)` asserts when args.length() == 1.

Comment 15

[David Caabeiro [:dcaabeiro]]

Attachment: bug-897027-fix.patch

Thanks everyone for the comments. This update will hopefully fix the previous issue (tested it on my platform's debug and release builds)

Please note the local variables assignments: 

    double x = GenericNaN();

This should handle (and hopefully not introduce new bugs) the cases where less arguments are being passed (thus being undefined) according to my understanding of the draft. Otherwise please feel free to advise me of better alternatives.

Comment 16

[Till Schneidereit [:till]]

Sorry for the late response, this somehow fell off my radar.

Can you wield your new access level 1 powers and run this through try server? Paste the link to the run here and report back when results are in. And be forewarned: try server results aren't easy to read, because of intermittend failures.

Comment 17

[David Caabeiro [:dcaabeiro]]

Sure, that's a great idea. After some initial configuration I got this test set up https://tbpl.mozilla.org/?tree=Try&rev=97a05046b428

There are a few tests being restarted a few times (perhaps the intermittent failures you referred to?) and a couple of failures (Mochitest) which from what I see don't seem to have much to do with this patch. Other than that the other tests passed.

Would you mind taking a look and let me know if this looks ok?

Comment 18

[Till Schneidereit [:till]]

Comment on attachment 815582 [details] [diff] [review]
bug-897027-fix.patch

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

I'm terrribly sorry for the late review - this fell off my radar completely.

Looks great. And thank you so much for the thorough tests of Number functions.

::: js/src/jsmath.cpp
@@ +298,5 @@
>          return true;
>      }
>  
> +    double y = GenericNaN();
> +    if (args.hasDefined(0) && !ToNumber(cx, args[0], &y))

this hasDefined isn't necessary, as we'll have returned above if it were false

@@ +647,5 @@
>          return true;
>      }
>  
> +    double x = GenericNaN();
> +    if (args.hasDefined(0) && !ToNumber(cx, args[0], &x))

Same here about hasDefined

::: js/src/tests/ecma_6/Math/20.2.2.ToNumber.js
@@ +3,5 @@
> + * http://creativecommons.org/licenses/publicdomain/
> + */
> +
> +/*
> + *   20.2.2 Function Properties of the Math Object

Nit: can you add the date of the spec draft this numbering refers to. It will get outdated at some point, otherwise.

@@ +8,5 @@
> + *
> + *   Each of the following Math object functions applies the ToNumber abstract operation
> + *   to each of its arguments (in left-to-right order if there is more than one).
> + *   If ToNumber returns an abrupt completion, that completion record is immediately returned.
> + *   Otherwise, the function performs a computation on the resulting Number value(s).

Please just remove this entire paragraph: the EcmaScript spec's license isn't compatible with Public Domain. While this is very likely to fall under fair use, no need to go anywhere near the boundaries of that.

Comment 19

[Till Schneidereit [:till]]

Comment on attachment 815582 [details] [diff] [review]
bug-897027-fix.patch

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

Evilpie had a few comments on IRC that are very helpful:

You can just use args.get(0) and args.get(1) without checking the arg count. args.get returns undefined if fewer args were provided, and ToNumber returns GenericNaN for undefined. Doing this makes the function a lot simpler. I don't think it's worth it to return early for the 0-args case.

Canceling the review based on that, sorry for the churn.

Comment 20

[David Caabeiro [:dcaabeiro]]

Attachment: New fix with requested changes

Thanks for the comments and the last-minute help on IRC, much appreciated!

Here's the latest version containing the requested changes.

Comment 21

[Till Schneidereit [:till]]

Comment on attachment 8334005 [details] [diff] [review]
New fix with requested changes

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

This looks great, thank you!

Can you do a final try run (just one platform, with only the reftests is ok)? If that turns out green, set checkin-needed on the bug.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8334005 [details] [diff] [review]
New fix with requested changes

Please just set the checkin-needed bug keyword in the future :)

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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