Don't use userobject.call/userobject.apply

[daurnimator]

• Version: v7.9.0

Internally node shouldn't rely on user provided functions/objects having 'normal' values.

e.g.

a = function() { console.log(1,2,3)};
a.call = "some value"
setTimeout(a, 100); true

Output:

TypeError: callback.call is not a function
    at ontimeout (timers.js:386:14)
    at tryOnTimeout (timers.js:250:5)
    at Timer.listOnTimeout (timers.js:214:5)

For this example, node's timers.js should be using Function.prototype.call or Reflect.apply

[daurnimator]

@mscdex this is not timers specific: this applies across many of node's libs (timers included).

another example (from util.js):

util = require("util");
a = function() {};
b = function() { return "b" }
a[util.inspect.custom] = b
b.call = "some value"
a

Output:

TypeError: maybeCustomInspect.call is not a function
    at formatValue (util.js:369:36)
    at Object.inspect (util.js:203:10)
    at REPLServer.self.writer (repl.js:459:19)
    at finish (repl.js:584:38)
    at REPLServer.defaultEval (repl.js:377:5)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.onLine (repl.js:536:10)
    at emitOne (events.js:101:20)
    at REPLServer.emit (events.js:191:7)

and also in sameish area:

util = require("util");
a = function() {};
a[util.inspect.custom] = function() { return "a" }; 
Object.defineProperty(a, "constructor", {get: function() {throw Error()}})

Output:

Error
    at Function.get (repl:1:65)
    at formatValue (util.js:368:16)
    at Object.inspect (util.js:203:10)
    at REPLServer.self.writer (repl.js:459:19)
    at finish (repl.js:584:38)
    at REPLServer.defaultEval (repl.js:377:5)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.onLine (repl.js:536:10)
    at emitOne (events.js:101:20)

[refack]

@daurnimator I believe that in both cases node adheres to the standards

1. setTimeout - https://html.spec.whatwg.org/multipage/webappapis.html#timer-initialisation-steps
2. Object.defineProperty - http://www.ecma-international.org/ecma-262/5.1/#sec-15.2.3.6
   Admittedly in the second case it throws because node called util.inspect.custom but that's only in the REPL. If you run it as a script, nothing will happen.
   node -p "var a = function() {}; a[util.inspect.custom] = function() { return 'a' }; Object.defineProperty(a, 'constructor', {get: function() {throw Error()}}); 5"

Node implements JavaScript (more precisely ECMAscript), and like many other programing languages, it allows you to make mistakes, from never ending loops, to division by 0, and yes, even manipulating the internal mechanics to throw errors.

I'm going to close this issue, since I didn't find a deviation from the standards. If you find such a deviation, feel free to reopen, and give us an example so we can do better...

[daurnimator]

setTimeout - https://html.spec.whatwg.org/multipage/webappapis.html#timer-initialisation-steps

When the standard says "Invoke the Function" it doesn't mean to use .call, it means to use the internal mechanism for calling (which is exposed by Reflect.apply).
See https://heycam.github.io/webidl/#invoke-a-callback-function which leads to https://tc39.github.io/ecma262/#sec-call

Object.defineProperty - http://www.ecma-international.org/ecma-262/5.1/#sec-15.2.3.6
Admittedly in the second case it throws because node called util.inspect.custom but that's only in the REPL. If you run it as a script, nothing will happen.
node -p "var a = function() {}; a[util.inspect.custom] = function() { return 'a' }; Object.defineProperty(a, 'constructor', {get: function() {throw Error()}}); 5"

Try printing/inspecting a:

$ node -p "var a = function() {}; a[util.inspect.custom] = function() { return 'a' }; Object.defineProperty(a, 'constructor', {get: function() {throw Error()}}); console.log(a)"
[eval]:1
var a = function() {}; a[util.inspect.custom] = function() { return 'a' }; Object.defineProperty(a, 'constructor', {get: function() {throw Error()}}); console.log(a)
                                                                                                                                     ^

Error
    at Function.get ([eval]:1:140)
    at formatValue (util.js:368:16)
    at inspect (util.js:203:10)
    at exports.format (util.js:69:24)
    at Console.log (console.js:43:37)
    at [eval]:1:160
    at ContextifyScript.Script.runInThisContext (vm.js:23:33)
    at Object.runInThisContext (vm.js:95:38)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (module.js:571:32)

Node implements JavaScript (more precisely ECMAscript), and like many other programing languages, it allows you to make mistakes, from never ending loops, to division by 0, and yes, even manipulating the internal mechanics to throw errors.

IMO internal mechanics throwing errors is an entirely different category to infinite loops and divide by zero.
I'll also note that this is an issue that popped up for me when trying to run+debug code under node.js; which ended up just making things more confusing.

I'm going to close this issue, since I didn't find a deviation from the standards. If you find such a deviation, feel free to reopen, and give us an example so we can do better...

Please see above: .call is not a standards compliant way to "invoke a function"

[refack]

@daurnimator note that in all cases IMHO node did what you asked it to do, and most importantly did not crash. It provided you with a proper Error with a stack trace.

We can argue the trade-offs in the implementation of the 'util' module (i.e. printing/inspecting) it is a node module that is not part of the standards, and indeed has it's limitations. For example Reflect.apply is a new language construct, and is not yet optimized for performance, so we did not refactor our code to use it (where appropriate).
We also try not to "break" the status-quo unless there is a very compelling reason, since what you consider a bug, other have come to rely upon as expected behaviour, and us changing that behaviour might bring more harm than good in the wider scope of the entire ecosystem.

[Trott]

I'm going to re-open this. I think @daurnimator has a point. In particular, the first sample code provided runs as expected in the browser (tested on Chrome) and I would think we'd want to emulate that robustness.

[daurnimator]

We can argue the trade-offs in the implementation of the 'util' module (i.e. printing/inspecting) it is a node module that is not part of the standards, and indeed has it's limitations.

Please note that this issue can be found all across the node code-base. I've mentioned setTimeout as the behaviour can be easily compared to browsers (and is standardised), and util as I haven't figured out a workaround when debugging.

Reflect.apply is a new language construct, and is not yet optimized for performance, so we did not refactor our code to use it (where appropriate).

Function.prototype.call would have been the correct choice beforeReflect was introduced.

[refack]

Also as you might know node welcomes all contributions. If you feel you can help us improve the util module or other in general, I invite to you open a PR.
I think a good start would be to add your examples as tests to our test suite (/test/parallel/) you can see some of the files in https://github.com/nodejs/node/search?utf8=%E2%9C%93&q=util+extension%3Ajs+path%3A%2Ftest%2Fparallel%2F&type=

[refack]

I'm going to re-open this. I think @daurnimator has a point. In particular, the first sample code provided runs as expected in the browser (tested on Chrome) and I would think we'd want to emulate that robustness.

@Trott IMHO it's not a matter of robustness, since there was not crash, nor any non-deterministic behaviour. I obviously welcome the re-open If you feel we're deviating from the standard.

[refack]

Just tried this:

b = Object.create(Function)
setTimeout(b, 0)

Chrome:

Uncaught TypeError: Function.prototype.toString is not generic
    at Function.toString (<anonymous>)
    at <anonymous>:1:1
(anonymous) @ VM271:1

node

TypeError: "callback" argument must be a function
    at exports.setTimeout (timers.js:348:11)
    at repl:1:1
    at REPLServer.defaultEval (repl.js:339:29)
    at bound (domain.js:280:14)
    at REPLServer.onLine (repl.js:536:10)
    at emitOne (events.js:101:20)
    at REPLServer.emit (events.js:191:7)
    at REPLServer.Interface._onLine (readline.js:241:10)
    at REPLServer.Interface._line (readline.js:590:8)
    at REPLServer.Interface._ttyWrite (readline.js:869:14)

IMHO our Error is better. You win some, you lose some...