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

RESOLVED FIXED in mozilla28

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: anba, Assigned: dcaabeiro)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

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

5 years ago
David, you have any interest in fixing this perhaps?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(david.caabeiro)
(Assignee)

Comment 2

5 years ago
Sure, feel free to assign me. Will try doing some work over this weekend.
Flags: needinfo?(david.caabeiro)

Comment 3

5 years ago
Great!  We probably should audit all the Math methods while we're at it -- might be more than just these two that need fixing.
Assignee: general → david.caabeiro
Status: NEW → ASSIGNED
(Assignee)

Comment 4

4 years ago
Created attachment 813138 [details] [diff] [review]
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.
Attachment #813138 - Flags: review?(jwalden+bmo)
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.
Attachment #813138 - Flags: review?(jwalden+bmo) → feedback+
(Assignee)

Comment 6

4 years ago
Created attachment 815216 [details] [diff] [review]
bug-897027-fix.patch

This new patch includes the requested test cases.
Attachment #813138 - Attachment is obsolete: true
Attachment #815216 - Flags: review?(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.
Attachment #815216 - Flags: review?(till) → review+
(Assignee)

Comment 8

4 years ago
Created attachment 815315 [details] [diff] [review]
bug-897027-fix.patch

New version with requested corrections.
Attachment #815216 - Attachment is obsolete: true
Attachment #815315 - Flags: review?(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.)
Attachment #815315 - Flags: review?(till) → review+
(Assignee)

Comment 10

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

Updated

4 years ago
Keywords: checkin-needed
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!
Flags: in-testsuite+
Keywords: checkin-needed
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..? :-)
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?
(Reporter)

Comment 14

4 years ago
(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.
(Assignee)

Comment 15

4 years ago
Created attachment 815582 [details] [diff] [review]
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.
Attachment #815315 - Attachment is obsolete: true
Attachment #815582 - Flags: review?(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.
(Assignee)

Comment 17

4 years ago
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 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.
Attachment #815582 - Flags: review?(till) → review+
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.
Attachment #815582 - Flags: review+
(Assignee)

Comment 20

4 years ago
Created attachment 8334005 [details] [diff] [review]
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.
Attachment #815582 - Attachment is obsolete: true
Attachment #8334005 - Flags: review?(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.
Attachment #8334005 - Flags: review?(till) → review+
(Assignee)

Updated

4 years ago
Attachment #8334005 - Flags: checkin?(till)
Comment on attachment 8334005 [details] [diff] [review]
New fix with requested changes

Please just set the checkin-needed bug keyword in the future :)
Attachment #8334005 - Flags: checkin?(till) → checkin+
https://hg.mozilla.org/mozilla-central/rev/afd90049af4c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.