vm: Error.prepareStackTrace behaves inconsistently across versions

[bengl]

• Version: 8.11.2
• Platform: Linux 4.16.4-1-ARCH
• Subsystem: vm

In Node 6.x (tested on v6.14.2), the following results in the prepareStackTrace defined inside the context being used:

const { runInNewContext } = require('vm');
console.log(runInNewContext(`() => {
  Error.prepareStackTrace = () => 'hello';
  return new Error();
}`)().stack);

The stack is logged as hello.

In Node 8.x (tested with 8.11.2), it will just output a normal stack, ignoring the prepareStackTrace defined in the context.

It seems that in Node 8, the prepareStackTrace from the currently running context (i.e. the one where .stack is called) is used, whereas in Node 6, the one where the Error was instantiated is used.

Note that this only happens when the Error is passed along, and not when it's a thrown exception. I suspect that's because when it's thrown, DecorateErrorStack is run, which gets the stack before it's passed to the main context:

node/src/node_contextify.cc

Lines 735 to 747 in faf417b

	static void DecorateErrorStack(Environment* env, const TryCatch& try_catch) {
	Local<Value> exception = try_catch.Exception();
	if (!exception->IsObject())
	return;
	Local<Object> err_obj = exception.As<Object>();
	if (IsExceptionDecorated(env, err_obj))
	return;
	AppendExceptionLine(env, exception, try_catch.Message(), CONTEXTIFY_ERROR);
	Local<Value> stack = err_obj->Get(env->stack_string());

Note also that when the stack is retrieved inside the new context, the in-context prepareStackTrace is always used, regardless of version.

[targos]

What about v10?

[devsnek]

this feature isn't actually provided by node, it's a debug feature from v8. iirc it's still documented but we should probably remove that.

/cc @nodejs/v8

edit: we haven't even documented it.

[bengl]

@targos Node 10 (tested with 10.4.0) produces the same result as Node 8.

[hashseed]

@schuay fwiw I think the old behavior of using prepareStackTrace from the origin context makes more sense. Wdyt?

[devsnek]

@hashseed i think people would draw a line sort of like "myErrorInstance" => "myErrorInstance's Constructor" => "That constructor .prepareStackTrace", so using the origin context would make sense.

personally though, i would argue that prepareStackTrace should be deprecated and removed

[jdalton]

personally though, i would argue that prepareStackTrace should be deprecated and removed

I'm not a fan of removing useful APIs without alternatives.

[devsnek]

@jdalton tc39/proposal-error-stacks#20 (comment)

the api might useful if it existed anywhere else besides v8 but it doesn't...

[hashseed]

I don't think removing prepareStackTrace at this point is a feasible option, even though it's not part of the standard. Same applies to Error.stack in the first place. There are some effort though to standardize it, and I'm hoping something similar to prepareStackTrace will be part of it too.

[schuay]

A d8 repro:

const that_realm = Realm.create();
Realm.eval(that_realm, '() => { Error.prepareStackTrace = () => 42; return new Error(); }')().stack;

In the example above, the stack property is formatted lazily in the current realm. There, we load prepareStackTrace from the global error object (code).

I agree the old behavior makes more sense, especially since we currently use a different prepareStackTrace depending on when exactly stack is first accessed. Opened https://crbug.com/v8/7848.

[Trott]

Thoughts on closing this? I'm inclined to close this on the grounds that it's a V8 issue and a bug should be opened in the V8 issue tracker. This is an undocumented API as far as Node.js goes, and the "new" behavior here is now on all current release lines except Node.js 6.x which is going EOL in 6 months.

[devsnek]

its already got a bug and fix in v8. it's just waiting on my rewrite of stack decoration (i'm about 50% done). i think this is safe to close.

[Trott]

its already got a bug and fix in v8. it's just waiting on my rewrite of stack decoration (i'm about 50% done). i think this is safe to close.

If it's awaiting some code change in Node.js, then I think we can leave it open. I thought it was entirely in V8-land.