Repository navigation
Raise TopLevel.Closed from managed Dispose() paths - #22045
Merged
Merged
Conversation
TopLevel teardown (HandleClosed) was only ever triggered by the backend invoking ITopLevelImpl.Closed. Browser, iOS, Android, macOS and the offscreen (designer previewer) impls never raise it from Dispose(), so EmbeddableControlRoot.Closed was never raised there and StopRendering() was never reached - the top level stayed registered in MediaContext forever. Make teardown idempotent behind EnsureClosed() and call it from the managed Dispose() paths (EmbeddableControlRoot, OffscreenTopLevel, PopupRoot). The guard lives in a new non-virtual entry point because WindowBase and Window override HandleClosed and run side effects before calling base. Also drop ChoreographerTimer's view-visibility gate on Android. It predates the render timer rewrite and vetoed ticks that DefaultRenderLoop had explicitly asked for, so the synchronous compositor round-trip in HandleClosed (Renderer.Dispose -> MediaContext.SyncDisposeCompositionTarget) could never complete once the view had unsubscribed - deadlocking every activity destroy. DefaultRenderLoop already owns the sleep/wake state machine, driven by StartRendering/StopRendering, which makes the extra gate redundant. Verified at runtime on X11, Wayland, Win32, macOS, Headless, Browser (WASM), Android and iOS: Closed fires exactly once per teardown, MediaContext returns to baseline, and platform-initiated closes still fire exactly once. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You can test this PR using the following package version. |
MrJul
added
bug
backport-candidate-12.1.x
MrJul
approved these changes
Aug 24, 2026
MrJul
left a comment
MrJul
left a comment
Member
There was a problem hiding this comment.
I've left a comment about OffscreenTopLevel but it predates this PR.
LGTM!
| public void Dispose() | ||
| { | ||
| PlatformImpl?.Dispose(); | ||
| EnsureClosed(); |
Member
There was a problem hiding this comment.
OffscreenTopLevel is internal, unused and doesn't even implement IDisposable. Should we just delete this class?
MrJul
pushed a commit
to MrJul/Avalonia
that referenced
this pull request
Sep 2, 2026
TopLevel teardown (HandleClosed) was only ever triggered by the backend invoking ITopLevelImpl.Closed. Browser, iOS, Android, macOS and the offscreen (designer previewer) impls never raise it from Dispose(), so EmbeddableControlRoot.Closed was never raised there and StopRendering() was never reached - the top level stayed registered in MediaContext forever. Make teardown idempotent behind EnsureClosed() and call it from the managed Dispose() paths (EmbeddableControlRoot, OffscreenTopLevel, PopupRoot). The guard lives in a new non-virtual entry point because WindowBase and Window override HandleClosed and run side effects before calling base. Also drop ChoreographerTimer's view-visibility gate on Android. It predates the render timer rewrite and vetoed ticks that DefaultRenderLoop had explicitly asked for, so the synchronous compositor round-trip in HandleClosed (Renderer.Dispose -> MediaContext.SyncDisposeCompositionTarget) could never complete once the view had unsubscribed - deadlocking every activity destroy. DefaultRenderLoop already owns the sleep/wake state machine, driven by StartRendering/StopRendering, which makes the extra gate redundant. Verified at runtime on X11, Wayland, Win32, macOS, Headless, Browser (WASM), Android and iOS: Closed fires exactly once per teardown, MediaContext returns to baseline, and platform-initiated closes still fire exactly once. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
MrJul
added
backported-12.1.x
and removed
backport-candidate-12.1.x
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
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.
This PR addresses 2 problems:
The PR re-routes Closed to EnsureClosed that has a double-call guard (can't have it in HandleClosed since it's virtual) and calls HandleClosed from various disposable toplevels. While it would technically be more correct to fix implementations, but I'd still make multiple calls HandleClosed to be safe by having that check.