[release/10.0.1xx] [bgen] Preserve all public methods supporting protocol methos as events. Fixes #24236.#24576
Conversation
…ts. Fixes #24236. This has the same underlying cause as #24262, but the fix for #24262 was insufficient. The fix for #24262 ensured that if an event is listening for a protocol callback, then the corresponding protocol member implementation wouldn't be trimmed away. However, it didn't prevent protocol members from being trimmed if no event was listening for that protocol member to be called, which is a problem when the protocol member in question is a required protocol member, and native code would just call it without checking whether an implemented existed. For the bug in question, what happens is this: ```cs var mgr = new CBCentralManager (new DispatchQueue ("com.xamarin.tests.ios-plain")); // Uncomment to trigger bug, 'CBCentralManagerDelegate centralManagerDidUpdateState:' is a required protocol member // mgr.UpdatedState += (sender, e) => { // Console.WriteLine ("State: " + mgr.State); // }; mgr.DiscoveredPeripheral += (sender, e) => { }; mgr.ScanForPeripherals (); ``` In this case, there's an `ICBCentralManagerDelegate` instance assigned to `mgr.Delegate` (when an event handler was attached to the `DiscoveredPeripheral` event), but that `ICBCentralManagerDelegate` instance does not have an implementation for `ICBCentralManagerDelegate.UpdateState` (aka `CBCentralManagerDelegate centralManagerDidUpdateState:`), because it was trimmed away. This causes the bug when `CBCentralManager` calls its delegate's `centralManagerDidUpdateState:` selector. The fix is to always preserve all protocol members implemented by the internal class that implements the protocol for event handling purposes. Fixes #24236.
There was a problem hiding this comment.
Pull request overview
This PR is a backport of #24566 to the release/10.0.1xx branch, fixing a critical issue where required protocol methods were being trimmed away by the linker when no event handlers were attached to them. The issue manifested as crashes in apps using protocols like CBCentralManager when native code called required protocol methods that had been trimmed.
Changes:
- Modified the binding generator to preserve all public methods on internal delegate classes using
[DynamicDependency]attribute withDynamicallyAccessedMemberTypes.PublicMethods - Added comprehensive tests to verify that both required and optional protocol methods are preserved even when no event handlers are attached
- Removed per-method preservation in favor of class-level preservation to ensure required protocol methods are always available
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/bgen/Generator.cs | Modified to add static constructor with [DynamicDependency] attribute to preserve all public methods on internal delegate wrapper classes; removed per-method preservation approach |
| tests/test-libraries/libtest.h | Added protocol with one required method (byeByeDolphins) and two optional methods (buildIntergalacticHighway, hitchhikeWithVogons) for testing |
| tests/test-libraries/libtest.m | Implemented test methods destroyEarth and buildHighway that call protocol methods to exercise the fix |
| tests/bindings-test/ApiDefinition.cs | Added C# bindings for the new test protocol methods |
| tests/monotouch-test/ObjCRuntime/RegistrarTest.cs | Added test cases to verify required protocol methods aren't trimmed (EventTestOSType) and that optional methods work correctly (EventTestCustomType) |
| [EventArgs ("")] | ||
| [Export ("byeByeDolphins:")] | ||
| void ByeByeDolphins (Hitchhiker sender); |
There was a problem hiding this comment.
The byeByeDolphins method is marked as @required in the Objective-C protocol definition (libtest.h line 371), but it's missing the [Abstract] attribute in the C# binding. Required protocol methods should be marked with [Abstract] to match the native declaration. This is inconsistent with how other required methods are defined in the codebase, such as in ObjCProtocolBlockTest (see ApiDefinition.cs lines 327, 331, 343, 347).
✅ [CI Build #6a31009] Build passed (Build packages) ✅Pipeline on Agent |
✅ [PR Build #6a31009] Build passed (Detect API changes) ✅Pipeline on Agent |
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
✅ [CI Build #6a31009] Build passed (Build macOS tests) ✅Pipeline on Agent |
💻 [CI Build #6a31009] Tests on macOS X64 - Mac Sonoma (14) passed 💻✅ All tests on macOS X64 - Mac Sonoma (14) passed. Pipeline on Agent |
💻 [CI Build #6a31009] Tests on macOS M1 - Mac Monterey (12) passed 💻✅ All tests on macOS M1 - Mac Monterey (12) passed. Pipeline on Agent |
💻 [CI Build #6a31009] Tests on macOS M1 - Mac Ventura (13) passed 💻✅ All tests on macOS M1 - Mac Ventura (13) passed. Pipeline on Agent |
💻 [CI Build #6a31009] Tests on macOS arm64 - Mac Sequoia (15) passed 💻✅ All tests on macOS arm64 - Mac Sequoia (15) passed. Pipeline on Agent |
💻 [CI Build #6a31009] Tests on macOS arm64 - Mac Tahoe (26) passed 💻✅ All tests on macOS arm64 - Mac Tahoe (26) passed. Pipeline on Agent |
🚀 [CI Build #6a31009] Test results 🚀Test results✅ All tests passed on VSTS: test results. 🎉 All 117 tests passed 🎉 Tests counts✅ cecil: All 1 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
This has the same underlying cause as #24262, but the fix for #24262 was insufficient.
The fix for #24262 ensured that if an event is listening for a protocol callback, then the corresponding protocol member implementation wouldn't be trimmed away.
However, it didn't prevent protocol members from being trimmed if no event was listening for that protocol member to be called, which is a problem when the protocol member in question is a required protocol member, and native code would just call it without checking whether an implemented existed.
For the bug in question, what happens is this:
In this case, there's an
ICBCentralManagerDelegateinstance assigned tomgr.Delegate(when an event handler was attached to theDiscoveredPeripheralevent), but thatICBCentralManagerDelegateinstance does not have an implementation forICBCentralManagerDelegate.UpdateState(akaCBCentralManagerDelegate centralManagerDidUpdateState:), because it was trimmed away.This causes the bug when
CBCentralManagercalls its delegate'scentralManagerDidUpdateState:selector.The fix is to always preserve all protocol members implemented by the internal class that implements the protocol for event handling purposes.
Fixes #24236.
Backport of #24566.