fix(native): keep snapshot() from running JS while it uses the terminal

native/README.md

@@ -33,15 +33,15 @@ C++ declarations:
 - `class TerminalWrap final : public Napi::ObjectWrap<TerminalWrap>`, with the members `GhosttyTerminal terminal_ = nullptr`, `GhosttyMouseEncoder mouse_encoder_ = nullptr`, `mouse_modes_dirty_` (starts `true`), `mouse_size_configured_`, and `mouse_size_`, and the free functions `CreateTerminal` and `GetNativeInfo` [native/terminal.hh:TerminalWrap]
 - Shim helpers `ResultName`, `ResultMessage`, `GhosttyStringToString`, `BuildInfoString`, `Utf8FromCodepoint`, and `HexColor`. The header defines `GHOSTTY_STATIC` when it is not defined, then includes `<ghostty/vt.h>` [native/libghostty_vt_shim.hh:ResultMessage]
 - `ResultMessage(op, result)` returns `"<op> failed: <ResultName>"` [native/libghostty_vt_shim.cc:ResultMessage]. `ResultName` maps unknown codes to `GHOSTTY_UNKNOWN_RESULT` [native/libghostty_vt_shim.cc:ResultName]
-- Private helpers: `BuildLines` (rows of one point tag), `ReadLine`, `ReadCellText` (grapheme or single-codepoint text), `ResolveStyleColor`, `OptionBool`, and `ThrowResult` [native/terminal.cc:TerminalWrap::BuildLines] [native/terminal.cc:TerminalWrap::ReadCellText]. Mouse helpers: `RequireMouseEncoder`, `PositiveUint32`, `NonNegativeUint32`, `FiniteFloat`, `ValidateMouseAxis`, `MouseCoordinate`, `ValidateUtf8MouseCoordinate`, `ParseMouseAction`, `ParseMouseButton`, `ParseMouseModifiers`, and `MouseSizeEqual` [native/terminal.cc:MouseCoordinate]
+- Private helpers: `BuildLines` (rows of one point tag), `ReadLine`, `ReadCellText` (grapheme or single-codepoint text), `ResolveStyleColor`, `OptionBool`, `DefineValue`, `DefineIndex`, and `ThrowResult` [native/terminal.cc:TerminalWrap::BuildLines] [native/terminal.cc:TerminalWrap::ReadCellText]. Mouse helpers: `RequireMouseEncoder`, `PositiveUint32`, `NonNegativeUint32`, `FiniteFloat`, `ValidateMouseAxis`, `MouseCoordinate`, `ValidateUtf8MouseCoordinate`, `ParseMouseAction`, `ParseMouseButton`, `ParseMouseModifiers`, and `MouseSizeEqual` [native/terminal.cc:MouseCoordinate]
 
 ## Invariants
 
 - `createTerminal` and the constructor throw `TypeError("createTerminal options must be an object")` unless they get exactly one object argument [native/terminal.cc:CreateTerminal].
 - `cols` and `rows`: a non-number throws `TypeError("<name> must be a positive integer")`. `NaN`, an infinity, a value below 1 or above 65535, or a non-integer throws `RangeError("<name> must be a positive 16-bit integer")`. The range check runs before the value is cast to `uint16_t` [native/terminal.cc:PositiveUint16].
 - `scrollbackLimit` is skipped when it is absent or `undefined`, and then defaults to 0. A non-number throws `TypeError`. `NaN`, an infinity, a negative value, a non-integer, or a value too large for `size_t` (2^64 or more on 64-bit targets) throws `RangeError`. Both use `"scrollbackLimit must be a non-negative integer"`, and the range check runs before the cast [native/terminal.cc:NonNegativeSize]. The value becomes Ghostty's scrollback byte limit (`GHOSTTY_TERMINAL_OPT_SCROLLBACK_MAX_BYTES`), so 0 keeps no scrollback. Ghostty prunes by whole pages, so a small nonzero limit still keeps some history [native/terminal.cc:TerminalWrap::TerminalWrap].
 - Most failed libghostty-vt calls throw an `Error` with `ResultMessage` text, for example `"ghostty_terminal_new failed: GHOSTTY_OUT_OF_MEMORY"` [native/terminal.cc:ThrowResult]. These do not throw: a failed palette read leaves the color out, a failed `ghostty_build_info` leaves the field out, a first `ghostty_grid_ref_graphemes` call that does not return `GHOSTTY_OUT_OF_SPACE` with `required > 0` falls back to the single codepoint, and the code checks no result from `ghostty_terminal_vt_write` [native/terminal.cc:TerminalWrap::ResolveStyleColor] [native/terminal.cc:TerminalWrap::ReadCellText] [native/terminal.cc:TerminalWrap::Feed].
-- Every method except `dispose` and `encodeMouse` calls `RequireTerminal` first. After dispose it throws `Error("GhosttyVtTerminal has been disposed")` [native/terminal.cc:TerminalWrap::RequireTerminal]. With `NAPI_CPP_EXCEPTIONS`, `RequireTerminal` reports a disposed terminal by throwing.
+- Every method except `dispose`, `encodeMouse`, and `snapshot` calls `RequireTerminal` first. After dispose it throws `Error("GhosttyVtTerminal has been disposed")` [native/terminal.cc:TerminalWrap::RequireTerminal]. With `NAPI_CPP_EXCEPTIONS`, `RequireTerminal` reports a disposed terminal by throwing.
 - `encodeMouse` reads and checks every argument first, and only then calls `RequireTerminal` and `RequireMouseEncoder` (`"GhosttyVtTerminal has been disposed"`). `Get()` runs getters, also getters on `Object.prototype` for keys that the caller leaves out, and such JS can dispose the terminal or call `encodeMouse` again. After the handles are taken, no JS runs until the call returns [native/terminal.cc:TerminalWrap::EncodeMouse].
 - `encodeMouse` throws `TypeError("encodeMouse expects event and options objects")` unless it gets exactly two objects, and `TypeError("mouse geometry must be an object")` for a non-object `geometry` [native/terminal.cc:TerminalWrap::EncodeMouse]. `screenWidth`, `screenHeight`, `cellWidth`, and `cellHeight` must be integers from 1 to 2^32-1, and the four paddings integers from 0 to 2^32-1: a non-number throws `TypeError("<name> must be a positive integer")` or `"... non-negative integer"`, and any other bad value `RangeError("<name> must be a positive 32-bit integer")` or `"... non-negative 32-bit integer"` [native/terminal.cc:PositiveUint32] [native/terminal.cc:NonNegativeUint32].
 - `encodeMouse` geometry limits, the same calculation that Ghostty makes before its unchecked casts: the padding sum of an axis, added in 64 bits, must not exceed the screen size (`RangeError("mouse geometry padding exceeds screenWidth")`, or `screenHeight`), and `f32(screen - padding) / f32(cell)` must be below 65536 (`RangeError("mouse geometry.screenWidth exceeds the 65535-cell limit")`, or `screenHeight`) [native/terminal.cc:ValidateMouseAxis].
@@ -51,7 +51,7 @@ C++ declarations:
 - Mouse mode cache: a non-empty `feed` sets `mouse_modes_dirty_`. The next `encodeMouse` that passes its checks then reloads the tracking mode and format with `ghostty_mouse_encoder_setopt_from_terminal`. The pinned Ghostty also resets its same-cell motion deduplication there (see "Mouse encoding" in the root README). A call that throws leaves `mouse_modes_dirty_` set. `encodeMouse` sets the size option only when the geometry differs from the last one [native/terminal.cc:TerminalWrap::Feed] [native/terminal.cc:TerminalWrap::EncodeMouse].
 - `feed` throws `TypeError` for an argument count other than 1 (`"feed expects exactly one argument"`), for a typed array that is not a `Uint8Array` (`"feed typed array must be Uint8Array"`), and for any other value (`"feed data must be a string, Buffer, or Uint8Array"`). Zero-length input returns without a libghostty-vt call [native/terminal.cc:TerminalWrap::Feed].
 - `resize` throws `TypeError("resize expects cols and rows")` for an argument count other than 2. It checks `cols` and `rows` as the constructor does [native/terminal.cc:TerminalWrap::Resize].
-- `snapshot` throws `TypeError("snapshot options must be an object")` for a defined non-object argument [native/terminal.cc:TerminalWrap::Snapshot]. `includeScrollback` and `includeCells` count only when they are boolean `true` [native/terminal.cc:OptionBool].
+- `snapshot` throws `TypeError("snapshot options must be an object")` for a defined non-object argument [native/terminal.cc:TerminalWrap::Snapshot]. `includeScrollback` and `includeCells` count only when they are boolean `true` [native/terminal.cc:OptionBool]. `snapshot` reads both options with `Get()` before it calls `RequireTerminal`, because a getter on `Object.prototype` can dispose the terminal during that read. It builds the result with `DefineValue` and `DefineIndex`, which define own data properties, so no setter on `Object.prototype` or `Array.prototype` runs while it uses `terminal_` [native/terminal.cc:TerminalWrap::Snapshot] [native/terminal.cc:DefineValue].
 - `snapshot`, `getVisibleText`, `formatPlain`, and `formatHtml` catch `std::exception` from the internal helpers and rethrow it as a JavaScript `Error` with the same message [native/terminal.cc:TerminalWrap::Snapshot].
 - `dispose` is idempotent. `DisposeNative` frees `mouse_encoder_` when it is not `nullptr`, then `terminal_` when it is not `nullptr`, and sets each to `nullptr` after it frees it [native/terminal.cc:TerminalWrap::DisposeNative].
 - Per-environment state: `Init` keeps the class constructor in the env's instance data, not in a static, so the main thread and each `worker_threads` Worker that loads the addon have their own. `Init` asserts that the instance data is unset before it sets it [native/terminal.cc:TerminalWrap::Init]. `Create` asserts that the constructor is set [native/terminal.cc:TerminalWrap::Create].
@@ -76,5 +76,5 @@ C++ declarations:
 - Run (after `npm ci`): `npm run build:libghostty && npm run build:native && npm test`
 - Every native test skips when the addon does not load (`test/native-support.ts:nativeSupport`), so check the test output for skips.
 - `mise run ci-ubsan` builds the addon with UBSan (Linux, GCC), then runs the tests and smoke. Undefined behavior aborts the run. The task also fails when the addon's object files contain no UBSan checks, which means the flags did not reach g++ (`mise.toml:tasks.ci-ubsan`).
-- `test/workers.test.ts` checks the per-environment state in child processes with `worker_threads`. `test/terminal.test.ts` covers `feed`, `snapshot`, `resize`, `dispose`, the `RangeError` cases of the `createTerminal` checks in the addon, the `encodeMouse` bytes of each mouse format, `cursorVisible`, the cell style fields, DECSTR, and whether `scrollbackLimit` keeps history. `test/mouse-safety.test.ts` runs the `encodeMouse` limits and the getter re-entry cases against both the package API and the raw addon object. It skips when the addon is missing, like the other native suites. Its child-process cases load `src/` through `tsx`, so they need no `dist/` build. No vitest test covers `formatPlain`, `formatHtml`, the content of `scrollbackLines`, or the typed-array errors.
+- `test/workers.test.ts` checks the per-environment state in child processes with `worker_threads`. `test/terminal.test.ts` covers `feed`, `snapshot`, `resize`, `dispose`, the `RangeError` cases of the `createTerminal` checks in the addon, the `encodeMouse` bytes of each mouse format, `cursorVisible`, the cell style fields, DECSTR, and whether `scrollbackLimit` keeps history. `test/snapshot-safety.test.ts` checks in child processes that `Object.prototype` accessors cannot dispose the terminal mid-`snapshot`. `test/mouse-safety.test.ts` runs the `encodeMouse` limits and the getter re-entry cases against both the package API and the raw addon object. It skips when the addon is missing, like the other native suites. Its child-process cases load `src/` through `tsx`, so they need no `dist/` build. No vitest test covers `formatPlain`, `formatHtml`, the content of `scrollbackLines`, or the typed-array errors.
 - For changes to `TerminalWrap` or `InitAll`, also run the TLA+ lifecycle model: [`.agents/skills/verify-native-lifecycle/SKILL.md`](https://github.com/coder/libghostty-vt-node/blob/main/.agents/skills/verify-native-lifecycle/SKILL.md).

native/terminal.cc

@@ -5,6 +5,7 @@
 #include <cmath>
 #include <limits>
 #include <stdexcept>
+#include <string>
 
 namespace libghostty_vt_node {
 namespace {
@@ -151,6 +152,18 @@ bool OptionBool(const Napi::Object& options, const char* name) {
   return value.IsBoolean() && value.As<Napi::Boolean>().Value();
 }
 
+// Set() runs a setter that Object.prototype or Array.prototype can carry, and
+// that JS can dispose the terminal mid-snapshot. Defining the property cannot.
+void DefineValue(Napi::Env env, napi_value object, const char* key, napi_value value) {
+  const napi_property_descriptor descriptor = {
+      key, nullptr, nullptr, nullptr, nullptr, value, napi_default_jsproperty, nullptr};
+  NAPI_THROW_IF_FAILED_VOID(env, napi_define_properties(env, object, 1, &descriptor));
+}
+
+void DefineIndex(Napi::Env env, napi_value array, uint32_t index, napi_value value) {
+  DefineValue(env, array, std::to_string(index).c_str(), value);
+}
+
 GhosttyMods ParseMouseModifiers(Napi::Env env, const Napi::Value& value) {
   if (value.IsUndefined()) return 0;
   if (!value.IsObject()) throw Napi::TypeError::New(env, "mouse modifiers must be an object");
@@ -472,8 +485,6 @@ Napi::Value TerminalWrap::EncodeMouse(const Napi::CallbackInfo& info) {
 
 Napi::Value TerminalWrap::Snapshot(const Napi::CallbackInfo& info) {
   Napi::Env env = info.Env();
-  RequireTerminal(env);
-
   bool include_scrollback = false;
   bool include_cells = false;
   if (info.Length() > 0 && !info[0].IsUndefined()) {
@@ -484,6 +495,8 @@ Napi::Value TerminalWrap::Snapshot(const Napi::CallbackInfo& info) {
     include_scrollback = OptionBool(options, "includeScrollback");
     include_cells = OptionBool(options, "includeCells");
   }
+  // The Get() reads above can run JS that disposes this terminal.
+  RequireTerminal(env);
 
   try {
     return BuildSnapshot(env, include_scrollback, include_cells);
@@ -600,19 +613,19 @@ Napi::Object TerminalWrap::BuildSnapshot(Napi::Env env, bool include_scrollback,
   for (uint32_t row = 0; row < rows; row++) {
     const std::string text = ReadLine(GHOSTTY_POINT_TAG_ACTIVE, row, include_cells ? &cells : nullptr, env);
     Napi::Object line = Napi::Object::New(env);
-    line.Set("row", Napi::Number::New(env, row));
-    line.Set("text", Napi::String::New(env, text));
-    visible_lines.Set(row, line);
+    DefineValue(env, line, "row", Napi::Number::New(env, row));
+    DefineValue(env, line, "text", Napi::String::New(env, text));
+    DefineIndex(env, visible_lines, row, line);
   }
 
   Napi::Object snapshot = Napi::Object::New(env);
-  snapshot.Set("cols", Napi::Number::New(env, cols));
-  snapshot.Set("rows", Napi::Number::New(env, rows));
-  snapshot.Set("cursorRow", Napi::Number::New(env, cursor_row));
-  snapshot.Set("cursorCol", Napi::Number::New(env, cursor_col));
-  snapshot.Set("cursorVisible", Napi::Boolean::New(env, cursor_visible));
-  snapshot.Set("isAltScreen", Napi::Boolean::New(env, active_screen == GHOSTTY_TERMINAL_SCREEN_ALTERNATE));
-  snapshot.Set("visibleLines", visible_lines);
+  DefineValue(env, snapshot, "cols", Napi::Number::New(env, cols));
+  DefineValue(env, snapshot, "rows", Napi::Number::New(env, rows));
+  DefineValue(env, snapshot, "cursorRow", Napi::Number::New(env, cursor_row));
+  DefineValue(env, snapshot, "cursorCol", Napi::Number::New(env, cursor_col));
+  DefineValue(env, snapshot, "cursorVisible", Napi::Boolean::New(env, cursor_visible));
+  DefineValue(env, snapshot, "isAltScreen", Napi::Boolean::New(env, active_screen == GHOSTTY_TERMINAL_SCREEN_ALTERNATE));
+  DefineValue(env, snapshot, "visibleLines", visible_lines);
 
   if (include_scrollback) {
     size_t scrollback_rows = 0;
@@ -621,15 +634,15 @@ Napi::Object TerminalWrap::BuildSnapshot(Napi::Env env, bool include_scrollback,
       throw std::runtime_error(ResultMessage("ghostty_terminal_get(scrollback rows)", result));
     }
     const uint32_t count = static_cast<uint32_t>(std::min<size_t>(scrollback_rows, std::numeric_limits<uint32_t>::max()));
-    snapshot.Set("scrollbackLines", BuildLines(env, GHOSTTY_POINT_TAG_HISTORY, count, 0));
+    DefineValue(env, snapshot, "scrollbackLines", BuildLines(env, GHOSTTY_POINT_TAG_HISTORY, count, 0));
   }
 
   if (include_cells) {
     Napi::Array cell_array = Napi::Array::New(env, cells.size());
     for (size_t i = 0; i < cells.size(); i++) {
-      cell_array.Set(static_cast<uint32_t>(i), cells[i]);
+      DefineIndex(env, cell_array, static_cast<uint32_t>(i), cells[i]);
     }
-    snapshot.Set("cells", cell_array);
+    DefineValue(env, snapshot, "cells", cell_array);
   }
 
   return snapshot;
@@ -639,9 +652,9 @@ Napi::Array TerminalWrap::BuildLines(Napi::Env env, GhosttyPointTag tag, uint32_
   Napi::Array lines = Napi::Array::New(env, rows);
   for (uint32_t row = 0; row < rows; row++) {
     Napi::Object line = Napi::Object::New(env);
-    line.Set("row", Napi::Number::New(env, static_cast<int64_t>(row) + row_offset));
-    line.Set("text", Napi::String::New(env, ReadLine(tag, row, nullptr, env)));
-    lines.Set(row, line);
+    DefineValue(env, line, "row", Napi::Number::New(env, static_cast<int64_t>(row) + row_offset));
+    DefineValue(env, line, "text", Napi::String::New(env, ReadLine(tag, row, nullptr, env)));
+    DefineIndex(env, lines, row, line);
   }
   return lines;
 }
@@ -735,14 +748,14 @@ Napi::Object TerminalWrap::BuildCellObject(
     GhosttyCell cell,
     const GhosttyGridRef& ref) {
   Napi::Object obj = Napi::Object::New(env);
-  obj.Set("row", Napi::Number::New(env, row));
-  obj.Set("col", Napi::Number::New(env, col));
-  obj.Set("text", Napi::String::New(env, text));
+  DefineValue(env, obj, "row", Napi::Number::New(env, row));
+  DefineValue(env, obj, "col", Napi::Number::New(env, col));
+  DefineValue(env, obj, "text", Napi::String::New(env, text));
 
   GhosttyCellWide wide = GHOSTTY_CELL_WIDE_NARROW;
   GhosttyResult result = ghostty_cell_get(cell, GHOSTTY_CELL_DATA_WIDE, &wide);
   if (result != GHOSTTY_SUCCESS) throw std::runtime_error(ResultMessage("ghostty_cell_get(wide)", result));
-  obj.Set("width", Napi::Number::New(env, wide == GHOSTTY_CELL_WIDE_WIDE ? 2 : 1));
+  DefineValue(env, obj, "width", Napi::Number::New(env, wide == GHOSTTY_CELL_WIDE_WIDE ? 2 : 1));
 
   bool has_styling = false;
   result = ghostty_cell_get(cell, GHOSTTY_CELL_DATA_HAS_STYLING, &has_styling);
@@ -754,19 +767,19 @@ Napi::Object TerminalWrap::BuildCellObject(
   result = ghostty_grid_ref_style(&ref, &style);
   if (result != GHOSTTY_SUCCESS) throw std::runtime_error(ResultMessage("ghostty_grid_ref_style", result));
 
-  if (style.bold) obj.Set("bold", Napi::Boolean::New(env, true));
-  if (style.italic) obj.Set("italic", Napi::Boolean::New(env, true));
-  if (style.underline != 0) obj.Set("underline", Napi::Boolean::New(env, true));
-  if (style.faint) obj.Set("faint", Napi::Boolean::New(env, true));
+  if (style.bold) DefineValue(env, obj, "bold", Napi::Boolean::New(env, true));
+  if (style.italic) DefineValue(env, obj, "italic", Napi::Boolean::New(env, true));
+  if (style.underline != 0) DefineValue(env, obj, "underline", Napi::Boolean::New(env, true));
+  if (style.faint) DefineValue(env, obj, "faint", Napi::Boolean::New(env, true));
   // Raw SGR 7 flag. Foreground and background are reported unswapped.
-  if (style.inverse) obj.Set("inverse", Napi::Boolean::New(env, true));
-  if (style.invisible) obj.Set("invisible", Napi::Boolean::New(env, true));
-  if (style.strikethrough) obj.Set("strikethrough", Napi::Boolean::New(env, true));
+  if (style.inverse) DefineValue(env, obj, "inverse", Napi::Boolean::New(env, true));
+  if (style.invisible) DefineValue(env, obj, "invisible", Napi::Boolean::New(env, true));
+  if (style.strikethrough) DefineValue(env, obj, "strikethrough", Napi::Boolean::New(env, true));
 
   const std::string fg = ResolveStyleColor(style.fg_color);
   const std::string bg = ResolveStyleColor(style.bg_color);
-  if (!fg.empty()) obj.Set("foreground", Napi::String::New(env, fg));
-  if (!bg.empty()) obj.Set("background", Napi::String::New(env, bg));
+  if (!fg.empty()) DefineValue(env, obj, "foreground", Napi::String::New(env, fg));
+  if (!bg.empty()) DefineValue(env, obj, "background", Napi::String::New(env, bg));
 
   return obj;
 }