[UIKit] Protect RegisterForTraitChanges observers from premature GC toggle-ref collection - #26431
[UIKit] Protect RegisterForTraitChanges observers from premature GC toggle-ref collection#26431linnkrb wants to merge 4 commits into
Conversation
…oggle-ref collection RegisterForTraitChanges (and its NSObject-target/Selector overloads) hand a reference to the observing object to UIKitCore's private _UITraitChangeRegistry, but never call MarkDirty() to register that object with the Mono toggle-ref GC bridge. Every other API in this codebase that hands a reference to native code for later callback (UIControl.AddTarget, UIGestureRecognizer, NSNotificationCenter.AddObserver, UIBarButtonItem, UIPickerView) already does this. Without it, xamarin_gc_toggleref_callback falls back to inferring liveness from -retainCount, which only reflects normal ObjC retains. Observer registries are conventionally non-retaining (to avoid retain cycles), so _UITraitChangeRegistry holding a reference to an object doesn't show up in its retainCount. If retainCount == 1 at the next GC, the bridge downgrades the managed peer to a weak GC handle, and it can be collected while _UITraitChangeRegistry still holds a now-dangling pointer to it - corrupting a registry that is touched by many unrelated UIKit code paths afterwards. We've observed this manifest as SIGSEGV/EXC_BAD_ACCESS crashes inside _UITraitChangeRegistry across a wide variety of unrelated call sites (UICollectionView teardown, gesture node updates, ScrollEdgeEffectView, UITextField construction) on iOS 26, none of which touch RegisterForTraitChanges themselves - consistent with heap corruption surfacing far from its actual cause. This patch marks the observer (and, for the target/action overload, the target) dirty before registration, mirroring the established pattern elsewhere in this codebase. Not able to validate this compiles/passes CI locally (this checkout could not resolve the pinned internal preview SDK/runtime packages from public NuGet feeds) - opening as a draft for CI + maintainer review.
6d31460 to
ce35016
Compare
|
Update: fixed a build environment issue (a NuGet packageSourceMapping collision with a machine-level config, plus a locale-dependent code-gen bug in `bgen` itself — negative enum values render with U+2212 instead of ASCII `-` under non-English locales like nb-NO, unrelated to this PR) and got a real local build working. With that, this compiles cleanly: `src` (which builds `Microsoft.iOS.dll`, containing the changed file) built with 0 errors. In the process I also found and fixed one real bug in my own patch — the `target?.MarkDirty(false)` null-conditional in the `NSObject target`/`Selector action` overload tripped nullable-flow analysis (CS8604) since `target` is declared non-nullable there; changed to a plain `target.MarkDirty(false)`. Pushed as an amended commit. Still could not get a fully green `make all` — it now fails only in the unrelated `external/Xamarin.MacDev` devtools submodule, which needs an internal-only `Microsoft.DotNet.Arcade.Sdk` preview version not available on public feeds. That's outside the scope of this change (doesn't touch UIKit bindings), so I stopped there rather than chase an unrelated pre-existing packaging gap. |
|
@dotnet-policy-service agree |
Fresh field evidence + a narrower trigger shapePulled three crashes from a single day of our production pilot fleet (iPad12,1, iOS 26.5.2) that all trace to the same underlying corruption this PR targets. I have our own dSYM for this build, so I could symbolicate our app-level frames — that narrowed down the trigger, though it's still not a fully minimal/isolated repro (our app frames themselves aren't useful to share here, they're internal to our codebase, but the pattern they revealed is). Crash 1 — direct hit in
|
Could you create a PR with this fix? It sounds like something we should fix on our end. |
| // 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); | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
One idea could be to do something similar: create a new internal class that implements the IUITraitChangeRegistration interface:
- Keeps a strong GCHandle to the
IUITraitChangeObservableinstance. - Contains the actual
IUITraitChangeRegistrationinstance returned from the native registerForTraitChanges API - Is returned from any
RegisterForTraitChangescall. - A manual implementation of
UnregisterForTraitChangeswould 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.
2 participants
Summary
RegisterForTraitChanges(all overloads, including theNSObject target/Selectorvariant) hands a reference to the observing object to UIKitCore's private_UITraitChangeRegistry, but never callsMarkDirty()to register that object with the Mono toggle-ref GC bridge.Every other API in this codebase that hands a reference to native code for a later callback already does this:
UIControl.AddTarget,UIGestureRecognizer,NSNotificationCenter.AddObserver,UIBarButtonItem,UIPickerView.UITraitChangeObservable.csappears to have been missed — plausibly because the interface was only fully wired up via default-interface-member inlining relatively recently (#20265, following #19410).Why this matters
Without
MarkDirty(),xamarin_gc_toggleref_callback(runtime/runtime.m) falls back to inferring liveness purely from-retainCount, which only reflects normal ObjC retains. Observer registries are conventionally non-retaining (to avoid retain cycles with their observers), so_UITraitChangeRegistryholding a reference to an object doesn't show up in that object'sretainCount. IfretainCount == 1at the next GC, the bridge downgrades the managed peer to a weak GC handle, and it can be collected while_UITraitChangeRegistrystill holds a now-dangling pointer to it — corrupting a registry that many unrelated UIKit code paths touch afterwards.We've observed this manifest as
SIGSEGV/EXC_BAD_ACCESScrashes inside_UITraitChangeRegistryon iOS 26 across a wide variety of unrelated call sites over many months in production (a .NET MAUI POS app):UICollectionViewteardown, gesture-node updates,ScrollEdgeEffectView, andUITextFieldconstruction — none of which callRegisterForTraitChangesthemselves. That's consistent with heap corruption surfacing far from its actual cause rather than N independent bugs. A related, narrower regression report on this MAUI version line: #34142 (auto-closed for lack of a minimal repro, not because it was fixed).Change
Adds a
MarkDirtyForTraitRegistrationhelper and calls it at each hand-written fan-in point before theClass[]-based native registration call (for theNSObject targetoverload,targetis also marked dirty, since it's the object native code calls back into via-action:).MarkDirty(bool)is idempotent (NSObject2.cs: returns early ifIsRegisteredToggleRefis already set), so this is safe to call redundantly across the overload fan-in.Caveat — please have CI validate this
I could not build/test this locally: this checkout (tried both
mainandrelease/10.0.1xx) fails to resolve the pinned internal preview SDK/runtime packages (10.0.400-preview.0.26381.105,Microsoft.NETCore.App.Runtime.Mono.*10.0.3) from public NuGet feeds, which appears to be expected for an external contributor building outside Microsoft's CI. Opening as a draft so CI (which does have the right feed access) can validate compilation, and so a maintainer familiar with the toggle-ref bridge can sanity-check the fix location before I'd ask for a real review.I don't have a minimal, isolated repro project yet (the crash requires sustained GC pressure + trait registration under real device conditions) — happy to build one if that would help move this forward, or if a maintainer can confirm/deny the hypothesis faster with instrumentation on their end.
Test plan
RegisterForTraitChangesis the right place for this vs. a deeper fix in the trait-change registry's native binding