fix(ses): fix #2951 stronger sniffing for v8

[erights]

Closes: #2951 Refs: #1798 , https://github.com/tc39/proposal-error-capturestacktrace , https://github.com/tc39/proposal-error-stacks , https://github.com/tc39/proposal-error-stack-accessor

Description

(Description mostly from Copilot's overview below, which is quite good!)

This PR's error taming implements stronger platform detection for v8, addressing issue #2951. The change improves the reliability of v8 detection by testing the prepareStackTrace mechanism, in addition to checking for the presence of captureStackTrace.

This is needed because some non-v8 platforms have implemented captureStackTrace without prepareStackTrace. The https://github.com/tc39/proposal-error-capturestacktrace proposal would make it standard, eventually causing all conformant JS engines to implement it.

A verbally reported bug motivated this, even though that bug itself may be a false alarm. The report raised the possibility that our error-taming of treating the Hermes platform as if it was v8, and then misbehaving because it does not act like v8. Whether or not this actually happens on Hermes, it is plausible and would happen elsewhere with ses prior to this PR.

Security Considerations

If non-v8 engines start implementing the prepareStackTrace mechanism, we may need to tighten the sniff test once again. Such is the nature of sniff tests. Since we do not yet know the specifics of such possible additions, it does not yet make sense to tighten the sniff test, unless I'm missing some we should sniff instead.

During such a transition period, it is possible that the falsly-triggered v8 error taming could misbehave, including in ways that may be exploitable.

Scaling Considerations

none

Documentation Considerations

This is a bug fix and does not need documentation beyond the issue, this PR, and the inline comments added by this PR.

Testing Considerations

• [ ] I did not add automated tests, because I am unclear on how automated tests would test this. Reviewers: suggestions welcome.

• [ ] What I did do is manually step through the logic in the vscode debugger while running on Node. The sniff test code worked as expected there. If we do rely on such manual testing, I should at least test on some non-v8 platform that does implement captureStackTrace.

Compatibility Considerations

See "Security Considerations" above.

Upgrade Considerations

none

[naugtur]

Get ready, this will hurt.

Hermes does implement Error.prepareStackTrace and calls it with error and CallSite array

CallSite implementation is incomplete, mainly because Hermes doesn't have a concept of source files.

(...) have been added, full paths are present generally

Result of calling all V8 CallSite methods:

method	return type	return value
getColumnNumber	number	3
getEnclosingColumnNumber	number	1
getEnclosingLineNumber	number	41
getEvalOrigin	undefined
getFileName	string	file:///(...)endo/packages/ses/test/hermes/prepare-stack.js
getFunction	undefined
getFunctionName	string	myfunction
getLineNumber	number	42
getMethodName	object
getPosition	number	965
getPromiseIndex	object
getScriptNameOrSourceURL	string	file:///(...)endo/packages/ses/test/hermes/prepare-stack.js
getScriptHash	string	8ce5e9bc697fd7bb9acae06a235f27efab97fdb82378433b06a63ffe38c8303b
getThis	undefined
getTypeName	object
isAsync	boolean	false
isConstructor	boolean	false
isEval	boolean	false
isNative	boolean	false
isPromiseAll	boolean	false
isToplevel	boolean	true
toString	string	myfunction (file:///(...)endo/packages/ses/test/hermes/prepare-stack.js:42:3)

Result of calling all the same methods in Hermes

method	return type	return value
getColumnNumber	number	8
getEnclosingColumnNumber	Error	undefined is not a function
getEnclosingLineNumber	Error	undefined is not a function
getEvalOrigin	object
getFileName	string	/(...)endo/packages/ses/test/hermes/prepare-stack.js
getFunction	undefined
getFunctionName	string	myfunction
getLineNumber	number	42
getMethodName	object
getPosition	Error	undefined is not a function
getPromiseIndex	object
getScriptNameOrSourceURL	Error	undefined is not a function
getScriptHash	Error	undefined is not a function
getThis	undefined
getTypeName	object
isAsync	boolean	false
isConstructor	object
isEval	object
isNative	boolean	false
isPromiseAll	boolean	false
isToplevel	object
toString	string	[object CallSite]

---

We could either make sniffing even stronger by testing for usefulness of toString or we could keep the V8 detection in a state that also covers hermes (might need to rename some things?) and add a fallback for broken toString

[naugtur]

I've pushed two commits:

1. test runner for hermes with some tests covering the smoke test and known issues
2. a potential fallback for repareStackTrace CallSite toString method being broken

[erights]

Get ready, this will hurt.

Thanks for the warning. It did indeed hurt :/

[erights]

potential fallback for repareStackTrace CallSite toString method being broken

Btw, given the topic, I first read this as "repairStackTrace" and became unreasonably hopeful for a moment ;) .

[erights]

@naugtur , since the problematic case is Hermes, and since you've contributed so much of this PR anyway, should you take it from here?

Github will allow me to assign you. But due to the fact that I started the PR, Github won't let me review. So if you do take it from here, just informally consider me a reviewer. Thanks!

[naugtur]

Happy to take over, but may need some help with specifying the desired scope / what's left to do.

@mhofman, care to elaborate on the comment about who calls prepareStackTrace? How does it affect what we should do here?

If Node is doing things, should I expect the behavior to differ between Node and Chrome?

[mhofman]

@mhofman, care to elaborate on the comment about who calls prepareStackTrace? How does it affect what we should do here?

If Node is doing things, should I expect the behavior to differ between Node and Chrome?

My understanding is that v8 itself only implements a mechanism so that the host can specify the callback to use to prepare stack traces, and Chrome and v8 both register and internal callback (setPrepareStackTraceCallback(prepareStackTraceCallback) in the case of Node.js) that ends up calling whatever function is at Error.prepareStackTrace if it exists.

Node.js further has an internal default behavior for prepareStackTrace that depends on whether source maps are enabled or not. That internal behavior is now exposed as a "proxy" function set as the initial value for Error.prepareStackTrace. That means that the comment currently saying "This case should not occur on v8 or any other platform" is in fact inaccurate.

Instead of bailing out and assume v8, we may want to still test behavior when Error.prepareStackTrace is called to check whether it's compliant, even if we find an existing Error.prepareStackTrace. That means we need to restore whatever we found there in the first place after the test if we don't end up applying our own (e.g. unsafe errors).

Furthermore I recommend that we go the extra mile, save the original Error.prepareStackTrace if it exists, and rely on it to generate the stack string (from the filtered stack) in our replacement, so that any source mapping that Node.js does is still applied.