viewer: detach view from RenderManager before destroying it on dispose#170
Open
mushogenshin wants to merge 1 commit into
Open
viewer: detach view from RenderManager before destroying it on dispose#170mushogenshin wants to merge 1 commit into
mushogenshin wants to merge 1 commit into
Conversation
`ThermionViewerFFI.dispose()` destroyed the underlying Filament
view without first removing it from `FFIRenderManager`'s Dart-side
`_attachments` map. The map keys each swap-chain entry to a list
of (renderOrder, View) tuples; if a view is destroyed while still
listed, any later `attach` / `detach` / `_syncViews` call from a
*different* viewer will iterate the now-stale tuple, retrieve the
freed view's native pointer, and pass it to
`RenderManager_setRenderableRenderThread`. Filament then does a
`handle_cast` and aborts with the generic
"Postcondition: corrupted heap Handle ... tag=(no tag)".
Reproduces reliably in multi-viewer Flutter apps when one viewer
is disposed concurrently with another viewer being mounted:
- Old viewer.dispose() runs (queued onto whatever the host
serialises on, e.g. a static `_createSerial` Future chain).
- New viewer's createTextureAndBindToView calls
renderManager.attach(newView, newSwapChain), which calls
`_syncViews()` — this walks every entry in `_attachments`,
including the entry containing the just-destroyed old view.
- The freed pointer hits Filament's handle table → SIGABRT.
Symptom on Android (which is the most multi-viewer-heavy platform
in practice) is an immediate crash on toggling viewer count.
iOS / macOS happen to work because their host plugins don't
require the same reattach pattern, but the bug is real on every
platform — the symptom is just less reproducible.
Fix: call `FilamentApp.instance!.renderManager.detach(view)`
before `View_setScene(view, nullptr)` and `destroyView(view)`.
`detach(view)` (no swap chain argument) iterates `_attachments`
and removes the view from every entry, then runs `_syncViews()`
to push the cleaned state to C++ RenderManager. After that,
destroying the view is safe — no other code path will try to
dereference its handle.
This was referenced May 10, 2026
Owner
|
If there's a dangling pointer, there's probably unsynchronized access elsewhere (most likely RenderManager itself). I don't think this change is the correct solution (even if it coincidentally fixes the symptom). Let me investigate further. |
nmfisher
pushed a commit
that referenced
this pull request
May 28, 2026
…cycles (#171) * fix: serialise calls to attach / detach / detachAll on RenderManager, flush engine while destroying swapchain, skip rendering swapchains with no attached views Commits: flushAndWait before destroying so backend has drained pending endFrame Filament's `Engine::destroy(SwapChain*)` is documented as not synchronising with the GPU; explicit guidance is to call `flushAndWait()` first if a frame might still be in flight. `destroySwapChain` previously called `detachAll(swapChain)` (which takes RenderManager's mutex and removes the swap chain from C++ mViewAttachments synchronously, so no further RenderManager::render iteration touches it) and then immediately `Engine_destroySwapChainRenderThread`. That's enough on Thermion's own render thread, but it's not enough for Filament's *backend* command queue: Filament's BEGIN_FRAME / render / END_FRAME commands are processed on its backend thread independently of the caller. An endFrame queued during the last RenderManager::render before detachAll could still be sitting in the backend queue when the synchronous `engine->destroy(swapChain)` frees the SwapChain object. The backend then runs endFrame on freed memory and aborts at Renderer.cpp:490 — "SwapChain must remain valid until endFrame is called." Symptom: SIGABRT on multi-viewer dispose / mount toggles in Flutter apps. Single-viewer is unaffected because the frame scheduler doesn't have anything queued to drain at dispose time. The earlier hypothesis (Issue #167) was that the precondition itself was the bug; that turned out to be a separate list-aliasing issue. With #167 / #168 / #170 / #171 in place, this surfaced as the real underlying race that all four were downstream of. Fix: insert `await flush()` (which calls Engine_flushAndWaitRenderThread) between detachAll and Engine_destroySwapChainRenderThread. flushAndWait blocks until the backend has processed every command submitted up to this point, including any pending endFrame referencing the swap chain. Then destroy is safe. flush() is heavy but acceptable on a destroy path that already implies the surface is going away. * rendermanager: skip begin/end frame cycle when swap chain has no attached views Filament's `Renderer` class is documented as a per-window primitive (see Renderer.h: "endFrame() schedules the current frame to be displayed on the Renderer's window."), but Thermion's RenderManager shares a single Renderer across every SwapChain in the Engine, calling `beginFrame(SC_i)` / `endFrame()` for each entry in `mViewAttachments` inside one render() call. That works fine when each entry has views — Filament's internal `mSwapChain` is set, read, and reset cleanly per pair. It misbehaves when an entry has ZERO views attached: the begin/end pair fires the same state machine as a real frame but does nothing useful, opening a window for SwapChain validity assertions to trip during the dispose / mount transitions multi-viewer apps create. Empty-views entries arise easily: `setRendering(false)` and `renderManager.detach(view)` both remove the view from the entry's view list but leave the SwapChain entry in `mViewAttachments` for later reattachment. Each render iteration then ran a wasted begin/end against that empty entry, while concurrent destroy / attach traffic from the disposing or mounting viewer hit Filament's internal state. On Android and Windows this reliably fired `endFrame:490 — SwapChain must remain valid until endFrame is called.` on multi-viewer count toggles. Single-viewer never triggered it because there was never an empty entry to iterate. Fix: scan `attachment.views` before calling beginFrame and return false (skip) if all entries are null. The begin/end pair only happens when there's actual rendering work, which is what Filament's Renderer expects. Single-viewer behaviour is unchanged. Multi-viewer dispose / mount no longer races against empty begin/end cycles. * multi-viewer: serialise destroySwapChain + snapshot _syncViews iteration Two fixes for the residual `endFrame:490 — SwapChain must remain valid` precondition that fires on mass dispose / mount in multi- viewer apps (8 viewers in particular): 1) **`FFIRenderManager._syncViews` snapshots before iterating.** The old loop iterated `_attachments.keys` and `_attachments[key]` directly. Each iteration awaits a render-thread round-trip, and while awaiting, concurrent `attach` / `detach` calls (common when multiple viewers mount/dispose at once) mutate the live map. The iterator state across awaits ends up stale: some swap chains get skipped, others written with old data. The C++ RenderManager then sees inconsistent attachments and the next render tickles the SwapChain validity assertion. Snapshot the keys + per-key view list at function entry, then iterate the snapshot. Tolerates concurrent modifications: if an entry is removed by the time we get back to it, the views list is null and we skip it cleanly. 2) **`FFIFilamentApp.destroySwapChain` serialises through a static Future chain.** With 8 simultaneous viewers disposing, eight `destroySwapChain` calls run concurrently. Each calls `flush()` (Engine::flushAndWait) between detach and engine->destroy — but if other destroys are queueing more commands on Filament's backend at the same instant, no individual flush guarantees a quiet backend. The shared Renderer's `mSwapChain` state ends up pointing at a SwapChain another destroy is in the middle of freeing. Serialise: each destroy awaits the previous one before starting. The flush in each destroy now has a stable target — nothing else is queueing destroy work — so it truly drains, and the synchronous engine->destroy can proceed safely. Together with the empty-views skip in `RenderManager::renderSwap- ChainAt` (52c33f7), these fixes close the timing windows for the shared-Renderer-with-multiple-SwapChains pattern. Filament's Renderer is documented as per-window, but Thermion's design shares it; both patches make the shared use safe under contention. Tested scenario: stress-test screen toggle 1 → 4 → 8 → 1 → 4. The 4-viewer state had been working (with frame drops); 8-viewer was stable but crashed on extended use; 8 → 1 transitions crashed reliably. All three are race surfaces with shared destroy/sync flow; serialisation + snapshotting eliminate them. * rendermanager: serialise attach / detach / detachAll through a static Future chain Previous fix (d261b8f) snapshotted `_syncViews` iteration so the loop wouldn't see stale state across awaits. That kept iteration self-consistent, but didn't address concurrent calls *between* sibling viewers: in multi-viewer apps where multiple viewers mount or dispose simultaneously, sibling viewers' `attach`, `detach`, and `detachAll` calls run in parallel. Each takes its own snapshot at a different moment — and one viewer's snapshot can capture another viewer's view that's about to be destroyed via `destroyView`. By the time the snapshot's `_syncViews` reaches `RenderManager_setRenderableRenderThread`, that view's native handle has been freed; Filament dereferences the dangling pointer and the process takes a SIGSEGV. (The previous SIGABRT preconditions all triggered on detected inconsistent state at the same boundary; SIGSEGV happens when the inconsistent state slips past the precondition checks and hits real memory.) Fix: introduce a static `_opChain` Future and a `_serialize` helper that wraps each mutation of `_attachments`. attach, detach, and detachAll now each await the previous op before running — concurrent calls queue up cleanly, each runs to completion (including its `_syncViews` round-trips) before the next starts. View destruction (`destroyView` awaits `detach(view)` first) cannot interleave inside another viewer's sync. Combined with the destroySwapChain serialisation in ffi_filament_app.dart (also d261b8f), every shared-state mutation in the multi-viewer pipeline now runs serially. Tested scenario was the same one that surfaced the SIGSEGV: toggle stress-test from 8 viewers back to 1. The dispose flow fires 8 simultaneous viewer.dispose chains plus 8 simultaneous ThermionWidget.dispose async closures, each touching the shared `_attachments` and View handle state. Without serialisation the attach for the new single viewer would race against destroyView for the eight outgoing viewers; with serialisation, the ordering is enforced at the boundary that matters.
Owner
|
@mushogenshin I suspect the underlying issue was that calls to |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
`ThermionViewerFFI.dispose()` destroys the underlying Filament `View` but doesn't first remove it from `FFIRenderManager`'s Dart-side `_attachments` map:
```dart
@OverRide
Future dispose() async {
...
View_setScene(view.getNativeHandle(), nullptr);
await FilamentApp.instance!.destroyScene(scene);
await FilamentApp.instance!.destroyView(view); // ← view destroyed without detaching
...
}
```
`_attachments` keys each swap chain to a list of `(renderOrder, View)` tuples. If a view is destroyed while still listed, any later `attach` / `detach` / `_syncViews` call from a different viewer will iterate the now-stale tuple, retrieve the freed view's native pointer, and pass it to `RenderManager_setRenderableRenderThread`. Filament then does a `handle_cast` and aborts with the generic:
```
Postcondition: corrupted heap Handle ... tag=(no tag)
```
Reproduction
Multi-viewer Flutter app where one viewer is disposed concurrently with another viewer being mounted:
Symptom on Android (most multi-viewer-heavy platform in practice) is an immediate crash on toggling viewer count. iOS / macOS happen to work because their host plugins don't require the same reattach pattern, but the bug is real on every platform — the symptom is just less reproducible.
Fix
Insert `await FilamentApp.instance!.renderManager.detach(view)` before `destroyView(view)` in `ThermionViewerFFI.dispose()`. `detach(view)` (no swap chain argument) iterates `_attachments` and removes the view from every entry, then runs `_syncViews()` to push the cleaned state to C++ RenderManager. After that, destroying the view is safe — no other code path will try to dereference its handle.
Verification
Related