Add JS callstack to console.error callback#166
Open
bkaradzic-microsoft wants to merge 6 commits into
Open
Conversation
Add a third `const char* jsStack` parameter to the `Babylon::Polyfills::Console::CallbackT` signature. When `console.error` is invoked, the polyfill now captures the JavaScript call stack at the call site (via N-API `new Error()` -> `stack`) and passes it to the host callback. The stack is empty for `log` / `warn` to avoid the per-call cost on hot paths. This lets host apps (e.g. BabylonNative's Playground) surface a JS callstack alongside the native callstack in their diagnostic banners without monkey-patching `console.error` from JavaScript -- which is the only way to do this today and which masks the real call site under one or two synthetic frames. This is a breaking change to the public `Console` API. Existing host code must change its 2-arg lambda to a 3-arg lambda; the new parameter can be safely ignored to preserve existing behavior. Add a `Console.ErrorProvidesJsStack` gtest exercising both the log (must be empty) and error (must be non-empty) paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the Babylon::Polyfills::Console polyfill so host applications can associate console.error(...) output with the originating JavaScript call site by providing a JS stack string to the host callback. This improves diagnostics without requiring JS-side monkey-patching of console.
Changes:
- Updated
Babylon::Polyfills::Console::CallbackTto include a thirdconst char* jsStackparameter. - Implemented best-effort JS stack capture for
console.errorvianew Error().stackand passed it to the callback (empty string forlog/warn). - Updated unit tests and documentation to reflect the new callback signature and behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| Tests/UnitTests/Shared/Shared.cpp | Updates Console::Initialize callbacks to the new 3-arg signature and adds a regression test for console.error stack delivery. |
| Polyfills/Console/Source/Console.cpp | Captures JS stack for LogLevel::Error and passes it into the console callback. |
| Polyfills/Console/Readme.md | Updates documentation and sample initialization code for the new jsStack callback parameter. |
| Polyfills/Console/Include/Babylon/Polyfills/Console.h | Updates the public callback type and documents the new jsStack parameter. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bghgary
requested changes
May 13, 2026
Add an additive `Babylon::Polyfills::Console::CaptureCurrentJsStack(env)` helper that captures the JavaScript callstack at the current JS execution point (the polyfill's own shim frames are skipped, so the top frame is the user's call site). Hosts opt in by calling the helper inside their existing `CallbackT`. Currently implemented via N-API `new Error().stack` under the hood; when a cheap N-API stack-capture primitive lands later, the helper's body can be swapped without touching the public API. Rationale -- compared to making `CallbackT` itself carry the stack: - **Non-breaking.** Every existing 2-arg `CallbackT` continues to compile and behave identically. Hosts that don't care about stacks need not change a line. - **No always-on cost.** `new Error().stack` runs only when the host invokes the helper -- per-call, per-level, per-call-site, fully host-controlled. Hosts can capture on log/warn/error or any subset; hosts that ignore the feature pay nothing. - **Frame-skip count is an implementation detail** of how the polyfill registers `error` / `warn` / `log` with the engine. Hosts don't need to know it. Replace the `Console.ErrorProvidesJsStack` test with `Console.CaptureCurrentJsStack`: register a normal 2-arg callback that calls the helper in its body, and assert that both error and log paths yield a non-empty stack. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Previously the test blocked on `errorStackPromise.get_future().get()` / `logStackPromise.get_future().get()` with no timeout. If either callback never fired (script eval throws, polyfill regresses, etc.) the test would hang the whole gtest suite indefinitely. Use `future::wait_for(30s)` and fail with an informative gtest message if the callback hasn't fired by then -- the same pattern used by `AppRuntime.DestroyDoesNotDeadlock` below it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- `Console.cpp`: clear any pending N-API exception after the best-effort stack capture. N-API operations like `Object::Get` can leave a pending JS exception on `env` independently of throwing a C++ exception (e.g., a property accessor that throws). Without clearing, returning to the polyfill's wrapper function would surface the pending exception and `console.*` would itself throw on the JS side -- defeating the "side-effect free" contract. - `Console.h`: document the lifetime of `CallbackT`'s `const char*` message (valid only for the duration of the callback; copy to retain). Strengthen the `CaptureCurrentJsStack` doc to explicitly state capture is best-effort, may return empty even from a valid JS context, and hosts must check before using. - `Readme.md`: note that the helper is best-effort and may return empty. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
`Napi::Function::New(...)` already returns `Napi::Object` (see `napi-inl.h:1447`), so the explicit cast was a no-op. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Host applications consuming JsRuntimeHost have no way to associate a
console.error(...)(or any otherconsole.*) call with the JavaScript call site that produced it. TheConsole::CallbackTsignature only delivers the formatted message and the log level. To get a JS stack today, hosts must monkey-patchconsole.errorfrom JavaScript (new Error().stack, push as extra arg, call original) -- which both pollutes the globalconsoleobject and masks the real call site under one or two synthetic frames.Fix
Add an additive helper:
Hosts opt in by calling
CaptureCurrentJsStack(env)inside their existing 2-argCallbackT. The polyfill's own shim frames are skipped, so the top frame is the user's call site.The format of the returned string is engine-defined opaque text -- consumers should treat it as diagnostic-only and not parse.
Why a helper instead of a third
CallbackTparameterThe first iteration of this PR added a 3rd
const char* jsStackparameter toCallbackT. Reviewer feedback (#166 review thread) rightly pointed out three problems with that approach:Initialize(env, [](msg, lvl){ ... })lambda would have to update.new Error()+ stack format + UTF-8 conversion would run on everyconsole.erroreven when the host doesn't use the parameter.CallbackTfires, the JS frames that produced theconsole.*call are still alive on the JS stack -- the engine is paused mid-call, one or two C++ transition layers behind. The host can capture from inside the existing 2-arg callback.The helper-based design fixes all three:
CallbackTcontinues to compile and behave identically. Hosts that don't care about stacks need not change a line.new Error().stackruns only when the host invokes the helper -- per-call, per-level, per-call-site, fully host-controlled.new Error()capture skips the Napi wrapper frame already). Hosts don't need to know it.Internally the helper is currently implemented via N-API
new Error().stack; when a cheap N-API stack-capture primitive lands later, the body can be swapped without touching the public API.Tests
New
Console.CaptureCurrentJsStackgtest inTests/UnitTests/Shared/Shared.cpp: registers a 2-argCallbackTthat callsCaptureCurrentJsStack(env)inside its body, asserts bothconsole.errorandconsole.logpaths yield a non-empty stack.In-tree consumer updates
Tests/UnitTests/Shared/Shared.cpp-- bothConsole::Initializelambdas use the helper.Polyfills/Console/Readme.md-- documents the helper alongside the existingInitializeexample.