Implement custom error handling

[CapacitorSet]

As discussed in #87. Produces stack traces that look the same as "native ones", with vm2 frames stripped. Controlled by the prettyErrors option for VM, true by default.

With no pretty errors

/tmp/vm2/node_modules/vm2/lib/main.js:281
                        throw this._internal.Decontextify.value(e);
                        ^
TypeError: Cannot read property 'f' of null
    at f (vm.js:2:7)
    at vm.js:4:1
    at Script.runInContext (vm.js:135:20)
    at VM.run (/tmp/vm2/node_modules/vm2/lib/main.js:272:67)
    at f (/tmp/vm2/index.js:12:5)
    at Object.<anonymous> (/tmp/vm2/index.js:15:1)
    at Module._compile (internal/modules/cjs/loader.js:816:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:827:10)
    at Module.load (internal/modules/cjs/loader.js:685:32)
    at Function.Module._load (internal/modules/cjs/loader.js:620:12)

With pretty errors

vm.js:2
        null.f();
              ^

TypeError: Cannot read property 'f' of null
    at f (vm.js:2:7)
    at vm.js:4:1
    at f (/tmp/vm2/index.js:12:5)
    at Object.<anonymous> (/tmp/vm2/index.js:15:1)```

[CapacitorSet]

Example code:

const { VM } = require('vm2');

const sourceCode = `function f() {
        null.f();
}
f();`;

const vm = new VM({
        prettyErrors: true,
});
vm.run(sourceCode);

[patriksimek]

Just an idea... what if we implement stack prettifying on the proxy level? All errors that come from the sandbox are decontextified (via Proxy). That way we don't have to use uncaughtException handler and it also can fix stack traces for caught exceptions. As I said, it's an idea... not sure if that is possible, but I think it is worth investigating.

[patriksimek]

Also, using a Domain may be a better way to catch uncaught exceptions from the VM. The API is deprecated, but the replacement API was not introduced yet so it should be fine to use it.

[CapacitorSet]

I'm not sure about moving the code to the decontextifier, it would be a bit messy to pass the settings and script name without significant gain. It would be nice not to register a custom uncaughtException handler, though: how do you feel about moving this code to the catch block? Something like:

} catch (dirtyEx) {
    const e = this._internal.Decontextify.value(dirtyEx);
    if (!this.options.prettyErrors)
        throw e;
    let rewrittenEx = new Error();
    a.stack = ...;
    throw rewrittenEx;
}

[CapacitorSet]

ping

[patriksimek]

Sorry for the delay. The issue with your proposal is you'll lose all properties available on the original error object. The goal is to only rewrite the stack property. What about using a Proxy with a get handler just for the stack property?

[CapacitorSet]

I'm trying to do it in Decontextify, but I can't seem to modify the returned stack. Do you have any pointers in that regard? I'm clueless.

I removed my code from the catch block and added the following in Decontextify.instance.get:

if (key === 'stack' && target instanceof Error) {
   // throw 1;
   a = new Error();
   return a.stack;

If you uncomment the throw 1 you'll see that the proof-of-concept doesn't print any stack trace (I'm getting just the offending line in the proof-of-concept and "TypeError: ..."), so that if block is clearly being executed, but if I return a fictitious stack trace the proof-of-concept will print the original stack trace.

[patriksimek]

I was able to trigger the stack trap with this condition:

if (instance instanceof Error && key === 'stack') {
	return prettify(instance.stack);
}

Don't forget to put that in the try-catch block.

One more thing - you don't have to rewrite the file name, you can define the name of the script with this code:

console.log(vm.run(new VMScript(`new Error('asdf')`, 'myscript.js')).stack);

The stack trace is then generated with the correct file name.

[tbenst]

@CapacitorSet thanks for the work on this. Just got bit by a hard-to-debug error due to no stacktrace, and I'd love to see this feature added! Wanted to gently follow-up to see if anything is blocking this (other than lack of time)?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[tbenst]

Commenting to keep open

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[tbenst]

@CapacitorSet @patriksimek let me know if I can help?

[CapacitorSet]

For some reason I couldn't apply the fix suggested by Patrik, and eventually abandoned the issue. @tbenst could you look into that?

[brainysmurf]

Commenting here. I think it would be great if this could feature could be implemented.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.