🐛 Handle non-Error debugger throw values#4740
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Bundles Sizes Evolution
|
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03b6d4c0c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
watson
force-pushed
the
watson/support-non-error-thrown
branch
from
03b6d4c to
9c9b8ac
Compare
JavaScript allows throwing arbitrary values, but the debugger hook typed thrown values as Error and read message/stack unconditionally. Type thrown values as unknown, safely format non-Error throwables, and keep an empty stacktrace for the current Live Debugger UI contract. Reuse the shared browser-core isError helper for cross-realm Error detection and make it non-throwing for hostile toStringTag getters. Add unit tests covering thrown strings, cross-realm Errors, and values that cannot be coerced to strings.
watson
force-pushed
the
watson/support-non-error-thrown
branch
from
9c9b8ac to
f32c7a4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f32c7a4f83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { | ||
| return String(error) | ||
| } catch { | ||
| return jsonStringify(sanitize(error)) ?? '<error: unable to stringify thrown value>' |
There was a problem hiding this comment.
issue: For a non-Error thrown object whose primitive coercion fails and whose enumerable getter also throws (for example { toString() { throw ... }, get x() { throw ... } }), this fallback still lets sanitize(error) throw while $dd_throw is running inside the instrumented catch before the original throw e pattern (test/apps/instrumentation-overhead/src/instrumented.ts). The fresh evidence after the prior review is this new sanitizer fallback path: it catches String(error) but not failures from sanitizing the same thrown value, so the SDK can still replace the application's original exception instead of only recording a snapshot.
Useful? React with 👍 / 👎.
1 participant
Motivation
JavaScript can throw any value, not just
Errorinstances. The debuggeronThrowhook treated caught values asError, which meant a thrown string produced an undefined throwable message and assumed a stacktrace was available.Changes
onThrownow accepts the thrown value asunknown, stores it as such in the active entry, and normalizes the throwable payload at snapshot creation time.Errorvalues keep their message and parsed stacktrace. Non-Errorvalues are formatted safely and send an empty stacktrace to preserve the current Live Debugger UI contract.Debugger throwable formatting now uses the shared
browser-coreisError()helper so cross-realm Errors, such as iframe-created Errors, keep their throwable details. The shared helper is also guarded so hostileSymbol.toStringTaggetters cannot throw from error detection.Unit tests cover thrown strings, cross-realm Errors, and values that cannot be coerced to strings.
Test instructions
Run
yarn test:unit --spec packages/browser-debugger/src/domain/api.spec.ts.Run
yarn test:unit --spec packages/browser-core/src/domain/error/error.spec.ts.Checklist