Skip to content
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

Jump to bottom

All empty NSDictionary instances have the same underlying native handle #13704

Closed
rolfbjarne opened this issue Jan 13, 2022 · 2 comments · Fixed by #13718 or #16491
Closed

All empty NSDictionary instances have the same underlying native handle #13704

rolfbjarne opened this issue Jan 13, 2022 · 2 comments · Fixed by #13718 or #16491
Labels
bug If an issue is a bug or a pull request a bug fix
Milestone
Future

Comments

@rolfbjarne
Copy link
Member

Example code:

var a = new NSDictionary<NSString, NSObject> ();
var b = new NSDictionary<NSString, NSString> ();
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}");

prints the same handle for both instances:

a: 0x0x7fff80821080
b: 0x0x7fff80821080
c: 0x0x7fff80821080

This is unfortunately a problem.

Say you have a type with two properties:

NSDictionary<NSString, NSObject> PropertyA { get; set; }
NSDictionary<NSString, NSString> PropertyB { get; set; }

and native code has set them both to an empty NSDictionary, then fetching one will make the other break, because this happens:

Unable to cast object of type 'Foundation.NSDictionary`2[[Foundation.NSString, Xamarin.MacCatalyst, Version=0.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065],[Foundation.NSObject, Xamarin.MacCatalyst, Version=0.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065]]' to type 'Foundation.NSDictionary`2[[Foundation.NSString, Xamarin.MacCatalyst, Version=0.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065],[Foundation.NSString, Xamarin.MacCatalyst, Version=0.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065]]'

The generic versions of NSArray, NSSet and NSOrderedSet probably have the same problem.
@rolfbjarne rolfbjarne added the bug If an issue is a bug or a pull request a bug fix label Jan 13, 2022
@rolfbjarne rolfbjarne added this to the Future milestone Jan 13, 2022
@rolfbjarne rolfbjarne changed the title All empty generic NSDictionary instances have the same underlying native handle All empty NSDictionary instances have the same underlying native handle Jan 13, 2022
@Therzok
Copy link
Contributor

Therzok commented Jan 13, 2022

Thank you for explaining the source of this issue, it basically showcases #5325 (comment)

@rolfbjarne rolfbjarne mentioned this issue Jan 13, 2022
Closed
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Jan 13, 2022
@rolfbjarne
[CoreData] Move CoreData into the .NET age.
b1ed56b 
* Remove obsolete API from .NET.
* Change API marked with XAMCORE_4_0 due to naming problems to be in .NET.
* Change API marked with XAMCORE_4_0 due to using non-generic NSDictionary to be
  in XAMCORE_5_0 instead (yay!). This is because of xamarin#13704, which can make using
  generic NSDictionary in API buggy, and I feel it's a bit too risky to change this
  for .NET with the time we have available (no time to fix xamarin#13704). Additionally,
  moving this to XAMCORE_5_0 makes it possible to keep grepping for XAMCORE_4_0 to
  see what's left. Update all the CoreData API to be better as defined by our XAMCORE_4_0
  define. Mostly using generic NSSet/NSDictionary types instead of the non-generic
  ones, and other misc naming improvements.
* Change API marked with XAMCORE_4_0 due to both of the above to do both of the
  above - add a version of the naming issue fixed for .NET + a version with the generic
  dictionary for XAMCORE_5_0.
@rolfbjarne rolfbjarne mentioned this issue Jan 13, 2022
Merged
rolfbjarne added a commit that referenced this issue Jan 14, 2022
@rolfbjarne
[CoreData] Move CoreData into the .NET age. (#13718)
e963fc6 
* Remove obsolete API from .NET.
* Change API marked with XAMCORE_4_0 due to naming problems to be in .NET.
* Change API marked with XAMCORE_4_0 due to using non-generic NSDictionary to be
  in XAMCORE_5_0 instead (yay!). This is because of #13704, which can make using
  generic NSDictionary in API buggy, and I feel it's a bit too risky to change this
  for .NET with the time we have available (no time to fix #13704). Additionally,
  moving this to XAMCORE_5_0 makes it possible to keep grepping for XAMCORE_4_0 to
  see what's left. Update all the CoreData API to be better as defined by our XAMCORE_4_0
  define. Mostly using generic NSSet/NSDictionary types instead of the non-generic
  ones, and other misc naming improvements.
* Change API marked with XAMCORE_4_0 due to both of the above to do both of the
  above - add a version of the naming issue fixed for .NET + a version with the generic
  dictionary for XAMCORE_5_0.
@rolfbjarne
Copy link
Member Author

This isn't fixed

@rolfbjarne rolfbjarne reopened this Jan 18, 2022
@rolfbjarne rolfbjarne mentioned this issue Oct 27, 2022
Closed
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Oct 27, 2022
@rolfbjarne
[ObjCRuntime] Change Runtime.GetNSObject<T> to create a new instance …
39144a7 
…if the existing one is of the wrong type. Fixes xamarin#13704.

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<NSString, NSString> ();
    var c = new NSDictionary<NSString, NSObject> ();
    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<NSDictionary<NSString, NSString>> (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<NSString, Object>`)
in our dictionary of native handles -> managed instances, and that's not
compatible with the desired return type from GetNSObject
(`NSDictionary<NSString, NSString>`)

This likely happens with all the non-mutable collection types we have a
generic version of (NSArray, NSDictionary, NSOrderedSet, NSSet).

Fixes xamarin#16378.
Fixes xamarin#13704.
@rolfbjarne rolfbjarne mentioned this issue Oct 27, 2022
Merged
mattleibow added a commit to dotnet/maui that referenced this issue Oct 27, 2022
@mattleibow
Dispose empty sets and try/catch exceptions
a920fb5 
Workaround for xamarin/xamarin-macios#13704
@mattleibow mattleibow mentioned this issue Oct 27, 2022
Merged
rmarinho pushed a commit to dotnet/maui that referenced this issue Oct 27, 2022
@mattleibow
Dispose empty sets and try/catch exceptions (#10955)
dbfaf45 
Workaround for xamarin/xamarin-macios#13704
rmarinho pushed a commit to dotnet/maui that referenced this issue Oct 27, 2022
@mattleibow @rmarinho
Dispose empty sets and try/catch exceptions (#10955)
20a28a4 
Workaround for xamarin/xamarin-macios#13704
rmarinho pushed a commit to dotnet/maui that referenced this issue Oct 27, 2022
@mattleibow @rmarinho
Dispose empty sets and try/catch exceptions (#10955)
10a8bba 
Workaround for xamarin/xamarin-macios#13704
rmarinho pushed a commit to dotnet/maui that referenced this issue Oct 27, 2022
@mattleibow @rmarinho
Dispose empty sets and try/catch exceptions (#10955)
c94e11e 
Workaround for xamarin/xamarin-macios#13704
rmarinho pushed a commit to dotnet/maui that referenced this issue Oct 27, 2022
@mattleibow @rmarinho
Dispose empty sets and try/catch exceptions (#10955)
4a0227e 
Workaround for xamarin/xamarin-macios#13704
Redth added a commit to dotnet/maui that referenced this issue Oct 27, 2022
@rmarinho @dotnet-maestro
@mattleibow @Redth
Cherrypick fron net7 to release branch (#10957)
a99302b 
* Squashed commit of the following: (#10942)

commit 22b3033
Merge: 50866a7 95ee1a3
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Thu Oct 27 11:10:47 2022 +0200

    Merge branch 'net7.0' into darc-net7.0-45fa6a48-68f2-4992-95f2-e40e8e14afd5

commit 50866a7
Author: Jonathan Dick <jondick@gmail.com>
Date:   Wed Oct 26 21:22:02 2022 -0400

    Bump net6 version for SR7

commit 8edadd3
Author: Rui Marinho <me@ruimarinho.net>
Date:   Wed Oct 26 18:55:17 2022 +0100

    Update NuGet.config

commit b7c5411
Author: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Date:   Wed Oct 26 17:48:49 2022 +0000

    Update dependencies from https://github.com/xamarin/xamarin-macios build 20221026.24

    Microsoft.tvOS.Sdk
     From Version 16.0.1475 -> To Version 16.0.1477

commit 4c57036
Author: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Date:   Wed Oct 26 17:48:09 2022 +0000

    Update dependencies from https://github.com/xamarin/xamarin-macios build 20221026.24

    Microsoft.iOS.Sdk
     From Version 16.0.1475 -> To Version 16.0.1477

commit 89f722e
Author: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Date:   Wed Oct 26 16:51:31 2022 +0000

    Update dependencies from https://github.com/xamarin/xamarin-macios build 20221026.21

    Microsoft.tvOS.Sdk
     From Version 16.0.1475 -> To Version 16.0.1476

commit 9dc4a41
Author: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Date:   Wed Oct 26 16:51:01 2022 +0000

    Update dependencies from https://github.com/xamarin/xamarin-macios build 20221026.21

    Microsoft.iOS.Sdk
     From Version 16.0.1475 -> To Version 16.0.1476

commit dde2135
Author: Rui Marinho <me@ruimarinho.net>
Date:   Wed Oct 26 17:46:35 2022 +0100

    Add Nuget.config feed

commit bad3509
Author: GitHub Actions Autoformatter <autoformat@example.com>
Date:   Wed Oct 26 16:41:26 2022 +0000

    Auto-format source code

commit 6d56d6c
Author: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Date:   Wed Oct 26 16:40:27 2022 +0000

    Update dependencies from https://github.com/xamarin/xamarin-macios build 20221026.22

    Microsoft.MacCatalyst.Sdk
     From Version 15.4.2370 -> To Version 15.4.2371

commit 2da2d4d
Author: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Date:   Wed Oct 26 16:40:10 2022 +0000

    Update dependencies from https://github.com/xamarin/xamarin-macios build 20221026.22

    Microsoft.macOS.Sdk
     From Version 12.3.2370 -> To Version 12.3.2371

Co-authored-by: Matthew Leibowitz <mattleibow@live.com>
# Conflicts:
#	eng/Version.Details.xml
#	eng/Versions.props

* Dispose empty sets and try/catch exceptions (#10955)

Workaround for xamarin/xamarin-macios#13704

* Update MAUI's net6 version

Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Matthew Leibowitz <mattleibow@live.com>
Co-authored-by: Jonathan Dick <jondick@gmail.com>
vs-mobiletools-engineering-service2 pushed a commit to vs-mobiletools-engineering-service2/xamarin-macios that referenced this issue Oct 28, 2022
@rolfbjarne
[ObjCRuntime] Change Runtime.GetNSObject<T> to create a new instance …
224f518 
…if the existing one is of the wrong type. Fixes xamarin#13704.

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<NSString, NSString> ();
    var c = new NSDictionary<NSString, NSObject> ();
    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<NSDictionary<NSString, NSString>> (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<NSString, Object>`)
in our dictionary of native handles -> managed instances, and that's not
compatible with the desired return type from GetNSObject
(`NSDictionary<NSString, NSString>`)

This likely happens with all the non-mutable collection types we have a
generic version of (NSArray, NSDictionary, NSOrderedSet, NSSet).

Fixes xamarin#16378.
Fixes xamarin#13704.
Merged
dalexsoto pushed a commit that referenced this issue Oct 28, 2022
@vs-mobiletools-engineering-service2 @rolfbjarne
[xcode14.1] [ObjCRuntime] Change Runtime.GetNSObject<T> to create a n…
3536335 
…ew 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<NSString, NSString> ();
    var c = new NSDictionary<NSString, NSObject> ();
    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<NSDictionary<NSString, NSString>> (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<NSString,
Object>`)
in our dictionary of native handles -> managed instances, and that's not
compatible with the desired return type from GetNSObject
(`NSDictionary<NSString, NSString>`)

This likely happens with all the non-mutable collection types we have a
generic version of (NSArray, NSDictionary, NSOrderedSet, NSSet).

Fixes #16378.
Fixes #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 <rolf@xamarin.com>
@ghost ghost locked as resolved and limited conversation to collaborators Nov 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
bug If an issue is a bug or a pull request a bug fix
Projects
None yet
Milestone
Future
2 participants
@Therzok @rolfbjarne