[Bugfix] GridData::views leaks expired weak_ptrs — ~UIGridView never unregisters #362

New issue

Closed

opened 2026-07-12 22:04:36 +00:00 by john · 0 comments

john commented

2026-07-12 22:04:36 +00:00

Owner

Found by adversarial review of the #355/#359 branch (feat/355-grid-input-revival, 85e4bbe). Reported as minor; not fixed there.

Defect

GridData::views (src/GridData.h) is the N-view registry that replaced owning_view in #359. Entries are only ever removed by GridData::unregisterView, which is reached from exactly two places:

UIGridView::set_grid (reassigning a view's data), and
PyUIGridViewType.tp_dealloc, gated on obj->data.use_count() <= 1 (the #251 pattern).

~UIGridView itself does not call unregisterView. So whenever the C++ view outlives its Python wrapper — the normal case when a scene holds the last shared_ptr — the wrapper is deallocated with use_count() > 1, the guard skips the unregister, and when the view is finally destroyed by scene removal its entry stays in views forever as an expired weak_ptr.

registerView never prunes either (it only scans for duplicates).

Consequence

views grows without bound across the lifetime of a GridData. Every markDirty() / markCompositeDirty() then iterates the dead entries and lock()s each one — so a long-running scene that repeatedly creates and destroys views over one persistent GridData pays a growing per-mutation cost, and the vector never shrinks.

Not a correctness bug (expired weak_ptrs lock() to null and are skipped), which is why it was ranked minor — but it is an unbounded leak on a hot path (markDirty is called on every entity move).

Fix

Call grid_data->unregisterView(this) from ~UIGridView(), which is the only place guaranteed to run exactly once per view. Consider also pruning expired entries opportunistically in registerView.

Test

A regression test should create and destroy N views over one persistent GridData and assert the registry does not grow (needs a way to observe views.size() — either a debug accessor or an indirect assertion via markDirty cost).

Found by adversarial review of the #355/#359 branch (`feat/355-grid-input-revival`, 85e4bbe). Reported as minor; not fixed there. ## Defect `GridData::views` (`src/GridData.h`) is the N-view registry that replaced `owning_view` in #359. Entries are only ever removed by `GridData::unregisterView`, which is reached from exactly two places: - `UIGridView::set_grid` (reassigning a view's data), and - `PyUIGridViewType.tp_dealloc`, **gated on `obj->data.use_count() <= 1`** (the #251 pattern). `~UIGridView` itself does **not** call `unregisterView`. So whenever the C++ view outlives its Python wrapper — the normal case when a scene holds the last `shared_ptr` — the wrapper is deallocated with `use_count() > 1`, the guard skips the unregister, and when the view is finally destroyed by scene removal its entry stays in `views` forever as an expired `weak_ptr`. `registerView` never prunes either (it only scans for duplicates). ## Consequence `views` grows without bound across the lifetime of a `GridData`. Every `markDirty()` / `markCompositeDirty()` then iterates the dead entries and `lock()`s each one — so a long-running scene that repeatedly creates and destroys views over one persistent GridData pays a growing per-mutation cost, and the vector never shrinks. Not a correctness bug (expired `weak_ptr`s `lock()` to null and are skipped), which is why it was ranked minor — but it is an unbounded leak on a hot path (`markDirty` is called on every entity move). ## Fix Call `grid_data->unregisterView(this)` from `~UIGridView()`, which is the only place guaranteed to run exactly once per view. Consider also pruning expired entries opportunistically in `registerView`. ## Test A regression test should create and destroy N views over one persistent GridData and assert the registry does not grow (needs a way to observe `views.size()` — either a debug accessor or an indirect assertion via `markDirty` cost).