[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 2 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?
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