From 7322a95a9db80e9d43df13a0af1614b40a24c5ad Mon Sep 17 00:00:00 2001 From: grom Date: Mon, 19 Oct 2026 20:58:03 -0600 Subject: [PATCH] =?UTF-8?q?fix(ui):=20WOW=5FUI=5FVERBS=20must=20not=20wrap?= =?UTF-8?q?=20setfenv=20=E2=80=94=20it=20unloaded=20a=20third=20of=20the?= =?UTF-8?q?=20interface?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A wrapper puts its own frame between the caller and the callee, which silently changes the meaning of every stack-level-relative call. `verb_prof::install` wraps every function in `_G`, and `_G` at that point holds Lua's own stdlib as well as our bindings — so it wrapped `setfenv`. pfUI's `api/api.lua:4` is `setfenv(1, pfUI:GetEnvironment())`, and level 1 from inside a wrapper is the wrapper, a C function, so Lua raises ``setfenv' cannot change environment of given object`. One armed session logged **91** of those and lost pfUI, WhatsTraining_Turtle, AI_VoiceOver, MasterTradeSkills, pfUI-addonskinner, pfUI-locplus and PizzaSlices: an instrument that unloaded a third of the interface it was there to measure, and whose numbers were therefore of a client without them. `STACK_RELATIVE` names the four `wrap_table` must leave alone. `setfenv` and `getfenv` take a level; `error(msg, level)` positions its message by one; and `debugstack([start])` is the reference's traceback by level, which `FuBarPlugin-2.0.lua:752` asks for as `debugstack(6, 1, 0)` and `AceDB-2.0.lua:742` reads a specific line of. `pcall`/`xpcall` stay wrapped: no level argument, and an armed session shows them working and worth seeing. The test wraps `_G` and then runs pfUI's line, so it is the bug rather than a restatement of the list; deleting the skip reproduces grom's exact error. The module doc also claimed `_G` "holds our bindings and nothing else" at install time, which is what made this easy to miss. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn --- crates/benilla-ui/src/script/verb_prof.rs | 49 ++++++++++++++++++++++- 1 file changed, 47 insertions(+), 2 deletions(-) diff --git a/crates/benilla-ui/src/script/verb_prof.rs b/crates/benilla-ui/src/script/verb_prof.rs index 8a61bee8..afb5d77b 100644 --- a/crates/benilla-ui/src/script/verb_prof.rs +++ b/crates/benilla-ui/src/script/verb_prof.rs @@ -8,8 +8,9 @@ //! (`CreateFrame`, `SetPoint`, `SetTexture`, `Show`), and nothing charged it to them. //! //! **How it attaches.** Armed, [`install`] walks `_G` and every widget method table at the end of -//! [`UiScript::new`](super::UiScript::new) — before any FrameXML or addon runs, so everything it -//! finds is ours — and replaces each function with a closure that times the call. A verb that +//! [`UiScript::new`](super::UiScript::new) — before any FrameXML or addon runs, so every function +//! it finds is one of ours or one of Lua's own, never an addon's — and replaces each with a closure +//! that times the call. Lua's own are in there too, which is why [`STACK_RELATIVE`] exists. A verb that //! calls back into Lua (a handler fired from inside `Show`) has that nested time inside its own, //! as the handler profiler's *total* column does; the two instruments read against each other. //! @@ -234,6 +235,23 @@ pub fn reset() { *ROWS.lock().unwrap_or_else(|p| p.into_inner()) = None; } +/// Functions [`wrap_table`] must leave alone: each reads the **call stack by level**, and a +/// wrapper puts its own frame between the caller and the callee, so the level means something +/// else. +/// +/// `setfenv` is the one that bit, in grom's 2026-10-19 session. pfUI's `api/api.lua:4` is +/// `setfenv(1, pfUI:GetEnvironment())`; level 1 from inside a wrapper is the wrapper, which is a C +/// function, so Lua raises ``setfenv' cannot change environment of given object`. That one armed +/// run logged **91** of those and took pfUI, AI_VoiceOver, WhatsTraining_Turtle, MasterTradeSkills +/// and others down with it — the instrument unloaded a third of the interface it was there to +/// measure, and the session's numbers were of a client without them. +/// +/// `error(msg, level)` positions the message by level, and `debugstack([start])` is the reference's +/// traceback by level — `FuBarPlugin-2.0.lua:752` asks for `debugstack(6, 1, 0)` and +/// `AceDB-2.0.lua:742` reads a specific line of it. `pcall`/`xpcall` stay wrapped: they take no +/// level, and one armed session shows them working and worth seeing (pfUI's feature probes). +const STACK_RELATIVE: &[&str] = &["setfenv", "getfenv", "error", "debugstack"]; + /// Wrap one table's function values in place, naming each ``. fn wrap_table(lua: &Lua, table: &Table, prefix: &str) -> mlua::Result { let mut wrapped = 0; @@ -247,6 +265,9 @@ fn wrap_table(lua: &Lua, table: &Table, prefix: &str) -> mlua::Result { }) .collect(); for key in keys { + if STACK_RELATIVE.contains(&key.as_str()) { + continue; + } let Ok(Value::Function(orig)) = table.raw_get::(key.as_str()) else { continue; }; @@ -318,4 +339,28 @@ mod tests { rows[0].micros ); } + + /// The 2026-10-19 regression, as a falsifier. Wrapping `_G` wholesale took `setfenv` with it, + /// and `setfenv(1, env)` from a chunk then raised ``setfenv' cannot change environment of + /// given object` — level 1 was the wrapper, a C function. pfUI's `api/api.lua:4` is exactly + /// this line, which is why one armed session cost 91 load errors. + #[test] + fn wrapping_the_globals_spares_the_stack_relative_ones() { + let lua = mlua::Lua::new(); + super::wrap_table(&lua, &lua.globals(), "").expect("wrap _G"); + // The globals are bound to locals first: after `setfenv` the chunk's own lookups go + // through the new environment, where they are not. + let answered: bool = lua + .load( + r#" + local setfenv, getfenv = setfenv, getfenv + local env = { ok = true } + setfenv(1, env) + return ok and getfenv(1) == env + "#, + ) + .eval() + .expect("setfenv(1, …) must still reach the calling chunk"); + assert!(answered, "the chunk got someone else's environment"); + } }