Skip to content

[UIKit] Protect RegisterForTraitChanges observers from premature GC toggle-ref collection #26431

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
linnkrb wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

[UIKit] Protect RegisterForTraitChanges observers from premature GC toggle-ref collection #26431

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ce35016
[UIKit] Protect RegisterForTraitChanges observers from premature GC t…
Aug 11, 2026
176f8db
Merge branch 'main' into fix/trait-change-observable-toggleref
linnkrb Aug 12, 2026
32124a7
Merge branch 'main' into fix/trait-change-observable-toggleref
linnkrb Aug 27, 2026
917c28e
Merge branch 'main' into fix/trait-change-observable-toggleref
linnkrb Aug 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions src/UIKit/UITraitChangeObservable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,26 @@ public static Class [] ToClasses (params Type [] traits)
return Class.FromTypes (traits);
}

// Register the observing object with the toggle-ref GC bridge before we hand a
// reference to it to native code. Without this, the managed peer's toggle-ref status
// defaults to whatever xamarin_gc_toggleref_callback infers from -retainCount, which
// only reflects normal ObjC retains: it can't see that _UITraitChangeRegistry now also
// holds a reference to this object internally (observer registries are conventionally
// non-retaining, to avoid retain cycles with their observers). If -retainCount is 1 at
// the next GC, the bridge downgrades the peer to a weak GC handle and it can be
// collected while _UITraitChangeRegistry still references it, corrupting the shared
// registry (a crash then tends to surface later, in unrelated code that next touches
// the registry, rather than here). MarkDirty is idempotent and mirrors the pattern
// already used by UIControl.AddTarget, UIGestureRecognizer, and
// NSNotificationCenter.AddObserver for the same reason.
private static void MarkDirtyForTraitRegistration (IUITraitChangeObservable observable)
{
// NSObject.MarkDirty() is 'protected'; the (bool) overload is 'internal' and can be
// called from anywhere in this assembly, which is what we need from a static method
// on an unrelated interface.
(observable as NSObject)?.MarkDirty (false);
}
Comment on lines +29 to +47

@rolfbjarne rolfbjarne Aug 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasoning here is incorrect: MarkDirty is used when a managed peer contains managed state, and mustn't be collected by the GC before the native object. It does not change the lifetime of the native object, only the managed object.

The correct fix is to make sure the IUITraitChangeObservable instance isn't collected by the GC before calling UnregisterForTraitChanges on it. However, if you're calling UnregisterForTraitChanges, you must keep the instance around somewhere, which would prevent the GC from collecting it, so I'm guessing you're not calling UnregisterForTraitChanges?

@linnkrb linnkrb Aug 27, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — that makes sense, and it points at the actual bug better than my patch does.

To answer directly: I checked, and at least the one RegisterForTraitChanges call site I could find in dotnet/maui (SwitchHandler.iOS.cs, SwitchProxy) does call UnregisterForTraitChanges — in Disconnect(platformView), which runs from DisconnectHandler. So it's not simply "no one calls Unregister."

The gap is that DisconnectHandler isn't guaranteed to run before the platform view's managed peer is collected. We've hit this repeatedly in our own MAUI app (iOS): under GC pressure, a handler's platform view can be finalized without an explicit DisconnectHandler() call ever firing — we've had to build a whole set of patterns around it (window-null teardown guards, IDestructible.Destroy() hooks, GC.SuppressFinalize pinning on ~26 custom handlers) specifically because relying on Disconnect* running reliably isn't safe. If that's what's happening to UISwitch/SwitchProxy here too, UnregisterForTraitChanges silently never runs, the closure-capturing SwitchProxy gets collected while _UITraitChangeRegistry still holds a now-dangling pointer to it, and the registry corruption surfaces later at unrelated call sites — which matches what we're seeing (UICollectionView teardown, gesture-node updates, ScrollEdgeEffectView, UITextField construction, none of which touch RegisterForTraitChanges themselves).

Given that, I think the fix needs to live in RegisterForTraitChanges/UnregisterForTraitChanges itself rather than at each call site: take a strong GCHandle on the observable when it registers, keyed by the returned IUITraitChangeRegistration, and free it when UnregisterForTraitChanges is called. That guarantees the object can't be collected while it's live in the registry — and if a caller's Disconnect/Unregister path never runs (as above), the failure mode becomes a leak instead of a dangling pointer, which is a much safer place to be while any missing-unregister call sites get found and fixed properly.

Happy to move the PR in that direction if that sounds right to you — want me to take a pass at it there instead of at the RegisterForTraitChanges call sites?

@rolfbjarne rolfbjarne Aug 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that makes sense.

There's already a precedent for something similar, NSObject.AddObserver returns an object that must be disposed to stop observing, and if that object isn't disposed manually, then a warning is printed (because presumably the GC collected the object because the developer didn't keep a reference to it):

macios/src/Foundation/NSObject2.cs

Lines 1324 to 1366 in 10b38c6

[Register ("__XamarinObjectObserver")]
class Observer : NSObject {
WeakReference? obj;
Action<NSObservedChange>? cback;
NSString key;
public Observer (NSObject obj, NSString key, Action<NSObservedChange> observer)
{
if (observer is null)
throw new ArgumentNullException (nameof (observer));
this.obj = new WeakReference (obj);
this.key = key;
this.cback = observer;
IsDirectBinding = false;
}
[Preserve (Conditional = true)]
public override void ObserveValue (NSString keyPath, NSObject ofObject, NSDictionary change, IntPtr context)
{
if (keyPath == key && context == Handle)
cback!.Invoke (new NSObservedChange (change));
else
base.ObserveValue (keyPath, ofObject, change, context);
}
/// <inheritdoc />
protected override void Dispose (bool disposing)
{
if (disposing) {
if (obj is not null) {
var target = (NSObject?) obj.Target;
if (target is not null)
target.RemoveObserver (this, key, Handle);
}
obj = null;
cback = null;
} else {
Runtime.NSLog ("Warning: observer object was not disposed manually with Dispose()");
}
base.Dispose (disposing);
}
}

One idea could be to do something similar: create a new internal class that implements the IUITraitChangeRegistration interface:

  • Keeps a strong GCHandle to the IUITraitChangeObservable instance.
  • Contains the actual IUITraitChangeRegistration instance returned from the native registerForTraitChanges API
  • Is returned from any RegisterForTraitChanges call.
  • A manual implementation of UnregisterForTraitChanges would be needed, and handle getting passed the new internal class correctly (free the GCHandle, etc.)

I'll have a look at doing this, it's not trivial.


/// <summary>
/// Registers a callback handler that will be executed when one of the specified traits changes.
/// </summary>
Expand All @@ -39,6 +59,7 @@ public IUITraitChangeRegistration RegisterForTraitChanges (Type [] traits, Actio

internal static IUITraitChangeRegistration _RegisterForTraitChanges (IUITraitChangeObservable This, Type [] traits, Action<IUITraitEnvironment, UITraitCollection> handler)
{
MarkDirtyForTraitRegistration (This);
return _RegisterForTraitChanges (This, ToClasses (traits), handler);
}

Expand All @@ -56,6 +77,7 @@ public IUITraitChangeRegistration RegisterForTraitChanges (Action<IUITraitEnviro
internal static IUITraitChangeRegistration _RegisterForTraitChanges (IUITraitChangeObservable This, Action<IUITraitEnvironment, UITraitCollection> handler, params Type [] traits)
{
// Add an override with 'params', unfortunately this means reordering the parameters.
MarkDirtyForTraitRegistration (This);
return _RegisterForTraitChanges (This, ToClasses (traits), handler);
}

Expand All @@ -74,6 +96,7 @@ public IUITraitChangeRegistration RegisterForTraitChanges<T> (Action<IUITraitEnv
internal static IUITraitChangeRegistration _RegisterForTraitChanges<T> (IUITraitChangeObservable This, Action<IUITraitEnvironment, UITraitCollection> handler)
where T : IUITraitDefinition
{
MarkDirtyForTraitRegistration (This);
return _RegisterForTraitChanges (This, ToClasses (typeof (T)), handler);
}

Expand All @@ -95,6 +118,7 @@ internal static IUITraitChangeRegistration _RegisterForTraitChanges<T1, T2> (IUI
where T1 : IUITraitDefinition
where T2 : IUITraitDefinition
{
MarkDirtyForTraitRegistration (This);
return _RegisterForTraitChanges (This, ToClasses (typeof (T1), typeof (T2)), handler);
}

Expand All @@ -119,6 +143,7 @@ internal static IUITraitChangeRegistration _RegisterForTraitChanges<T1, T2, T3>
where T2 : IUITraitDefinition
where T3 : IUITraitDefinition
{
MarkDirtyForTraitRegistration (This);
return _RegisterForTraitChanges (This, ToClasses (typeof (T1), typeof (T2), typeof (T3)), handler);
}

Expand Down Expand Up @@ -146,6 +171,7 @@ internal static IUITraitChangeRegistration _RegisterForTraitChanges<T1, T2, T3,
where T3 : IUITraitDefinition
where T4 : IUITraitDefinition
{
MarkDirtyForTraitRegistration (This);
return _RegisterForTraitChanges (This, ToClasses (typeof (T1), typeof (T2), typeof (T3), typeof (T4)), handler);
}

Expand All @@ -163,6 +189,10 @@ public IUITraitChangeRegistration RegisterForTraitChanges (Type [] traits, NSObj

internal static IUITraitChangeRegistration _RegisterForTraitChanges (IUITraitChangeObservable This, Type [] traits, NSObject target, Selector action)
{
MarkDirtyForTraitRegistration (This);
// 'target' receives the callback via -action:, so it needs the same protection as
// 'This' even though it isn't the object being observed.
target.MarkDirty (false);
return _RegisterForTraitChanges (This, ToClasses (traits), target, action);
}

Expand All @@ -179,6 +209,7 @@ public IUITraitChangeRegistration RegisterForTraitChanges (Type [] traits, Selec

internal static IUITraitChangeRegistration _RegisterForTraitChanges (IUITraitChangeObservable This, Type [] traits, Selector action)
{
MarkDirtyForTraitRegistration (This);
return _RegisterForTraitChanges (This, ToClasses (traits), action);
}

Expand Down