From 3536335c3a5d2f6cee194e6028a680564bba25d0 Mon Sep 17 00:00:00 2001 From: VS MobileTools Engineering Service 2 Date: Fri, 28 Oct 2022 13:53:15 -0700 Subject: [PATCH] [xcode14.1] [ObjCRuntime] Change Runtime.GetNSObject to create a new instance if the existing one is of the wrong type. Fixes #13704. (#16502) Objective-C has an optimization where creating an empty dictionary would return the same instance every time (probably to a constant empty dictionary). This causes a problem with how we've bound generic dictionaries, because all empty dictionaries would have the same native handle, even if we'd bound them with different managed types. This surfaces in unfortunate ways when we try to look up a managed instance given a native handle, we find that we already have a managed instance, but the managed instance is of the wrong type. Example code: var a = new NSDictionary (); var b = new NSDictionary (); var c = new NSDictionary (); Console.WriteLine ($"a: 0x{a.Handle:X}"); Console.WriteLine ($"b: 0x{b.Handle:X}"); Console.WriteLine ($"c: 0x{c.Handle:X}"); would produce something like: a: 0x0x7fff80821080 b: 0x0x7fff80821080 c: 0x0x7fff80821080 now for this code: Runtime.GetNSObject> (b.Handle) it would throw an exception like this: Unable to cast object of type 'Foundation.NSDictionary`2[[Foundation.NSString],[Foundation.NSObject]]' to type 'Foundation.NSDictionary`2[[Foundation.NSString],[Foundation.NSString]]' because we'll have the 'c' object (with type `NSDictionary`) in our dictionary of native handles -> managed instances, and that's not compatible with the desired return type from GetNSObject (`NSDictionary`) This likely happens with all the non-mutable collection types we have a generic version of (NSArray, NSDictionary, NSOrderedSet, NSSet). Fixes https://github.com/xamarin/xamarin-macios/issues/16378. Fixes https://github.com/xamarin/xamarin-macios/issues/13704. This PR is somewhat simpler to review by ignoring whitespace: https://github.com/xamarin/xamarin-macios/pull/16491/files?w=1 Backport of #16491 Co-authored-by: Rolf Bjarne Kvinge --- src/ObjCRuntime/Runtime.cs | 54 +++++++++---------- .../ObjCRuntime/RegistrarTest.cs | 5 +- .../monotouch-test/ObjCRuntime/RuntimeTest.cs | 17 ++++++ 3 files changed, 46 insertions(+), 30 deletions(-) diff --git a/src/ObjCRuntime/Runtime.cs b/src/ObjCRuntime/Runtime.cs index 3ffda5a757d5..d7fa4a5fc993 100644 --- a/src/ObjCRuntime/Runtime.cs +++ b/src/ObjCRuntime/Runtime.cs @@ -1437,38 +1437,36 @@ static IntPtr CreateNSObject (IntPtr type_gchandle, IntPtr handle, NSObject.Flag return null; var obj = TryGetNSObject (ptr, evenInFinalizerQueue: false); - T? o; - - if (obj is null) { - // Try to get the managed type that correspond to this exact native type - IntPtr p = Class.GetClassForObject (ptr); - // If unknown then we'll get the Class that Lookup to NSObject even if this is not NSObject. - // We can use this condition to fallback on the provided (generic argument) type - Type target_type; - if (p != NSObjectClass) { - target_type = Class.Lookup (p); - if (target_type == typeof(NSObject)) - target_type = typeof(T); - else if (typeof (T).IsGenericType) - target_type = typeof(T); - else if (target_type.IsSubclassOf (typeof(T))) { - // do nothing, this is fine. - } else if (Messaging.bool_objc_msgSend_IntPtr (ptr, Selector.GetHandle ("isKindOfClass:"), Class.GetHandle (typeof (T)))) { - // If the instance itself claims it's an instance of the provided (generic argument) type, - // then we believe the instance. See bug #20692 for a test case. - target_type = typeof(T); - } - } else { - target_type = typeof(NSObject); + + // First check if we got an object of the expected type + if (obj is T o) + return o; + + // We either didn't find an object, or it was of the wrong type, so we need to create a new instance. + + // Try to get the managed type that correspond to this exact native type + IntPtr p = Class.GetClassForObject (ptr); + // If unknown then we'll get the Class that Lookup to NSObject even if this is not NSObject. + // We can use this condition to fallback on the provided (generic argument) type + Type target_type; + if (p != NSObjectClass) { + target_type = Class.Lookup (p); + if (target_type == typeof(NSObject)) + target_type = typeof(T); + else if (typeof (T).IsGenericType) + target_type = typeof(T); + else if (target_type.IsSubclassOf (typeof(T))) { + // do nothing, this is fine. + } else if (Messaging.bool_objc_msgSend_IntPtr (ptr, Selector.GetHandle ("isKindOfClass:"), Class.GetHandle (typeof (T)))) { + // If the instance itself claims it's an instance of the provided (generic argument) type, + // then we believe the instance. See bug #20692 for a test case. + target_type = typeof(T); } - o = ConstructNSObject (ptr, target_type, MissingCtorResolution.ThrowConstructor1NotFound); } else { - o = obj as T; - if (o is null) - throw new InvalidCastException ($"Unable to cast object of type '{obj.GetType ().FullName}' to type '{typeof(T).FullName}'."); + target_type = typeof(NSObject); } - return o; + return ConstructNSObject (ptr, target_type, MissingCtorResolution.ThrowConstructor1NotFound); } static public T? GetNSObject (IntPtr ptr, bool owns) where T : NSObject diff --git a/tests/monotouch-test/ObjCRuntime/RegistrarTest.cs b/tests/monotouch-test/ObjCRuntime/RegistrarTest.cs index c35b6a0f8343..12f9bf28ebc6 100644 --- a/tests/monotouch-test/ObjCRuntime/RegistrarTest.cs +++ b/tests/monotouch-test/ObjCRuntime/RegistrarTest.cs @@ -2172,8 +2172,9 @@ public void TestCtors () // sure this is by design or by accident, but that's how we're behaving right now at least ptr = Messaging.IntPtr_objc_msgSend (Class.GetHandle (typeof (D2)), Selector.GetHandle ("alloc")); ptr = Messaging.IntPtr_objc_msgSend_long (ptr, Selector.GetHandle ("initWithBar:"), 2); - // Unable to cast object of type 'AppDelegate+D1' to type 'AppDelegate+D2' - Assert.Throws (() => Runtime.GetNSObject (ptr), "b ex"); + // Failed to marshal the Objective-C object 0x60000230a410 (type: MonoTouchFixtures_ObjCRuntime_RegistrarTest_D2). Could not find an existing managed instance for this object, nor was it possible to create a new managed instance (because the type 'MonoTouchFixtures.ObjCRuntime.RegistrarTest+D2' does not have a constructor that takes one NativeHandle argument + var ex = Assert.Throws (() => Runtime.GetNSObject (ptr), "b ex"); + Assert.That (ex.Message, Does.Contain ("Could not find an existing managed instance for this object, nor was it possible to create a new managed instance (because the type 'MonoTouchFixtures.ObjCRuntime.RegistrarTest+D2' does not have a constructor that takes one"), "Exception message"); var obj = Runtime.GetNSObject (ptr); Assert.AreEqual ("bar", obj.ctor1, "b ctor1"); } finally { diff --git a/tests/monotouch-test/ObjCRuntime/RuntimeTest.cs b/tests/monotouch-test/ObjCRuntime/RuntimeTest.cs index e534eea154bc..d0b68159fd62 100644 --- a/tests/monotouch-test/ObjCRuntime/RuntimeTest.cs +++ b/tests/monotouch-test/ObjCRuntime/RuntimeTest.cs @@ -141,6 +141,23 @@ public void GetNSObject_Posing_Class () } } + [Test] + public void GetNSObject_T_SameHandleDifferentInstances () + { + using var a = new NSDictionary (); + using var b = new NSDictionary (); + using var c = new NSDictionary (); + + if (a.Handle != b.Handle || a.Handle != c.Handle) + Assert.Ignore ("The dictionaries are actually different"); + + var handle = a.Handle; + + Assert.DoesNotThrow (() => Runtime.GetNSObject (handle), "A"); + Assert.DoesNotThrow (() => Runtime.GetNSObject> (handle), "B"); + Assert.DoesNotThrow (() => Runtime.GetNSObject> (handle), "C"); + } + [Test] public void UsableUntilDead () {