-
Notifications
You must be signed in to change notification settings - Fork 565
Make Java.Lang.Object handle lookup cache less agressive #717
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.
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
Conversation
src/Mono.Android/Java.Lang/Object.cs
Outdated
| if (res != null && res.Handle != IntPtr.Zero && JNIEnv.IsSameObject (handle, res.Handle)) | ||
| if (res != null && res.Handle != IntPtr.Zero && JNIEnv.IsSameObject (handle, res.Handle)) { | ||
| if (requiredType != null && !requiredType.IsAssignableFrom (res.GetType ())) { | ||
| instances.Remove (handle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that we need the instances.Remove() here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would speed up (slightly) subsequent possible lookups though (and also I thought we'd want to update the cache with the new object at some point?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would only speed up subsequent lookups if the handle were replaced, which isn't possible to know within the context of PeekObject().
Additionally, this could very easily break things in obscure ways:
class Foo : Java.Lang.Object {}
...
var f = new Foo();
// f.Handle is registered in Object.instances
var error = JavaArray<int>.FromJniHandle (f.Handle, JniHandleOwnership.DoNotTransfer);
// f is no longer registered!If Foo overrides any Java-side virtual methods, things will blow up the next time such a method is invoked.
Instance registration is full of peril. I can't see how this instances.Remove() is a good thing.
When using Android.OS.Bundle to retrieve a collection for a specific key one can either use the untyped `Get` method or one of the generic `Get*ArrayList` methods to retrieve the list. In the first case we will get back an `IList` instance, in the second case we'll get an `IList<T>` instance. However, if one calls `Get` first and then one of the generic methods next, for the same key, the result will be an InvalidCastException thrown because we attempt to cast the IList obtained from `Get` to `IList<T>` needed by the generic `Get*ArrayList` method. This happens because the same native handle is used in both cases and the first call to `Get` causes `Java.Lang.Object` to cache the IList instance so that the second call to `Get*ArrayList` gets the cached object instead of a new, properly typed, one. The solution is to extend `Java.Lang.Object.PeekObject` to allow specifying the desired type of the object corresponding to the native handle and, if the requirement isn't met, evict the entry from the cache returning `null` to the caller, thus letting it do the right thing by creating an instance of a class with correct type. Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=58405
|
build |
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=58405 When using `Android.OS.Bundle` to retrieve a collection for a specific key one can either use the untyped `Get` method or one of the generic `Get*ArrayList` methods to retrieve the list. In the first case we will get back an `IList` instance, in the second case we'll get an `IList<T>` instance. However, if one calls `Get` first and then one of the generic methods next, for the same key, the result will be a thrown `InvalidCastException` because we attempt to cast the `IList` obtained from `Get` to `IList<T>` needed by the generic `Get*ArrayList` method. This happens because the same native handle is used in both cases and the first call to `Get` causes `Java.Lang.Object` to cache the `IList` instance so that the second call to `Get*ArrayList` gets the cached object instead of a new, properly typed, one. The solution is to extend `Java.Lang.Object.PeekObject` to allow specifying the desired type of the object corresponding to the native handle and, if the requirement isn't met, evict the entry from the cache returning `null` to the caller, thus letting it do the right thing by creating an instance of a class with correct type.
When using Android.OS.Bundle to retrieve a collection for a specific
key one can either use the untyped
Getmethod or one of thegeneric
Get*ArrayListmethods to retrieve the list. In the firstcase we will get back an
IListinstance, in the second case we'llget an
IList<T>instance. However, if one callsGetfirst and thenone of the generic methods next, for the same key, the result will be
an InvalidCastException thrown because we attempt to cast the IList
obtained from
GettoIList<T>needed by the genericGet*ArrayListmethod. This happens because the same native handle is used in both cases
and the first call to
GetcausesJava.Lang.Objectto cache the IListinstance so that the second call to
Get*ArrayListgets the cached objectinstead of a new, properly typed, one.
The solution is to extend
Java.Lang.Object.PeekObjectto allow specifyingthe desired type of the object corresponding to the native handle and, if
the requirement isn't met, evict the entry from the cache returning
nullto the caller, thus letting it do the right thing by creating an instance
of a class with correct type.
Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=58405