console.log should *not* be a constructor

[lachrist]

• Version: v11.9.0
• Platform: Darwin Laurents-MacBook-Pro-2.local 17.7.0 Darwin Kernel Version 17.7.0: Fri Nov 2 20:43:16 PDT 2018; root:xnu-4570.71.17~1/RELEASE_X86_64 x86_64
• Subsystem:

In node, console.log behaves like a constructor without a prototype field. Other runtimes throw a proper type error in both lines below:

$ node
> new console.log();

consoleCall {}
> Reflect.construct(Boolean, [], console.log);
[Boolean: false]

Much love,
Laurent

[TimothyGu]

Essentially we should create the log function as a method (({ log(...args) { … } })) or as an arrow function.

[Hakerh400]

Console methods can't be arrow functions, since they are bound to a Console instance. Defining them as methods should work. Also, if inspector is attached, all methods are wrapped, thus something like this is also needed:

diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc
index 48ebd73817..a7d72bd327 100644
--- a/src/inspector_js_api.cc
+++ b/src/inspector_js_api.cc
@@ -146,6 +146,12 @@ void CallAndPauseOnStart(const FunctionCallbackInfo<v8::Value>& args) {
 }

 void InspectorConsoleCall(const FunctionCallbackInfo<Value>& info) {
+  Isolate* iso = Isolate::GetCurrent();
+  if (info.IsConstructCall()) {
+    iso->ThrowException(v8::Exception::TypeError(String::NewFromUtf8(
+        iso, "Console methods are not constructors")));
+    return;
+  }
   Environment* env = Environment::GetCurrent(info);
   Isolate* isolate = env->isolate();
   Local<Context> context = isolate->GetCurrentContext();

I would like to open a PR if this looks good.

[addaleax]

@Hakerh400 I think rather than us throwing an exception, we could use V8’s ability to prevent constructor behaviour altogether – you can search around for ConstructorBehavior::kThrow in our source code to see how that’s done. (That also has nice properties like not creating a .prototype property on the function.)

More generally, there is a TODO comment that might address this in a blanket fashion for all C++-backed JS methods exposed by Node:

node/src/env-inl.h

Lines 750 to 756 in c2d374f

	v8::Local<v8::Function> function =
	NewFunctionTemplate(callback, v8::Local<v8::Signature>(),
	// TODO(TimothyGu): Investigate if SetMethod is ever
	// used for constructors.
	v8::ConstructorBehavior::kAllow,
	v8::SideEffectType::kHasSideEffect)
	->GetFunction(context)

Changing kAllow to kThrow would do the trick here. The TODO comment expresses some doubt about this always being correct, but from a quick look through the usage of SetMethod() and SetMethodNoSideEffect(), I think it is okay, because we never use these helpers to set up constructors.

[lachrist]

Yeah, throwing an error if console.log is called as a constructor is a partial solution. It will not throw if used as a new.target e.g.: Reflect.construct(Boolean, [], console.log). Plus, it will allow the construct proxy handler to be called:

var p = new Proxy(console.log, {
  construct: () => {
    throw new Error("This should never be called");
  }
});
new p();

[jdalton]

Is this covered by the console spec?

[lachrist]

Maybe not. But, in the ECMAScript standard, primordials have a consistent constructor / non-constructor behaviour. I've made a script that reveals all the primordials that do not have a consistent constructor behavior. It only print the functions of the console API. The only other exception is Proxy which makes sense as proxies do not have an internal [[Prototype]] field. Why not normalise the console API as the other runtimes do?

global.console._stdout._writableState.onwrite
global.console._stdout._writableState.corkedRequestsFree.finish
global.console._stderr._writableState.onwrite
global.console._stderr._writableState.corkedRequestsFree.finish
global.console.log
global.console.debug
global.console.info
global.console.dirxml
global.console.warn
global.console.error
global.console.dir
global.console.time
global.console.timeEnd
global.console.timeLog
global.console.trace
global.console.assert
global.console.clear
global.console.count
global.console.countReset
global.console.group
global.console.groupCollapsed
global.console.groupEnd
global.console.table
global.Proxy

const done = new Set();
const isconstructor = (value) => {
  try {
    Reflect.construct(Boolean, [], value);
    return true;
  } catch (error) {
    return false;
  }
};
const loop = (value, path) => {
  if (value !== null && (typeof value === "object" || typeof value === "function")) {
    if (!done.has(value)) {
      done.add(value);
      if (isconstructor(value) && !Reflect.getOwnPropertyDescriptor(value, "prototype"))
        console.log(path);
      Reflect.ownKeys(value).forEach((key) => {
        const descriptor = Reflect.getOwnPropertyDescriptor(value, key);
        if ("value" in descriptor) {
          loop(descriptor.value, path+"."+String(key));
        } else {
          loop(descriptor.get, path+"."+String(key)+"[get]");
          loop(descriptor.set, path+"."+String(key)+"[set]");
        }
      });
      loop(Reflect.getPrototypeOf(value, path+".[__proto__]"));
    }
  }
};
loop(global, "global");

[jdalton]

@lachrist

Maybe not. But,

Ah okay. We should ping folks involved in that effort to see about covering any gap:
\cc @domfarolino

[TimothyGu]

The Console Standard does not cover it directly, but by virtue of the method being defined through Web IDL, Web IDL rules apply – and they are that the method should not be a constructor.

[Hakerh400]

@addaleax Thanks for the suggestion. With kThrow it seems to work. The patch is made in f084076. If it looks ok, I think it's ready for a PR.

[addaleax]

@Hakerh400 The changes look okay, but I would suggest making separate PRs for the changes to the console source code and to env-inl.h, respectively; even if they have the same goal, they have pretty different characteristics in terms of what and how they are changing things.

[Hakerh400]

This can probably be closed as fixed.

[addaleax]

@Hakerh400 Changing ConstructorBehavior::kAllow to ConstructorBehavior::kThrow in general is still outstanding here. I’ve opened #26700 to address that.

[TimothyGu]

Fixed in #26700.