Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

console.log should *not* be a constructor #25987

Closed
lachrist opened this issue Feb 7, 2019 · 13 comments
Closed

console.log should *not* be a constructor #25987

lachrist opened this issue Feb 7, 2019 · 13 comments
Labels
console Issues and PRs related to the console subsystem.

Comments

@lachrist
Copy link

lachrist commented Feb 7, 2019

  • 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

@Fishrock123 Fishrock123 added the console Issues and PRs related to the console subsystem. label Feb 7, 2019
@TimothyGu
Copy link
Member

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

@TimothyGu TimothyGu reopened this Feb 7, 2019
@Hakerh400
Copy link
Contributor

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
Copy link
Member

addaleax commented Feb 9, 2019

@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
Copy link
Author

lachrist commented Feb 9, 2019

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
Copy link
Member

jdalton commented Feb 9, 2019

Is this covered by the console spec?

@lachrist
Copy link
Author

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
Copy link
Member

jdalton commented Feb 10, 2019

@lachrist

Maybe not. But,

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

@TimothyGu
Copy link
Member

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
Copy link
Contributor

@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
Copy link
Member

@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.

addaleax pushed a commit that referenced this issue Mar 1, 2019
Ref: #25987

PR-URL: #26096
Refs: #25987
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
BridgeAR pushed a commit that referenced this issue Mar 4, 2019
Ref: #25987

PR-URL: #26096
Refs: #25987
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
@Hakerh400
Copy link
Contributor

This can probably be closed as fixed.

@addaleax
Copy link
Member

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

@TimothyGu
Copy link
Member

Fixed in #26700.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
console Issues and PRs related to the console subsystem.
Projects
None yet
Development

No branches or pull requests

6 participants