Node: Breaking change in V8 for Error.prepareStackTrace

[schuay]

The proposed change (not yet landed): https://crrev.com/c/1113438
And the original related node issue: https://github.com/nodejs/node/issues/21270

This changes formatting behavior for stack traces to use the Error.prepareStackTrace function set in the original creation context of the error object, instead of the current context.

The following node tests start failing:

parallel/test-repl-pretty-custom-stack
parallel/test-repl-pretty-stack
parallel/test-repl-underscore

Failures look similar to:

    AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
    + expected - actual

    - '    at repl:1:18'
    + 'Error: foo'
        at Immediate.setImmediate (/b/swarming/w/ir/cache/builder/v8_node_linux64_rel/node.js/test/parallel/test-repl-underscore.js:200:16)
        at runCallback (timers.js:695:18)
        at tryOnImmediate (timers.js:666:5)
        at processImmediate (timers.js:648:5)
        at process.topLevelDomainCallback (domain.js:121:23)

Full test run logs: https://logs.chromium.org/v/?s=v8%2Fbuildbucket%2Fcr-buildbucket.appspot.com%2F8942571998063714288%2F%2B%2Fsteps%2Frun_tests%2F0%2Fstdout

Presumably this happens because custom stack formatting is set in one context but not another.

Can this reasonably be fixed in node?

[schuay]

cc @hashseed @psmarshall

[hashseed]

@addaleax

[Trott]

Does this change make V8 more aligned with a spec?

If so, then it's possible that the versions of the tests in https://github.com/nodejs/node-chakracore might already be modified to comport to the new behavior.

If not, then I imagine the tests all need to be changed to use RegExp matching instead of string comparisons so that they can be written in an engine-agnostic way.

/cc @nodejs/chakracore

[devsnek]

@Trott Error.prepareStackTrace is a special v8 thing that isn't in any spec.

[Trott]

@Trott Error.prepareStackTrace is a special v8 thing that isn't in any spec.

I guess this is an opportunity to see if a RegExp-matching version of each of the tests can be created that passes for current Node.js, Node.js with this upcoming V8 update, and node-chakracore.

If I get some time later, I might try to do that. If someone else beats me to it, even better. :-D

[Trott]

Does this behavior change look correct/acceptable?

Code:

'use strict';

const repl = require('repl');

const r = repl.start({
  prompt: '',
  terminal: false,
  useColors: false
});
r.write('throw new Error("whoops");\n');
r.close();

Output currently:

Error: whoops

Output after this V8 change lands:

Error: whoops
    at repl:1:7
    at Script.runInContext (vm.js:100:20)
    at REPLServer.defaultEval (repl.js:319:29)
    at bound (domain.js:396:14)
    at REPLServer.runBound [as eval] (domain.js:409:12)
    at REPLServer.onLine (repl.js:615:10)
    at REPLServer.emit (events.js:182:13)
    at REPLServer.EventEmitter.emit (domain.js:442:20)
    at REPLServer.Interface._onLine (readline.js:290:10)
    at REPLServer.Interface._normalWrite (readline.js:433:12)

If that looks correct, I'll modify the tests to accommodate both the current and changed behavior.

If the new output is deemed undesirable, then this will require some work on the REPL code rather than the tests.

@nodejs/repl

[addaleax]

It seems like this was done quite intentionally: https://github.com/nodejs/node/blob/master/lib/repl.js#L491-L513 (c5f54b1fad19a)

I haven’t tried it, but maybe instead of mutating Error.prepareStackTrace we want to mutate self.context.Error.prepareStackTrace now?

[Trott]

Not sure if it changes anything, but FWIW, as best as I can tell, these additional stack trace lines only appear when using the REPL programmatically. Interactive REPL behavior seems unchanged. (I could be wrong, but that's what I'm seeing.)

[Trott]

/ping @lance

[addaleax]

@trott That might be because for the buit-in REPL, we use Node’s main Context rather than creating a new one (i.e. repl.start({ useGlobal: true }))?

[Trott]

@lance Any chance you want to grab the V8 patch and see about repairing the stacktrace pruning feature in the REPL?

[Trott]

FWIW, this fixed the underscore test (and I think some of the test cases in the other two tests as well) for me but did not fix everything. Still didn't do the expected trimming for some of the stack traces.

diff --git a/lib/repl.js b/lib/repl.js
index 6b5f480387..0de6cfd0ff 100644
--- a/lib/repl.js
+++ b/lib/repl.js
@@ -399,11 +399,12 @@ function REPLServer(prompt,
   self._domain.on('error', function debugDomainError(e) {
     debug('domain error');
     const top = replMap.get(self);
-    const pstrace = Error.prepareStackTrace;
-    Error.prepareStackTrace = prepareStackTrace(pstrace);
+    const originalPrepareStackTrace = e.constructor.prepareStackTrace;
+    e.constructor.prepareStackTrace =
+      prepareStackTrace(Error.prepareStackTrace);
     if (typeof e === 'object')
       internalUtil.decorateErrorStack(e);
-    Error.prepareStackTrace = pstrace;
+    Error.prepareStackTrace = originalPrepareStackTrace;
     const isError = internalUtil.isError(e);
     if (!self.underscoreErrAssigned)
       self.lastError = e;

[schuay]

Is there any progress on this? I'd like to go ahead and land the V8 patch soonish.

[devsnek]

I'll take a look today after tc39 if no one else gets to it by then... should just be looking up Error from the repl's context instead of global

[schuay]

@devsnek any news?

[devsnek]

@schuay we're gonna need a magic way to get the context of an error's constructor for this to work in node. i think its worth pursuing https://crrev.com/c/1119768 as the solution for node.

the following would probably work but i would rather not do it

void RealmGlobalForObject(const FunctionCallbackInfo<Value>& args) {
  args.GetReturnValue().Set(args[0]->CreationContext()->Global());
}

realmGlobalForObject(error).Error.prepareStackTrace

[jasnell]

There's been no further activity on this in nearly 2 years. Does this need to remain open?

[devsnek]

This was handled a while back in https://github.com/nodejs/node/commit/b046bd193573faf89ac088076b223d5535fb4876