Skip to content

Commit 2f80937

Browse files
RedthCopilot
andcommitted
Harden modal navigation seam lifecycle and threading
Second review pass found four issues in the seam's lifecycle and threading, all on paths that only open up during teardown or a handler swap. RequestSync resolved IDispatcher through _window.Handler.MauiContext. During handler teardown that context — and the IDispatcher registered in its scope — is already gone, so the lookup returned null and the documented any-thread guarantee silently degraded to inline execution at exactly the moment a backend is most likely to be finishing work off the UI thread. It now uses Window.Dispatcher, which is public, non-null and handler-independent. A marshalled RequestSync could also be overtaken by teardown: the callback ran after Window.Destroying and repopulated modal state on a torn-down window, driving presentation through disconnected resources. Requests are now tagged with a scope generation and dropped when the window has been destroyed or its handler replaced in the meantime. HandlerChanging cleared the resolution latch while _window.Handler still pointed at the OUTGOING handler, so a reentrant sync during the transition could build and latch an override from the DI scope that was going away. Resolution now stays blocked through the transition and is re-enabled from HandlerChanged, once the new handler is actually installed. A failed pop lost the caller's animation intent. The metadata was cleared before the dismissal was attempted (and, on the inline path, only recorded when the pop was deferred at all), so the retry after a failure always fell back to unanimated. It is now recorded unconditionally and cleared only once the dismissal succeeds, covering the inline and deferred failure paths. Also narrows PageAttached delivery. It previously fell back to the built-in platform whenever the override was not yet resolvable, which for a window without a service scope meant an external backend could end up with the built-in hook installed (on Tizen, the back-button handler) in addition to its own. The notification is now withheld until resolution has completed and routed to exactly one of the two, still at most once per page handler. Adds regressions for background-thread marshalling with a null handler, destroy-queued and handler-swap-queued sync being dropped, no resolution from the outgoing scope during a handler swap, and failed-pop retry preserving the animation flag. Each was verified to fail against a mutation of its fix. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent 8498a4b commit 2f80937

5 files changed

Lines changed: 394 additions & 114 deletions

File tree

src/Controls/src/Core/Platform/ModalNavigationManager/IModalNavigationHost.cs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,12 @@ public interface IModalNavigationHost
143143
/// this method returns. Do not call it while holding a lock, and finish mutating your own state
144144
/// before calling it.
145145
/// </para>
146+
/// <para>
147+
/// A marshalled request is bound to the window's current handler scope. If the window is
148+
/// destroyed, or its handler is replaced, before the request runs, the request is dropped rather
149+
/// than executed against a torn-down window or a service scope that no longer exists. Calling
150+
/// this after disposal is therefore harmless but has no effect.
151+
/// </para>
146152
/// </remarks>
147153
void RequestSync();
148154
}

src/Controls/src/Core/Platform/ModalNavigationManager/IModalNavigationPlatform.cs

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -137,10 +137,10 @@ public interface IModalNavigationPlatform : IDisposable
137137
/// <see cref="IModalNavigationHost.PlatformModalStack"/>, because a failed dismissal means the
138138
/// modal is presumed to still be on screen and dropping it would leave a visible modal that no
139139
/// stack knows about. The cross-platform modal stack is left without the page, so the next
140-
/// reconciliation pass retries the dismissal. As with
141-
/// <see cref="PushModalAsync(Page, bool)"/>, the fault is rethrown to the caller of
142-
/// <c>Navigation.PopModalAsync</c> only when the pop was applied inline; a deferred pop that
143-
/// faults is logged instead.
140+
/// reconciliation pass retries the dismissal — and <paramref name="animated"/> is preserved
141+
/// across that retry. As with <see cref="PushModalAsync(Page, bool)"/>, the fault is rethrown to
142+
/// the caller of <c>Navigation.PopModalAsync</c> only when the pop was applied inline; a
143+
/// deferred pop that faults is logged instead.
144144
/// </para>
145145
/// </remarks>
146146
Task PopModalAsync(Page modal, bool animated);
@@ -156,7 +156,11 @@ public interface IModalNavigationPlatform : IDisposable
156156
/// a handler, so a backend that resolves late does not miss the notification.
157157
/// </para>
158158
/// <para>
159-
/// The framework guarantees it is never delivered twice for the same page-handler attachment.
159+
/// The framework guarantees it is never delivered twice for the same page-handler attachment,
160+
/// and that the built-in platform is never notified for an attachment that this instance
161+
/// handles. Notification is withheld until the window has a service scope — until then the
162+
/// framework cannot tell whether a factory is registered, and notifying the built-in platform
163+
/// early would leave its hooks installed alongside this instance's.
160164
/// </para>
161165
/// </remarks>
162166
void PageAttached();

src/Controls/src/Core/Platform/ModalNavigationManager/ModalNavigationManager.cs

Lines changed: 125 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -63,40 +63,46 @@ public ModalNavigationManager(Window window)
6363
InitializePlatform();
6464

6565
_window.HandlerChanging += OnWindowHandlerChanging;
66+
_window.HandlerChanged += OnWindowHandlerChanged;
6667
_window.Destroying += (_, _) =>
6768
{
6869
ClearModalPages(platform: true);
6970

7071
// Teardown is terminal for the override: it must not come back to life on the next lazy
7172
// access while the window is being torn down. Only attaching a new handler (which brings
7273
// a new service scope, e.g. an Android activity recreation) re-enables resolution.
73-
DisposePlatformOverride(allowRecreate: false);
74+
_destroyed = true;
75+
DisposePlatformOverride();
76+
InvalidateWindowScope();
7477
};
7578
}
7679

7780
IModalNavigationPlatform? _platformOverride;
7881

79-
// Latched once resolution has been attempted so the factory is consulted exactly once per
80-
// window scope. Also used as the terminal marker after teardown: it stays true with a null
81-
// override so nothing can lazily recreate one.
82+
// Latched once resolution has been attempted, so the factory is consulted exactly once per
83+
// window scope. It is also set (with a null override) to *block* resolution while the window is
84+
// between handlers, because during that window _window.Handler still points at the outgoing
85+
// handler and resolving would latch the scope that is going away.
8286
bool _platformOverrideResolved;
8387

84-
// The page handler the current override instance has already been notified about.
88+
// Terminal after Window.Destroying. Cleared only when a new handler is installed.
89+
bool _destroyed;
90+
91+
// Bumped whenever the window's service scope changes or goes away. Work queued against an older
92+
// generation is stale and must be dropped rather than run against a disconnected scope.
93+
int _scopeGeneration;
94+
95+
// The page handler the current override (or the built-in platform) has been notified about.
8596
IElementHandler? _pageAttachedNotifiedForHandler;
8697

87-
// Delivers PageAttached at most once per (override instance, page handler) pair. Both the
88-
// creation path and PageAttachedHandler can reach this, and the reconciliation triggered by a
89-
// page change can resolve the override before PageAttachedHandler runs, so keying on the handler
90-
// instance is what keeps the notification from being doubled or lost.
91-
void NotifyPageAttached(IModalNavigationPlatform platformOverride)
98+
void InvalidateWindowScope()
9299
{
93-
var pageHandler = _window.Page?.Handler;
100+
_scopeGeneration++;
94101

95-
if (pageHandler is null || ReferenceEquals(_pageAttachedNotifiedForHandler, pageHandler))
96-
return;
97-
98-
_pageAttachedNotifiedForHandler = pageHandler;
99-
platformOverride.PageAttached();
102+
// Block resolution until a new handler is actually installed.
103+
_platformOverride = null;
104+
_platformOverrideResolved = true;
105+
_pageAttachedNotifiedForHandler = null;
100106
}
101107

102108
// Resolved lazily because a Window is constructed long before it has a handler, and therefore
@@ -106,7 +112,7 @@ void NotifyPageAttached(IModalNavigationPlatform platformOverride)
106112
{
107113
get
108114
{
109-
if (_platformOverrideResolved)
115+
if (_platformOverrideResolved || _destroyed)
110116
return _platformOverride;
111117

112118
// The window handler's IMauiContext is the per-window service scope, so the platform
@@ -138,26 +144,52 @@ void NotifyPageAttached(IModalNavigationPlatform platformOverride)
138144
}
139145

140146
// The page handler may already have been attached before the window had a service scope
141-
// to resolve from, in which case PageAttached was never delivered to this instance.
142-
if (_platformOverride is not null)
143-
NotifyPageAttached(_platformOverride);
147+
// to resolve from, in which case PageAttached was never delivered.
148+
DeliverPageAttached();
144149

145150
return _platformOverride;
146151
}
147152
}
148153

149-
void DisposePlatformOverride(bool allowRecreate)
154+
// Delivers PageAttached at most once per page handler: to the override when one is registered,
155+
// and to the built-in platform otherwise.
156+
//
157+
// The built-in notification is deliberately withheld until resolution has completed. Before the
158+
// window has a service scope we cannot yet tell whether a factory is registered, and running the
159+
// built-in hook then would leave an external backend with the built-in platform's hook installed
160+
// (on Tizen, the back-button handler) in addition to its own. Resolution re-delivers.
161+
void DeliverPageAttached()
162+
{
163+
var pageHandler = _window.Page?.Handler;
164+
165+
if (pageHandler is null || ReferenceEquals(_pageAttachedNotifiedForHandler, pageHandler))
166+
return;
167+
168+
// Resolving calls back into this method, so re-check afterwards to avoid delivering twice.
169+
var platformOverride = PlatformOverride;
170+
171+
if (ReferenceEquals(_pageAttachedNotifiedForHandler, pageHandler))
172+
return;
173+
174+
if (platformOverride is null && !_platformOverrideResolved)
175+
return;
176+
177+
_pageAttachedNotifiedForHandler = pageHandler;
178+
179+
if (platformOverride is not null)
180+
platformOverride.PageAttached();
181+
else
182+
OnPageAttachedHandler();
183+
}
184+
185+
void DisposePlatformOverride()
150186
{
151187
var platformOverride = _platformOverride;
152188
_platformOverride = null;
153189

154190
// A replacement instance has to be told about the current page handler itself.
155191
_pageAttachedNotifiedForHandler = null;
156192

157-
// When recreation isn't allowed the resolved latch stays set with a null override, which is
158-
// what makes the destroyed state terminal.
159-
_platformOverrideResolved = !allowRecreate;
160-
161193
platformOverride?.Dispose();
162194
}
163195

@@ -184,22 +216,51 @@ void DisposePlatformOverride(bool allowRecreate)
184216

185217
void IModalNavigationHost.RequestSync()
186218
{
219+
// Snapshot the scope this request belongs to. A request that is queued and then overtaken by
220+
// teardown or a handler swap must not run against the replaced or disconnected scope.
221+
var generation = _scopeGeneration;
222+
187223
// Documented as callable from any thread. The whole reconciliation entry — readiness checks,
188224
// page lifecycle events and the platform push/pop calls — must run on the UI thread, so
189225
// marshal the entire entry point when the caller isn't already there.
190-
var dispatcher = _window.Handler?.MauiContext?.Services?.GetService<IDispatcher>();
226+
var dispatcher = TryGetWindowDispatcher();
191227

192228
if (dispatcher is not null && dispatcher.IsDispatchRequired)
193229
{
194-
dispatcher.Dispatch(SyncModalStackFromPlatformRequest);
230+
dispatcher.Dispatch(() => SyncModalStackFromPlatformRequest(generation));
195231
return;
196232
}
197233

198-
SyncModalStackFromPlatformRequest();
234+
SyncModalStackFromPlatformRequest(generation);
235+
}
236+
237+
// Window.Dispatcher is public, non-null and independent of the window handler, so the any-thread
238+
// guarantee still holds during handler teardown — at which point the handler's MauiContext, and
239+
// the IDispatcher registered in its scope, are already gone.
240+
IDispatcher? TryGetWindowDispatcher()
241+
{
242+
try
243+
{
244+
return _window.Dispatcher;
245+
}
246+
catch (InvalidOperationException)
247+
{
248+
// The window was created on a thread without a dispatcher and there is no application
249+
// dispatcher to fall back to. Run inline rather than dropping the request.
250+
return null;
251+
}
199252
}
200253

201-
// Named wrapper so the fire-and-forget logging reports a stable, meaningful caller.
202-
void SyncModalStackFromPlatformRequest() => SyncModalStackWhenPlatformIsReady();
254+
void SyncModalStackFromPlatformRequest(int generation)
255+
{
256+
// Drop callbacks that were queued before the window was destroyed, or before its handler was
257+
// replaced. Running them would repopulate modal state on a torn-down window, or drive
258+
// presentation through resources belonging to a scope that no longer exists.
259+
if (_destroyed || generation != _scopeGeneration)
260+
return;
261+
262+
SyncModalStackWhenPlatformIsReady();
263+
}
203264

204265
void OnWindowHandlerChanging(object? sender, HandlerChangingEventArgs e)
205266
{
@@ -211,18 +272,29 @@ void OnWindowHandlerChanging(object? sender, HandlerChangingEventArgs e)
211272

212273
// The override was created against the old handler's service scope and most likely holds
213274
// platform views owned by it, so drop it.
214-
DisposePlatformOverride(allowRecreate: false);
275+
DisposePlatformOverride();
215276
}
216277

217-
// Attaching a new handler brings a new service scope, so a fresh override may be resolved
218-
// from it. This is the only path that lifts the terminal state left behind by teardown.
219-
if (e.NewHandler is not null)
220-
{
221-
_platformOverride = null;
222-
_platformOverrideResolved = false;
223-
}
278+
// _window.Handler is still the OUTGOING handler at this point, so resolution stays blocked
279+
// until OnWindowHandlerChanged confirms the new one is installed. Without this a reentrant
280+
// sync could latch an override built from the scope that is going away.
281+
InvalidateWindowScope();
224282
}
225283

284+
void OnWindowHandlerChanged(object? sender, EventArgs e)
285+
{
286+
_scopeGeneration++;
287+
288+
if (_window.Handler is null)
289+
return;
290+
291+
// A new handler brings a new service scope. This is the only path that lifts the blocked
292+
// state set by OnWindowHandlerChanging, and the terminal state set by teardown.
293+
_destroyed = false;
294+
_platformOverride = null;
295+
_platformOverrideResolved = false;
296+
_pageAttachedNotifiedForHandler = null;
297+
}
226298
public Task<Page?> PopModalAsync()
227299
{
228300
return PopModalAsync(true);
@@ -392,9 +464,13 @@ async Task SyncPlatformModalStackAsync()
392464
animated = pendingAnimated;
393465
}
394466

395-
_pendingPopAnimations.Remove(platformModal);
467+
// Record the intent for the duration of the attempt. It is only cleared once the
468+
// dismissal actually succeeds, so a failed pop that gets retried by a later pass still
469+
// animates the way the caller asked.
470+
_pendingPopAnimations[platformModal] = animated;
396471

397472
var page = await PopModalPlatformAsync(animated);
473+
_pendingPopAnimations.Remove(page);
398474
page.Parent?.RemoveLogicalChild(page);
399475
syncAgain = true;
400476
}
@@ -474,17 +550,19 @@ async Task SyncPlatformModalStackAsync()
474550
bool isPlatformReady = IsModalReady;
475551
bool applyNow = isPlatformReady && !syncing;
476552

477-
if (!applyNow)
478-
{
479-
// The request has already been taken off the logical stack above, so reconciliation would
480-
// have no way to recover how the caller wanted this dismissal animated. Remember it until
481-
// the platform actually dismisses the modal.
482-
_pendingPopAnimations[modal] = animated;
483-
}
553+
// The request has already been taken off the logical stack above, so reconciliation would
554+
// otherwise have no way to recover how the caller wanted this dismissal animated. Record it
555+
// unconditionally and only clear it once the platform has actually dismissed the modal, so
556+
// both a deferred pop and a retry after a failed pop keep the caller's intent.
557+
_pendingPopAnimations[modal] = animated;
484558

485559
Task popTask = applyNow ? PopModalPlatformAsync(animated) : Task.CompletedTask;
486560

487561
await popTask;
562+
563+
if (applyNow)
564+
_pendingPopAnimations.Remove(modal);
565+
488566
modal.Parent?.RemoveLogicalChild(modal);
489567
_window.OnModalPopped(modal);
490568

@@ -602,20 +680,7 @@ void OnCurrentPageHandlerChanged(object? sender, EventArgs e)
602680

603681
partial void OnPageAttachedHandler();
604682

605-
public void PageAttachedHandler()
606-
{
607-
// Resolving may create the override, and creation delivers PageAttached itself. NotifyPageAttached
608-
// is idempotent per page handler, so calling it again here is a no-op in that case.
609-
var platformOverride = PlatformOverride;
610-
611-
if (platformOverride is not null)
612-
{
613-
NotifyPageAttached(platformOverride);
614-
return;
615-
}
616-
617-
OnPageAttachedHandler();
618-
}
683+
public void PageAttachedHandler() => DeliverPageAttached();
619684

620685
void ClearModalPages(bool xplat = false, bool platform = false)
621686
{

0 commit comments

Comments
 (0)