Skip to content

Commit fcfa22b

Browse files
committed
[Java.Runtime.Environment] Permit identityHashCode collisions
Fixes: #5 java.lang.System.identityHashCode() returns a 32-bit integer. MonoRuntimeValueManager was using this as value as the key to map JNI handles to managed peer instances. What happens if two different Java instances have the same identityHashCode() value? [0] :-) The answer is "you get the wrong managed peer", and things promptly go weird. (Is there a way to test this? Not easily.) Change MonoRuntimeValueManager so that instead of using a: Dictionary<int, WeakReference<IJavaPeerable>> which can support only one managed peer per identityHashCode() value, instead use a: Dictionary<int, List<WeakReference<IJavaPeerable>>> This allows each identityHashCode() "key" to refer to *multiple* managed peers, allowing things to stay (reasonably) sane. [0]: You may ask "How likely is that?", but (1) with 64-bit processes, *all bets are off* for *32-bit* values, and (2) this can happen on Android [1, 2]. [1]: https://bugzilla.xamarin.com/show_bug.cgi?id=27408 [2]: https://github.com/xamarin/monodroid/commit/ea65174bcdb4cf5d32ef5f566275877333f4e09c
1 parent cb3f2d4 commit fcfa22b

File tree

1 file changed

+93
-36
lines changed

1 file changed

+93
-36
lines changed

src/Java.Runtime.Environment/Java.Interop/MonoRuntimeValueManager.cs

Lines changed: 93 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -73,24 +73,30 @@ protected override void Dispose (bool disposing)
7373
if (RegisteredInstances == null)
7474
return;
7575

76-
foreach (var o in RegisteredInstances.Values) {
77-
IJavaPeerable t;
78-
if (!o.TryGetTarget (out t))
79-
continue;
80-
t.Dispose ();
76+
lock (RegisteredInstances) {
77+
foreach (var o in RegisteredInstances.Values) {
78+
foreach (var r in o) {
79+
IJavaPeerable t;
80+
if (!r.TryGetTarget (out t))
81+
continue;
82+
t.Dispose ();
83+
}
84+
}
85+
RegisteredInstances.Clear ();
8186
}
82-
RegisteredInstances.Clear ();
8387
}
8488

85-
Dictionary<int, WeakReference<IJavaPeerable>> RegisteredInstances = new Dictionary<int, WeakReference<IJavaPeerable>>();
89+
Dictionary<int, List<WeakReference<IJavaPeerable>>> RegisteredInstances = new Dictionary<int, List<WeakReference<IJavaPeerable>>>();
8690

8791

8892
public override List<JniSurfacedPeerInfo> GetSurfacedPeers ()
8993
{
9094
lock (RegisteredInstances) {
9195
var peers = new List<JniSurfacedPeerInfo> (RegisteredInstances.Count);
9296
foreach (var e in RegisteredInstances) {
93-
peers.Add (new JniSurfacedPeerInfo (e.Key, e.Value));
97+
foreach (var p in e.Value) {
98+
peers.Add (new JniSurfacedPeerInfo (e.Key, p));
99+
}
94100
}
95101
return peers;
96102
}
@@ -111,26 +117,52 @@ public override void Add (IJavaPeerable value)
111117
}
112118
int key = value.JniIdentityHashCode;
113119
lock (RegisteredInstances) {
114-
WeakReference<IJavaPeerable> existing;
115-
IJavaPeerable target;
116-
if (RegisteredInstances.TryGetValue (key, out existing) && existing.TryGetTarget (out target) && !Replaceable (target))
117-
Runtime.ObjectReferenceManager.WriteGlobalReferenceLine (
118-
"Warning: Not registering PeerReference={0} IdentityHashCode=0x{1} Instance={2} Instance.Type={3} Java.Type={4}; " +
119-
"keeping previously registered PeerReference={5} Instance={6} Instance.Type={7} Java.Type={8}.",
120-
value.PeerReference.ToString (),
121-
key.ToString ("x"),
122-
RuntimeHelpers.GetHashCode (value).ToString ("x"),
123-
value.GetType ().FullName,
124-
JniEnvironment.Types.GetJniTypeNameFromInstance (value.PeerReference),
125-
target.PeerReference.ToString (),
126-
RuntimeHelpers.GetHashCode (target).ToString ("x"),
127-
target.GetType ().FullName,
128-
JniEnvironment.Types.GetJniTypeNameFromInstance (target.PeerReference));
129-
else
130-
RegisteredInstances [key] = new WeakReference<IJavaPeerable> (value, trackResurrection: true);
120+
List<WeakReference<IJavaPeerable>> peers;
121+
if (!RegisteredInstances.TryGetValue (key, out peers)) {
122+
peers = new List<WeakReference<IJavaPeerable>> () {
123+
new WeakReference<IJavaPeerable>(value, trackResurrection: true),
124+
};
125+
RegisteredInstances.Add (key, peers);
126+
return;
127+
}
128+
129+
for (int i = peers.Count - 1; i >= 0; i--) {
130+
var wp = peers [i];
131+
IJavaPeerable p;
132+
if (!wp.TryGetTarget (out p)) {
133+
// Peer was collected
134+
peers.RemoveAt (i);
135+
continue;
136+
}
137+
if (!JniEnvironment.Types.IsSameObject (p.PeerReference, value.PeerReference))
138+
continue;
139+
if (Replaceable (p)) {
140+
peers [i] = new WeakReference<IJavaPeerable>(value, trackResurrection: true);
141+
} else {
142+
WarnNotReplacing (key, value, p);
143+
}
144+
return;
145+
}
146+
peers.Add (new WeakReference<IJavaPeerable> (value, trackResurrection: true));
131147
}
132148
}
133149

150+
void WarnNotReplacing (int key, IJavaPeerable ignoreValue, IJavaPeerable keepValue)
151+
{
152+
Runtime.ObjectReferenceManager.WriteGlobalReferenceLine (
153+
"Warning: Not registering PeerReference={0} IdentityHashCode=0x{1} Instance={2} Instance.Type={3} Java.Type={4}; " +
154+
"keeping previously registered PeerReference={5} Instance={6} Instance.Type={7} Java.Type={8}.",
155+
ignoreValue.PeerReference.ToString (),
156+
key.ToString ("x"),
157+
RuntimeHelpers.GetHashCode (ignoreValue).ToString ("x"),
158+
ignoreValue.GetType ().FullName,
159+
JniEnvironment.Types.GetJniTypeNameFromInstance (ignoreValue.PeerReference),
160+
keepValue.PeerReference.ToString (),
161+
RuntimeHelpers.GetHashCode (keepValue).ToString ("x"),
162+
keepValue.GetType ().FullName,
163+
JniEnvironment.Types.GetJniTypeNameFromInstance (keepValue.PeerReference));
164+
}
165+
134166
static bool Replaceable (IJavaPeerable peer)
135167
{
136168
if (peer == null)
@@ -140,13 +172,28 @@ static bool Replaceable (IJavaPeerable peer)
140172

141173
public override void Remove (IJavaPeerable value)
142174
{
175+
if (value == null)
176+
throw new ArgumentNullException (nameof (value));
177+
143178
int key = value.JniIdentityHashCode;
144179
lock (RegisteredInstances) {
145-
WeakReference<IJavaPeerable> wv;
146-
IJavaPeerable t;
147-
if (RegisteredInstances.TryGetValue (key, out wv) &&
148-
wv.TryGetTarget (out t) &&
149-
object.ReferenceEquals (value, t))
180+
List<WeakReference<IJavaPeerable>> peers;
181+
if (!RegisteredInstances.TryGetValue (key, out peers))
182+
return;
183+
184+
for (int i = peers.Count - 1; i >= 0; i--) {
185+
var wp = peers [i];
186+
IJavaPeerable p;
187+
if (!wp.TryGetTarget (out p)) {
188+
// Peer was collected
189+
peers.RemoveAt (i);
190+
continue;
191+
}
192+
if (object.ReferenceEquals (value, p)) {
193+
peers.RemoveAt (i);
194+
}
195+
}
196+
if (peers.Count == 0)
150197
RegisteredInstances.Remove (key);
151198
}
152199
}
@@ -159,13 +206,23 @@ public override IJavaPeerable PeekPeer (JniObjectReference reference)
159206
int key = GetJniIdentityHashCode (reference);
160207

161208
lock (RegisteredInstances) {
162-
WeakReference<IJavaPeerable> wv;
163-
if (RegisteredInstances.TryGetValue (key, out wv)) {
164-
IJavaPeerable target;
165-
if (wv.TryGetTarget (out target))
166-
return target;
209+
List<WeakReference<IJavaPeerable>> peers;
210+
if (!RegisteredInstances.TryGetValue (key, out peers))
211+
return null;
212+
213+
for (int i = peers.Count - 1; i >= 0; i--) {
214+
var wp = peers [i];
215+
IJavaPeerable p;
216+
if (!wp.TryGetTarget (out p)) {
217+
// Peer was collected
218+
peers.RemoveAt (i);
219+
continue;
220+
}
221+
if (JniEnvironment.Types.IsSameObject (reference, p.PeerReference))
222+
return p;
167223
}
168-
RegisteredInstances.Remove (key);
224+
if (peers.Count == 0)
225+
RegisteredInstances.Remove (key);
169226
}
170227
return null;
171228
}

0 commit comments

Comments
 (0)