1287340 - Subclassing bound functions is wrong

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S [:evilpie]]

At least this is was https://github.com/tc39/test262/pull/720 claims.

Comment 1

[Eric Faust [:efaust]]

I'll take a look. Thanks for finding this.

Comment 2

[Eric Faust [:efaust]]

Attachment: Part 1: Add constructContentFunction to allow self-hosted code to pass new.target directly without calling Reflect.construct

Comment 3

[Eric Faust [:efaust]]

Attachment: Part 2: Fix Function.prototype.bind to correctly pass new.target through to bound functions.

This is noisy, but simple. We just have a lot of arms that are split out to avoid function call overhead. The test tries to exercise every case.

Comment 4

[Till Schneidereit [:till]]

Comment on attachment 8777991 [details] [diff] [review]
Part 1: Add constructContentFunction to allow self-hosted code to pass new.target directly without calling Reflect.construct

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

Looks good. Except for the browser_contextmenu.js changes, which don't seem to belong here.

Comment 5

[Till Schneidereit [:till]]

Comment on attachment 8777993 [details] [diff] [review]
Part 2: Fix Function.prototype.bind to correctly pass new.target through to bound functions.

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

Given that _IsConstructing isn't used anymore - and doesn't have any potential uses - r=me on ripping that out.

Also, it'd be really good to do at least some cursory benchmark testing of this. Jandem and I invested too much work in making this fast to now go and accidentally slow it down.

::: js/src/builtin/Function.js
@@ +306,4 @@
>        default:
>          assert(a.length !== 0,
>                 "bound function construction without args should be handled by caller");
> +        return _ConstructFunction(fun, newTarget, a);

I think this intrinsic should be renamed now. _VariadicConstructFunction, perhaps?

::: js/src/tests/ecma_6/Class/boundFunctionSubclassing.js
@@ +16,5 @@
> +// Check that we actually pass the proper new.target when calling super()
> +function toBind() { }
> +
> +var boundArgs = [];
> +for (let i = 0; i < 5; i++) {

Perhaps let this go to something like 50? We might increase the amount of unrolling we do at some point in the future, and we might mess it up.

Comment 6

[Tom S [:evilpie]]

Can this land?

Comment 7

[Eric Faust [:efaust]]

Attachment: Part 3: Flip new.target jit branching logic for perf increase

Make the most common use cases take fewer branches.

Comment 8

[Eric Faust [:efaust]]

Attachment: Part 4: Isolate extra branches in the constructing case to the constructing case for perf reasons

Comment 9

[Eric Faust [:efaust]]

These last two changes fix the perf problems I was seeing locally for the non-constructing case. The constructing case is still slightly slower, but it's just doing more work.

Comment 10

[Till Schneidereit [:till]]

Comment on attachment 8783587 [details] [diff] [review]
Part 4: Isolate extra branches in the constructing case to the constructing case for perf reasons

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

r=me

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/764d9165ede6
Part 1: Implement constructContentFunction for self-hosted code. (r=till)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8e8e8603716
Part 2: Improve new.target jit performance. (r=h4writer)
https://hg.mozilla.org/integration/mozilla-inbound/rev/56b2e554763a
Part 3: Properly forward new.target in bound function subclassing. (r=till)

Comment 12

[Eric Faust [:efaust]]

Part 2 and 4 landed together as part 3

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/764d9165ede6
https://hg.mozilla.org/mozilla-central/rev/c8e8e8603716
https://hg.mozilla.org/mozilla-central/rev/56b2e554763a