[Java.Runtime.Environment] don't call PeekPeer() inside AddPeer()

[jonathanpeppers]

Context: dotnet/android#10004

This breaks the "replaceable" logic otherwise.

[Copilot]

Pull Request Overview

This PR removes the call to PeekPeer inside AddPeer to fix issues with the "replaceable" logic in the Java runtime environment.

• Removed PeekPeer check from MonoRuntimeValueManager.cs
• Removed PeekPeer check from ManagedValueManager.cs

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/Java.Runtime.Environment/Java.Interop/MonoRuntimeValueManager.cs	Removed PeekPeer call from AddPeer to prevent premature return when a duplicate peer is encountered.
src/Java.Runtime.Environment/Java.Interop/ManagedValueManager.cs	Removed PeekPeer check from AddPeer, aligning with the updated logic to support replaceable peers.

[jonathanpeppers]

/azp run

[azure-pipelines]

No pipelines are associated with this pull request.

[jonpryor]

This could use a test. dotnet/android#10004 notes behavior around "replaceable" values. What somewhat confuses me about this is #1323 also attempted to test replaceable values, and did not need any changes within dotnet/java-interop to pass. Meanwhile, the tests failed under dotnet/android#9921, for reasons I haven't gotten a chance to investigate.

Is the test in #1323 incorrect or insufficient? Are we hitting a different corner case around "replaceable" values?

The explanation in this PR also leaves me wanting:

This breaks the "replaceable" logic otherwise.

How does this break "replaceable" logic?

Especially considering this existing test, which attempts to assert that duplicate AddPeer() calls is idempotent:

java-interop/tests/Java.Interop-Tests/Java.Interop/JniRuntimeJniValueManagerContract.cs

Lines 67 to 77 in 7b7ae83

	[Test]
	public void AddPeer_NoDuplicates ()
	{
	int startPeerCount = GetSurfacedPeersCount ();
	using (var v = new MyDisposableObject ()) {
	// MyDisposableObject ctor implicitly calls AddPeer();
	Assert.AreEqual (startPeerCount + 1, GetSurfacedPeersCount (), DumpPeers ());
	valueManager.AddPeer (v);
	Assert.AreEqual (startPeerCount + 1, GetSurfacedPeersCount (), DumpPeers ());
	}
	}

The existing test passes, but why? I wonder if we're now getting a WarnNotReplacing()?

java-interop/src/Java.Runtime.Environment/Java.Interop/ManagedValueManager.cs

Line 86 in 7b7ae83

WarnNotReplacing (key, value, p);

[jonathanpeppers]

I do need to revisit what's happening this repo (java.interop) & the test above, but on the dotnet/android side:

1. Object A is replaceable, it will be returned if asked for by PeekPeer()
2. Object B comes around and needs to replace A
3. PeekPeer() returns A and so none of the "replacing" logic runs at all, it returns early here:

java-interop/src/Java.Runtime.Environment/Java.Interop/ManagedValueManager.cs

Lines 60 to 62 in 7b7ae83

	var o = PeekPeer (value.PeerReference);
	if (o != null)
	return;

So, I think this one we could merge, as a test caught it and is fixed by the change:

• [Mono.Android] fix "replaceable" objects in ManagedValueManager android#10004

Further investigation & test changes needed on this PR.

[jonpryor]

I believe that this has been superseded by #1323, which also removes the PeekPeer() invocation from ManagedValueManager.cs, and adds unit tests (and more!).