Will have to run a benchmark to see performance impact...

Would be good to benchmark this as well. You beat me to it!

Would be good to benchmark this as well.

Working on it...

CI: https://ci.nodejs.org/job/node-test-pull-request/8003/

FWIW it used to be common with this sort of code to put at the top of the file:

var apply = (function(){}).apply;

And then use that (as it has no dependencies on even someone overriding the global Function)

Looks good now :)

Had to alter the implementation a bit from the initial attempt to get the benchmark to be neutral. Current implementation in this PR compares like this against master:

                                                    improvement confidence    p.value
 timers/immediate.js type="breadth" thousands=2000      -0.43 %            0.67056272
 timers/immediate.js type="breadth1" thousands=2000      0.10 %            0.92761906
 timers/immediate.js type="breadth4" thousands=2000     -1.59 %            0.19085909
 timers/immediate.js type="clear" thousands=2000         0.11 %            0.89212673
 timers/immediate.js type="depth" thousands=2000        -1.63 %            0.07178678
 timers/immediate.js type="depth1" thousands=2000        0.90 %            0.35150859
 timers/set-immediate-breadth-args.js millions=5         0.49 %            0.50576472
 timers/set-immediate-breadth.js millions=10            -0.34 %            0.36237802
 timers/set-immediate-depth-args.js millions=10         -1.44 %            0.10138701
 timers/set-immediate-depth.js millions=10              -0.81 %            0.42620317
 timers/timers-breadth.js thousands=500                 -0.18 %            0.80576168
 timers/timers-cancel-pooled.js thousands=500            1.10 %            0.27800863
 timers/timers-cancel-unpooled.js thousands=100          0.31 %            0.22503088
 timers/timers-depth.js thousands=1                      0.02 %            0.95753131
 timers/timers-insert-pooled.js thousands=500           -0.21 %            0.85337983
 timers/timers-insert-unpooled.js thousands=100          0.06 %            0.93189629
 timers/timers-timeout-pooled.js thousands=500          -0.77 %            0.48345237

CI: https://ci.nodejs.org/job/node-test-pull-request/8004/

Well, for perfect robustness (and Web IDL compliance, which I know we don't particularly care for) we need to get the initial value of Function.prototype.apply and cache it – and do this for every single built-in prototype method we use. I'm good with this change, but it's important to recognize that these type of changes could become slippery slopes very quickly.

we need to get the initial value of Function.prototype.apply and cache it

I think we should leak C++ helpers for call and apply rather then cache the JS aliases. So we actually use "the [[Call]] internal method of a function object"

I think we should leak C++ helpers for call and apply rather then cache the JS aliases. So we actually use "the [[Call]] internal method of a function object"

They are equivalent, and the C++ helpers will be much slower than JS.

LGTM pending some benchmark / profiling output, which is the reason this code was this way.

Benchmark results are included above. They show no significant performance change. No changes have been applied to this PR since those benchmarks have been run.

They are equivalent, and the C++ helpers will be much slower than JS.

How about hiding the defaults behind symbols on Function during bootstrapping?

The benchmark results look ok to me, but I am also concerned about this being a slippery slope. Just how far down the rabbit hole of protecting against userland do we have to go?

Just how far down the rabbit hole of protecting against userland do we have to go?

I'd like us to go this far down the rabbit hole:

• Does the problematic behavior affect actual users or is it likely to affect actual users?
• Is there a fix that does not have significant performance impact or that is likely to introduce more bugs?

If the answer to both questions is "yes" (or at least "seems likely"), then I'm good with it.

On the "affect actual users", I should note that I may be making an incorrect assumption here. I assumed that because @daurnimator opened an issue about this behavior, it is something that affected them.

Well, for perfect robustness (and Web IDL compliance, which I know we don't particularly care for) we need to get the initial value of Function.prototype.apply and cache it

@TimothyGu I followed up on your suggestion #12981

Just noting... I've been stewing over an internal module that captures the original exports for key items like toString(), apply(), call() etc on startup before any user code is able to run, specifically to protect ourselves from cases like this.

I should note that I may be making an incorrect assumption here. I assumed that because @daurnimator opened an issue about this behavior, it is something that affected them.

Indeed. And this was just the first thing I ran into, as setTimeout was the first function I called to test out some code. => The second function I called was console.log (and hence util.inspect) when this didn't work!
I expect I'll run into case after case of similar things to this as I work on my current project. I will continue to report things as I run into them.

Landed in 98609fc.

is this applicable to lts v6.x?

is this applicable to lts v6.x?

@MylesBorins Yes.

Failures on v6.x

== release test-timers-user-call ===
Path: parallel/test-timers-user-call
/Users/mborins/code/node/v6.x/test/common/index.js:440
    return fn.apply(this, arguments);
              ^

TypeError: fn.apply is not a function
    at Timeout._onTimeout (/Users/mborins/code/node/v6.x/test/common/index.js:440:15)
    at ontimeout (timers.js:386:11)
    at tryOnTimeout (timers.js:250:5)
    at Timer.listOnTimeout (timers.js:214:5)
Command: out/Release/node /Users/mborins/code/node/v6.x/test/parallel/test-timers-user-call.js
[01:53|% 100|+ 1355|-   1]: Done
make: *** [test] Error 1

Can you please backport

@MylesBorins Needs #12027 to land first, then that error should go away.

(Removing backport-requested-v6.x because I don't think a backport is needed. But if I'm wrong, by all means, re-add it.)

@Trott I manually backported #12027 and this lands cleanly now!