rewrite bind function in prototype with console inside causes Maximum call stack size exceeded

[lessfish]

Version: v12.4
Platform:Darwin Kernel Version 18.6.0

my issue code is quite simple:

Function.prototype.bind = function () {
  console.log(1)
}

const fn = function () {}
fn.bind()

it works well in chrome, I don't know if it'a a feature or bug in nodejs

thanks

[BridgeAR]

We use bind in Node.js itself. Due to replacing that function Node.js fails, since the original functionality does not exist anymore. Please do not manipulate the prototype of built-in functions. That might have lots of side effects.

[lessfish]

@BridgeAR console.log uses bind inside, and if we replace that function, console.log will use my function, and results in the Infinite loop, can I understand like this?

[targos]

This is worse now, because it triggers an assertion:

./node[2254009]: ../../src/node_url.cc:2219:void node::url::{anonymous}::Parse(const v8::FunctionCallbackInfo<v8::Value>&): Assertion `args[4]->IsFunction()' failed.
 1: 0xa941a0 node::Abort() [./node]
 2: 0xa9421e  [./node]
 3: 0xb3d11a  [./node]
 4: 0xce407b  [./node]
 5: 0xce5626  [./node]
 6: 0xce5ca6 v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [./node]
 7: 0x14e3bd9  [./node]
[1]    2254009 abort (core dumped)  ./node

Candidate for primordials migration.

/cc @aduh95

[aduh95]

There are two uses of Function.prototype.bind in core that are executed by console.log calls, unfortunately both of them are causing significant perf regressions and I had to revert them in the PRs I've opened (8d5b676#diff-330bdd22a188135540523f69deb4f9563e03b00a913cd369e1ee84d899f178a9R497 and 8dddb89#diff-28ea8bff0d427c8a06dab3edfba075da70ed458a5a23d250e500a88e92a468f8R150). We can try to add them again after the next V8 version has landed in case it comes with improved performance for primordials 🤞

[targos]

If we cannot change those to use primordials, we need to do something else. It's not OK for the process to crash in this case.

[benjamingr]

I wonder what the perf regression is of "cheating with"*:

• Watch function.prototype.bind (with a setter)
• Until set, use the "regular" Function.prototype.bind
• if the setter is called we "fallback" to a much slower mode where our setter executes the original .bind in core bode but the overridden one if set with AsyncResource to know we're "in core".

That would mean (hopefully) no perf regressions for 99.9% of users while still being resilient (but slower) for people who want to override .bind

• this is a common thing V8 does when optimizing

[dario-piotrowicz]

Given @BridgeAR comment above maybe Node.js should simply not allow users to override .bind? that should be easy enough to do, would that be too restrictive? is overriding .bind really that common/useful? 🤔 🤐

[ljharb]

@dario-piotrowicz that's kind of irrelevant, since the JS spec requires that it be overridable - if node restricted that, then node wouldn't be a compliant JS engine.

[dario-piotrowicz]

@ljharb I see 😓 that makes sense thanks 👍