From 09aca72c464a13c9ea62efbc89d297b689ab4a47 Mon Sep 17 00:00:00 2001 From: grom Date: Mon, 2 Nov 2026 11:10:57 -0700 Subject: [PATCH] ui: hooksecurefunc over a widget method has to read through the metatable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The table form read the field with `raw_get`, on the reasoning that a field reached only through a metatable is not that table's to replace. A widget carries none of its own methods — they live on the shared class table its metatable's `__index` names (`object.rs`'s `frame_meta_for`) — so `hooksecurefunc(frame, "SetAlpha", cb)` found nothing there and armed nothing, silently. Measured: **0 fires across 2 `SetAlpha` calls**, while the plain-table and global forms fired correctly. That is not a corner of the form, it is what the form is for. 29 files in grom's corpus hook a widget method, and the one that shows: `Cinematic/Frames.lua:459` hooks each pfUI frame's own `SetAlpha` so that Cinematic can **follow** pfUI's out-of-combat fade rather than fight it ("their frames have SetAlpha followed straight away by ours, taking what it set as the new base"). With the hook unarmed the two wrote a different alpha to the same frame on alternating frames — which is grom's "still flashing my unitframe when cinematic is on". The reference reads `table[name]` plainly and assigns the wrapper back, which *shadows* the class method for that one object and leaves its siblings alone. This now does the same: read through `__index`, write raw onto the host, which is the same thing a plain assignment would do since frame metatables carry no `__newindex`. A test pins both halves — the chain fires on the hooked frame, and a sibling of the same kind is untouched, with nothing written to the class table. Corpus roster byte-identical (87 ok / 1 fail) with all 29 files' hooks armed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn --- .../src/script/compat/classic_api/hooks.rs | 66 ++++++++++++++++++- docs/ADDON_COMPAT.md | 2 +- 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/crates/benilla-ui/src/script/compat/classic_api/hooks.rs b/crates/benilla-ui/src/script/compat/classic_api/hooks.rs index 4416c266..ff34898f 100644 --- a/crates/benilla-ui/src/script/compat/classic_api/hooks.rs +++ b/crates/benilla-ui/src/script/compat/classic_api/hooks.rs @@ -60,8 +60,25 @@ fn hooksecurefunc(lua: &Lua, args: MultiValue) -> mlua::Result<()> { return Ok(()); // not the documented shape; nothing to arm }; let name = name.to_str()?.to_string(); - // `raw_get`, not `get`: a field reached only through a metatable is not this table's to replace. - let Value::Function(orig) = host.raw_get::(name.as_str())? else { + // Read **through** the metatable, write onto the host itself — which is the reference's own + // `local orig = table[name]` followed by `table[name] = wrapper`, and the only reading of + // this form that works on a widget. A frame carries none of its methods: they live on the + // shared class table its metatable's `__index` names (`object.rs`'s `frame_meta_for`), so a + // raw read of `frame.SetAlpha` finds nothing and the hook silently never arms. That is not + // the rare case, it is the *common* one — the two-argument form exists for widgets. + // + // Measured 2026-11-02, before this: `hooksecurefunc(f, "SetAlpha", cb)` fired **0 times** + // across two `SetAlpha` calls, while the plain-table and global forms fired correctly. The + // frame-by-frame symptom was grom's unit frame strobing whenever Cinematic was on: + // `Cinematic/Frames.lua:459` hooks each pfUI frame's own `SetAlpha` precisely so it can + // *follow* pfUI's out-of-combat fade instead of fighting it, and with the hook unarmed the + // two wrote a different alpha to the same frame on alternating frames. 29 files in the + // corpus use this form, openmap's canvas, pin and data-provider hooks among them. + // + // The write stays raw: it shadows the class method for this one object, which is what the + // reference does and what makes a per-frame hook possible at all. Frame metatables carry no + // `__newindex`, so raw and plain assignment agree here anyway. + let Value::Function(orig) = host.get::(name.as_str())? else { return Ok(()); }; host.raw_set(name, chained(lua, orig, hook)?) @@ -104,6 +121,51 @@ mod tests { assert_eq!(s.eval::("return seen").unwrap(), "2/3"); } + /// The two-argument form over a **widget method**, which is what that form is for: a frame + /// carries none of its own methods, so the original has to be read through the metatable and + /// the wrapper written onto the frame, shadowing the class method for this one object. + /// + /// Read raw instead, the hook silently never armed — measured as 0 fires for 2 calls. The + /// symptom was a strobing unit frame: `Cinematic/Frames.lua:459` hooks each pfUI frame's + /// `SetAlpha` so it can follow pfUI's own fade, and an unarmed hook left the two writing a + /// different alpha to the same frame on alternating frames. + #[test] + fn the_two_argument_form_arms_on_a_widget_method_and_leaves_its_siblings_alone() { + let s = UiScript::new().unwrap(); + s.run( + r#" + f = CreateFrame("Frame", "HookedFrame", UIParent) + other = CreateFrame("Frame", "UnhookedFrame", UIParent) + seen, others = 0, 0 + hooksecurefunc(f, "SetAlpha", function(self, a) seen = seen + 1 end) + hooksecurefunc(f, "SetAlpha", function(self, a) seen = seen + 10 end) + f:SetAlpha(0.5) + other:SetAlpha(0.5) + "#, + ) + .unwrap(); + + assert_eq!( + s.eval::("return seen").unwrap(), + 11, + "both hooks run, chained, on one call" + ); + assert_eq!( + s.eval::("return f:GetAlpha()").unwrap(), + 0.5, + "and the original still did its work" + ); + // The shadow is the frame's own: a sibling of the same kind still calls the class method. + assert_eq!(s.eval::("return others").unwrap(), 0); + assert_eq!(s.eval::("return other:GetAlpha()").unwrap(), 0.5); + assert_eq!( + s.eval::("return type(rawget(other, 'SetAlpha'))") + .unwrap(), + "nil", + "hooking one frame must not write onto the shared class table" + ); + } + #[test] fn hooking_twice_chains_in_install_order_rather_than_replacing() { let s = UiScript::new().unwrap(); diff --git a/docs/ADDON_COMPAT.md b/docs/ADDON_COMPAT.md index 4ad3ea60..6597e319 100644 --- a/docs/ADDON_COMPAT.md +++ b/docs/ADDON_COMPAT.md @@ -250,7 +250,7 @@ separately, so "cannot say" has to be distinguishable from "no". | System | partial | `compat/classic_api/misc.rs`. `GetPhysicalScreenSize` answers the UI screen rect's size, with a stated deviation: modern WoW returns true *device* pixels, where this is the logical rect the host pushes through `set_screen_size` — the two differ under a non-1 UI scale or DPI factor, which this crate cannot see. `CopyToClipboard` not built: `benilla-ui` is deliberately engine-free (no windowing, no OS handles) and a clipboard is an OS capability; the workspace's `arboard` sits behind the app's dev tooling, so reaching it means a new host-callback seam through `Model`, which is plumbing rather than a binding | | Combat | partial | `compat/classic_api/misc.rs`. `InCombatLockdown()` → always `false`, and that is the honest answer rather than a stub: the lockdown exists on modern clients to refuse secure-frame edits mid-combat and 1.12 has no secure-template system at all, so nothing here is ever locked down. `StartAttack`/`StopAttack` deliberately not added here — they are 1.12's *own* verbs that benilla hasn't built yet, so registering them from `compat/` would put a stock gap in the wrong layer and hide it from the reference-surface accounting | | Class, Totem, Tracking, Instance | todo, blocked | `FillLocalizedClassList`: the nine localized class names are DBC-resolved app-side (`benilla-app`'s `ui_unit::class_names`) and `benilla-ui` deliberately holds no DBC — the player's own class reaches it through `PlayerRecord`, but not all nine, so this needs a feed like `SelfMovement`'s, not a binding. `Totem` (4 functions): no totem state exists in the model at all; 1.12's own totem bar is unbuilt, so there is nothing to wrap. `Tracking` (3): `Model::tracking` is the one *active* tracking aura (behind the stock `GetTrackingTexture`), not the enumerable list of known tracking spells these index into. `GetInstanceInfo`: `script/instance.rs` feeds only the instance *type*; the modern tuple also wants the instance name, difficulty index and name, and max players — none tracked, and the name would have to be guessed from zone text | -| Hooks (`hooksecurefunc`) | done | `compat/classic_api/hooks.rs`, both documented forms (over a global, over a table field). Pure Lua machinery, no game state. The contract it keeps, each covered by its own test: the original runs first with the caller's arguments; **the original's return values pass through untouched** and the hook's are discarded; a hook that errors has its error recorded (`Model::record_script_error`) and the original's returns still come back, while the *original's* own error still propagates; and hooking twice **chains** in install order rather than replacing. A name holding no function is a silent no-op — the reference raises, but an addon written against a client that has the verb it meant to hook is the common case here and taking its UI down over a missing one would be worse. The original and hook live in the Lua registry, since this crate holds no persistent `Function` handle | +| Hooks (`hooksecurefunc`) | done | `compat/classic_api/hooks.rs`, both documented forms (over a global, over a table field). Pure Lua machinery, no game state. The contract it keeps, each covered by its own test: the original runs first with the caller's arguments; **the original's return values pass through untouched** and the hook's are discarded; a hook that errors has its error recorded (`Model::record_script_error`) and the original's returns still come back, while the *original's* own error still propagates; and hooking twice **chains** in install order rather than replacing. A name holding no function is a silent no-op — the reference raises, but an addon written against a client that has the verb it meant to hook is the common case here and taking its UI down over a missing one would be worse. The original and hook live in the Lua registry, since this crate holds no persistent `Function` handle. **Corrected 2026-11-02, and it had been silently eating the form that matters most**: the table arm read the field with `raw_get`, reasoning that a field reached only through a metatable was not that table's to replace. A widget carries none of its own methods — they live on the shared class table its metatable's `__index` names (`object.rs`'s `frame_meta_for`) — so `hooksecurefunc(frame, "SetAlpha", cb)` found nothing and armed nothing, measured as **0 fires across 2 calls** while the plain-table and global forms fired correctly. That is not a corner: the two-argument form exists for widgets, and **29 files in grom's corpus use it**. The reference reads `table[name]` plainly and assigns back, which *shadows* the class method for that one object; ours now does the same (read through `__index`, write raw onto the host, which frames have no `__newindex` to care about). The symptom it was causing: grom's unit frame strobing whenever Cinematic was on — `Cinematic/Frames.lua:459` hooks each pfUI frame's own `SetAlpha` precisely so it can **follow** pfUI's out-of-combat fade instead of fighting it, and with the hook unarmed the two wrote a different alpha to the same frame on alternating frames. Corpus roster byte-identical with the 29 files' hooks armed | | Frame (widget methods) | partial | `compat/classic_api/frame.rs`. These are **methods, not globals**, written straight into the stock class method tables (`__benilla_frame_methods` and the two region leaves) — `object.rs`'s `frame_meta_for` *links* those tables rather than copying them ("a later write to a table shows through"), so a late install from `compat/` reaches every widget already made and every one made later. Built: `SetShown`, `GetSize`/`SetSize`, `GetRect`, `SetFormattedText` (FontString), `HookScript`, `IsEventRegistered`, `GetEffectiveAlpha`, and `SetAttribute`/`SetAttributeNoHandler`/`GetAttribute`/`ClearAttribute`. The geometry/visibility/text ones are composed from the stock verbs as a Lua chunk, so they reuse the stock resolution exactly instead of duplicating it in Rust — note the chunks return a function rather than reading `...`, since this VM is 1.12's Lua **5.0** where `...` is a parameter-list token only, never an expression. Skipped, each with the reason in that file's header: the **mask** and **atlas** families (benilla's UI quad carries one texture and a UV rect — no mask slot, no atlas registry; needs a render-path feature first), the **secure-template** family (`RegisterStateDriver`, `RegisterUnitWatch`, `SecureCmdOptionParse`, `SecureButton_*`, … — modern restricted execution, which 1.12 has no equivalent of; `SetAttribute`/`GetAttribute` ARE built because stripped of secure semantics they are just a per-frame key/value store, and that much is honest, but the drivers that *watch* attributes are the actual subsystem and are not), the **per-line** text-measurement getters (`GetNumLines`, `GetWrappedWidth`, `IsTruncated`, … — need per-line layout facts the host measurer is not asked for today; `measure.rs` answers one whole-string width/height). **`GetStringHeight` was on that skipped list and has been built**, because the blanket reason did not apply to it: the whole-string height is the other half of the number `GetStringWidth` already reads, so `measure::current_measure` now owns the staleness rule for both and the method is a read, not a new host question. AceGUI-3.0 calls it **unguarded** (`AceGUIWidget-Label.lua:54` `height = label:GetStringHeight()`; `AceGUIWidget-CheckBox.lua:90,210` `self:SetHeight(28 + self.desc:GetStringHeight())`), so a nil method there raises and takes the library's own zero-height fallback with it. **Correction, and it matters:** those are the `Libs/` (Mainline), `2.4.3/` and `3.3.5/` library trees AI_VoiceOver ships — **not** the `1.12/` tree its TOC actually loads, which reads `label:GetHeight()` instead and has had every one-argument `SetPoint` rewritten besides. This method is therefore ClassicAPI **parity surface**, not the fix for anything on this install, and the earlier claim here that it explained "can't see the VoiceOver options" was wrong. That symptom was **ours and elsewhere**: the ScrollFrame child re-anchor replaced the child's whole anchor list where 1.12's `SetPoint` merges one point into it, so AceGUI's `TOPLEFT`/`TOPRIGHT`-authored scroll child lost the half that gave it a width and resolved to no rect — and nothing below it resolved either. Fixed in `layout.rs`'s `set_frame_anchored`; measured through the same probe, which went from every node under the scroll child answering `GetLeft() == nil` to a populated, correctly placed tree. Kept because the blanket "needs per-line layout facts" reason genuinely does not apply to it and a 2.4.3/3.3.5 client of this library would need it; what was learned is that an addon shipping several library copies has to be read through its own TOC chain before any call site in it is evidence. **`SetColorTexture` has since been built too**, on the same footing: it is the modern spelling of `SetTexture`'s colour arm, which 1.12 already has (`0x770360`), so it composes in four lines of the same Lua chunk and adds no behaviour. Traced by porting the `Addon_Ports` pair — `Cinematic/Options.lua:701` paints its list border with it and died at session start without it, and OpenMapClassic its map chrome. Also in this family, each plausible and untraced this pass: `IsMouseOver`/`GetMouseFoci`/`IsDragging`/`SetResizeBounds`/`RegisterUnitEvent` (now traced: `Cinematic/Frames.lua:2611` is the next load blocker)/desaturation/rotation/the EditBox cursor-and-highlight getters. **`SetEnabled` has since been built**, on that same footing and for the same kind of reason: retail replaced 1.12's `Enable`/`Disable` pair (`0x780160`, `0x7801b0`) with the one setter, and a later-era addon writes the setter **unguarded on its own controls** even where it guards on someone else's — `Cinematic/Options.lua:4176` is a tick's own `Refresh`, greying it when a rule owns the row, and `:305` right above it is the same file guarding `control.SetEnabled and …` on a control it did not make. The raise unwound `RefreshOptions` → `SettingsChanged` → `SetProfile` → `Welcome.lua:43`, so **picking a profile from the welcome window applied the profile and then abandoned the panel refresh**, with the traceback in chat; that is what grom saw on 2026-11-02. Composed from the stock pair on Button's table, which CheckButton and LootButton inherit. EditBox takes it too, written out rather than composed because the reference gives a box no enabled state at all: it writes the mouse and keyboard the frame already owns and drops focus, so input is refused and the text is **not** greyed, which 1.12 has no slot for. A **Slider still answers neither this nor the trio** — addons duck-type on them to tell a control from a bar, which `tests/slider.rs` pins. All 12 new methods are registered in `widget_surface.rs`'s `ALLOWED_BEYOND` with their reasons | | Container (`C_Container`) | done | `compat/classic_api/container.rs`; almost entirely translation over the stock 1.12 bag verbs' own data (`Model.containers`), not new game logic. `GetContainerItemID/Info`, `HasContainerItem`, `GetContainerItemDurability`, `GetContainerNumFreeSlots`/`GetContainerFreeSlots`, `CalculateTotalNumberOfFreeBagSlots`, `PlayerHasHearthstone`/`UseHearthstone` (scans bags 0-4 for item id 6948), `SwapItems`/`MoveItem`/`AutoStoreItem` (queue the same `ContainerMove`/`BagAutoStore` the stock pickup/place path does), `SortBags`/`SortBankBags` (no-ops — 1.12 has no sort algorithm to run) and the three sort-preference toggle pairs (stored, nothing downstream reads them) are real. Deliberately nil/false, not guessed: `GetContainerItemCharges` (same charge-slot-identity blocker as SuperWoW's), `GetContainerItemRepairCost` (server-only in vanilla), `GetContainerItemQuestInfo`/`GetContainerItemEquipmentSetInfo` (no such tracking exists, and no such feature exists in 1.12 at all for the latter), `IsContainerItemOpenable` (no lockbox flag tracked) | | CreatureInfo (`C_CreatureInfo`) | partial | `compat/classic_api/creature_info.rs`, the 3 of 9 anything installed actually calls. `GetRaceInfo` and `GetFactionInfo` read a **new app→VM feed of the player's own `ChrRaces.dbc`** (`benilla-formats`' `chr_races.rs` → `benilla-app`'s `chr_races.rs` → `UiScript::set_race_info`), built because this crate holds no DBC and a frozen race table gets a modified realm's names wrong: **OctoWoW's row 10 displays "High Elf" where Blizzard's says "Blood Elf"**, while both keep `clientFileString "BloodElf"` for the art paths — the two columns genuinely disagree, so neither can be derived from the other, and a test pins that. The side on `GetFactionInfo` comes from `ui_unit.rs`'s `race_faction_group`, the same answer `UnitFactionGroup("player")` gives, so the two cannot disagree about a race; a non-playable row carries no side and reads nil rather than a guess. **`GetRaceInfo` answering nil past the last race is load-bearing** — pfUI walks `i = 1, 2, 3…` until nil, so an always-non-nil answer hangs the client at load; its own test says so. `GetCreatureID` decodes the template entry out of bits 24-47 of the 16-hex-digit GUID token the rest of `compat/` passes around, accepting **only** the creature high word so a player or pet GUID answers nil instead of a plausible-looking wrong number (the decode is inlined rather than calling `benilla_protocol::guid::entry` — this crate depends on no protocol or engine crate, and one shift is not worth breaking that boundary for). Not built: `GetCreatureInfoByID`, `RequestLoadCreatureByID`, `GetCreatureFamilyInfo`/`IDs`, `GetCreatureTypeInfo`/`IDs` (each needs a creature/family/type table this VM is not fed, and nothing installed asks) and `GetClassInfo` (the class table exists app-side as `ChrClasses` but has no VM feed yet — the same shape of work this row's race feed just did, if an addon ever wants it). With no install the race table stays empty and every reader answers nil, so an addon's race walk finds none and still loads |