Subj : sbbs3: add SQLite JavaScript class
To   : GitLab note in main/sbbs
From : Rob Swindell
Date : Fri May 22 2026 21:03:05

https://gitlab.synchro.net/main/sbbs/-/merge_requests/681#note_9019

Code review pass. Overall the design is solid: clean separation of `SQLite` / `SQLiteStatement` / `SQLiteRow` (live view) / `SQLiteValue` (eager snapshot) / `SQLiteTable` / `SQLiteRecord`; injection-resistant API (exec rejects params, query/run require params when SQL has placeholders, all binding through `sqlite3_bind_*`, no escape helpers exposed); lifetime via `__sqlite_parent` rooting + generation counter for stale-row detection; `JS_SUSPENDREQUEST`/`RESUMEREQUEST` around every blocking SQLite call; `sqlite3_close_v2` so child statements stay valid after `db.close()`; thorough JSDOC strings and a comprehensive constants set.

A few findings, ranked.

### Strategic

**No Windows build wiring.** Diff updates `GNUmakefile`, `objects.mk`, and `CMakeLists.txt`, but `sbbs.vcxproj` is untouched. On Windows `js_sqlite.cpp` is simply never compiled, and the `#else` stub at `src/sbbs3/js_sqlite.cpp:4307` keeps the exported symbol satisfied Ä so it builds clean but the feature is silently absent. If Windows is in scope, the `.vcxproj` needs `js_sqlite.cpp` added and sqlite3 linked. If Linux-only is intentional, worth saying so in the MR description.

### Bugs

**1. Possible null deref on `transaction(null)` / `forEach(null)`** Ä `src/sbbs3/js_sqlite.cpp:1190` and `:1740`:
```cpp
if (argc < 1 || !JSVAL_IS_OBJECT(argv[0])
    || !JS_ObjectIsFunction(cx, JSVAL_TO_OBJECT(argv[0]))) { ... }
```
SM 1.8.5's `JSVAL_IS_OBJECT(JSVAL_NULL)` returns true (legacy quirk), so `null` passes the first check. `JSVAL_TO_OBJECT(JSVAL_NULL)` returns `NULL`, and `JS_ObjectIsFunction(cx, NULL)` derefs. The same idiom in `set_log` at `:2358` *does* guard `!JSVAL_IS_NULL(in)` first Ä that one is correct; mirror it in the other two.

**2. Duplicate `JS_IsArrayObject` call** Ä `src/sbbs3/js_sqlite.cpp:855-856`:
```cpp
JSBool is_array = JS_FALSE;
JS_IsArrayObject(cx, o);          // result discarded
is_array = JS_IsArrayObject(cx, o);
```
First call is dead; looks like a copy-paste artifact. Harmless but should go.

### Minor / consistency

**3. Unchecked `JS_NewStringCopyZ`/`JS_NewStringCopyN`** in several array-building paths: `js_sqlite_row_get` at `:612`, `js_sqlite_stmt_get` (`STMT_PROP_COLUMN_NAMES`) at `:1297`, `js_conn_columns` at `:1973`/`:1978`/`:1989`. Pattern when alloc fails:
```cpp
JSString* s = JS_NewStringCopyZ(cx, cn ? cn : "");
jsval v = STRING_TO_JSVAL(s);     // STRING_TO_JSVAL(NULL)  bad jsval
JS_SetElement(cx, arr, i, &v);    // stores garbage / masks the real OOM exception
```
Other sites in the same file *do* check (`:2848`, `:3821`, `js_conn_tables:1814`). Worth normalizing.

**4. `js_conn_transaction` if the callback closes the connection** Ä `:1757-1764`. If `argv[0]` calls `db.close()`, then `p->db` is `NULL` and the subsequent COMMIT/ROLLBACK `sqlite3_exec(p->db, ...)` hits `SQLITE_MISUSE`. Not a crash (SQLite's MISUSE path is NULL-safe), but the "ok" branch then throws something confusing. Re-check `p->db != NULL` after the call before issuing the commit/rollback.

**5. `sqlite3_bind_text` return value ignored** in `table_load_schema` Ä `:2610`, `:2635`, `:2698`. With `SQLITE_TRANSIENT` and a known-valid index these can only really fail on OOM, but the bare-call style is inconsistent with the rest of the file.

**6. Schema cache is never invalidated.** Design choice already documented at the struct doc on `:119` ("scripts that need to see post-DDL schema changes must reopen the connection"), but the `db.table` namespace JSDOC at `:4228` doesn't mention it. Users hitting `db.exec("ALTER TABLE t ADD COLUMN x ...")` followed by `db.table.t.x` will get surprised. A one-line addition to that docstring would close the gap.

### Things checked and OK

- Integer/double dispatch in `bind_jsval` with the 2^53 safe-int boundary.
- `INTEGER PRIMARY KEY`  rowid-alias detection (correctly excludes `INT`, `BIGINT`, composite PKs).
- `sql_quote_ident` worst-case sizing (`name*2 + 3`).
- `record_apply_defaults` running schema-derived SQL (explicitly trusted, commented).
- All malloc'd buffer-size calcs in `record_insert`/`update`/`remove`, `table_load_by_pk`, `table_build_select_prefix`, `table_select` Ä tight but exact.
- `pthread_once` install of the global log handler.
- `set_log` thread guard (`pthread_equal` against installing thread) and JS-VM cross-thread suppression.
- Rooting pattern (`__sqlite_parent`) keeping conns alive past statements and statements past rows.

Ä *Authored by Claude (Claude Code), on behalf of @rswindell*

---
 þ Synchronet þ Vertrauen þ Home of Synchronet þ [vert/cvs/bbs].synchro.net

.