Uh oh!
There was an error while loading. Please reload this page.
[typemap] Fix trimmable peer creation race - #12650
Conversation
Defer implicit peer registration until replaceable state is assigned, preserve DoNotRegister through generated Java.Interop activation, and return the canonical registered peer from racing lookup and array-conversion paths. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It changes core peer-registration/canonicalization behavior across runtime + generator code paths, which is correctness-critical and warrants final human validation.
Review tier: Lite
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
 | tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/TrimmableTypeMapRuntimeCoverageTests.cs — 💡 suggestion Concurrency — ActivationBarrier is written on one thread and read on other… |
 | src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/TypeMapAssemblyEmitter.cs — 💡 suggestion Maintainability — EmitJniObjectReferenceOptions() currently hardcodes the… |
What changed in this PR
Fixes a CoreCLR trimmable typemap peer-registry race where concurrent peer creation could allow a rejected/unregistered managed wrapper to escape, by ensuring DoNotRegister is propagated through activation and by canonicalizing return values to the registered “winner” where appropriate.
Changes:
- Adjust trimmable typemap activation to defer peer registration until after
Replaceable/Activatablestate is assigned, and optionally return the already-registered compatible peer. - Propagate
JniHandleOwnership.DoNotRegisterinto generated Java.Interop-style activation IL (and extend IL maxstack tracking for new opcodes). - Add regression tests for stale lookup and concurrent
GetPeer/GetObjectArrayscenarios plus an emitted-IL assertion.
| File | Description |
|---|---|
| tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/TrimmableTypeMapRuntimeCoverageTests.cs | Extends runtime coverage assertions and adds synchronization hooks to validate activation/registration behavior. |
| tests/Mono.Android-Tests/Mono.Android-Tests/Android.Runtime/JnienvArrayMarshaling.cs | Adds deterministic regression tests for stale and concurrent object-array conversions under trimmable typemap. |
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/TypeMapAssemblyGeneratorTests.cs | Verifies generated activation IL propagates DoNotRegister into JniObjectReferenceOptions. |
| src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMap.cs | Routes activation through a unified registration path and introduces DoNotRegister implicit-ownership for activation. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/TypeMapAssemblyEmitter.cs | Updates emitted Java.Interop-style activation IL to incorporate DoNotRegister. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/PEAssemblyBuilder.cs | Extends maxstack tracking to support additional IL opcodes used by the new emission sequence. |
| external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs | Canonicalizes GetPeer() results to the registered peer when a concurrent registration wins. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Name the generated ownership bit constants and make the activation barrier reference volatile for explicit cross-thread visibility. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
simonrozsival
left a comment
There was a problem hiding this comment.
⚠️ Needs Changes
Correctness-focused review. The root-cause analysis holds up: I traced the pre-fix flow and confirmed the implicit peer registered from ConstructPeerCorebeforeMarkCreatedPeer set Replaceable, so AddPeer's target.Replaceable && !value.Replaceable branch let the intermediary evict an already-registered peer — exactly the identity divergence in #10973. DoNotRegister + register-after-marking is the right fix, and the emitted bit math checks out: DoNotRegister (0x10) >> 2 lands exactly on the DoNotRegisterTarget bit (1 << 2), so Copy | … yields CopyAndDoNotRegister (5).
Issues: 1 error · 2 warnings · 1 suggestion
The one that matters: the canonicalization lives in the shared JniRuntime.JniValueManager.GetPeer, not in the trimmable value manager. GetPeer's only production caller is Java.Lang.Object.GetObject(), which is on the hot path for every runtime — so Mono, llvm-ir CoreCLR, and NativeAOT all pick up a new AddPeer call plus a dispose-and-substitute path for a bug none of them have. None of the added regression tests cover those runtimes, and the PR notes Mono couldn't be validated locally. Putting the canonicalization behind a virtual creation hook that only TrimmableTypeMapValueManager overrides keeps base-class behavior identical.
CI:MAUI Tests / MAUI Integration failed with MAUI R2R Helix submission failed for CoreCLR/android-arm64/Full R2R with exit code 1 — a Helix submission failure, so almost certainly infrastructure rather than these changes, but it needs to be green before this leaves draft. Most other legs are still pending.
Nice work: the three regression tests are genuinely deterministic rather than timing-dependent — putting the Barrier inside the activation constructor forces both callers past the registry miss before either can register, so the race can't be scheduled away. And asserting that the losing wrapper is disposed (not merely dropped) is the right check, since abandoning it would leak a global reference.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Revert the JniRuntime.JniValueManager.GetPeer() canonicalization added in the previous commit and drop the peer-substitution logic from TrimmableTypeMap.RegisterCreatedPeer(). GetPeer() is the base implementation shared by every JniValueManager, and its only production caller is Java.Lang.Object.GetObject(), which is on the hot path for Mono, llvm-ir CoreCLR and NativeAOT as well. Canonicalizing there changed peer creation for three runtimes that never had this bug, and none of the added regression tests cover them. Substituting the registered peer was also incompatible with the documented CreatePeer() contract: JniRuntimeJniValueManagerContract.CreatePeer_Replaceable- DoesNotReplace requires CreatePeer() to return a *new* peer even when a compatible peer is already registered. Neither was needed. The bug is that the implicit peer registered itself from ConstructPeerCore() before MarkCreatedPeer() applied Replaceable, and AddPeer() lets a non-replaceable peer evict a replaceable one — so each new intermediary evicted the peer an earlier caller was holding. Activating with DoNotRegister and registering after marking is sufficient: the loser of a race now keeps its own peer as an unregistered alias instead of corrupting the registry. Also address review feedback: make the test peer's ConstructorInvocations counter atomic (ActivationBarrier guarantees concurrent constructor entry) and document the cross-assembly enum coupling behind the emitted `Copy | ((ownership & DoNotRegister) >> 2)`. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The expression `Copy | ((ownership & DoNotRegister) >> 2)` reads as arbitrary bit twiddling. Spell out what it computes and why: which bit is being carried between the two enums and why they are two positions apart, why the mask drops the transfer bits, why Copy is always set, and why arithmetic is preferred over a branch in emitted IL. Comment-only; the emitted IL is unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The generated Java.Interop-style activation stub converted its JniHandleOwnership argument into JniObjectReferenceOptions by emitting `Copy | ((ownership & DoNotRegister) >> 2)` inline. That is unreadable at the call site, and it only worked because JniHandleOwnership.DoNotRegister (0x10) and the DoNotRegisterTarget bit of JniObjectReferenceOptions (1 << 2) happen to sit two bit positions apart in two unrelated enums. Move the mapping into JNIEnv.ToJniObjectReferenceOptions(), an aggressively inlineable internal helper, where it is a plain conditional between two named enum members and no longer depends on that accidental alignment. The generated stub now just loads `ownership` and calls it. The generator test previously pinned the emitted byte sequence, which asserted the shape rather than the meaning; it now resolves the call target from metadata and asserts the stub calls JNIEnv.ToJniObjectReferenceOptions with `ownership`. Also drop the And/Or/Shr_un stack-tracking cases added for the old sequence, which no longer has any consumer. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The summaries and inline comments for the Java.Interop-style CreateInstance emitters still described the options argument as `Copy | ((ownership & DoNotRegister) >> 2)`, which is no longer what is emitted. Point them at JNIEnv.ToJniObjectReferenceOptions(ownership) instead. Comment-only. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The Mac build failed with CS8602 in Generate_JiStyleCtor_PropagatesDoNotRegister: GetILBytes() returns byte[]?, and the hand-rolled IL scan dereferenced it via il.Length. It only slipped through locally because CI treats warnings as errors; the previous revision used AsSpan(), which tolerates null. Rewrite the assertion to follow the MemberRef-table idiom already used by Generate_JiStyleCtor_EmitsDeleteRefCall: resolve JNIEnv.ToJniObjectReferenceOptions from metadata, then look for `ldarg.2; call <that token>`. That removes the null dereference, the unguarded il[i - 1] read, and the possibility of a stray 0x28 byte inside an operand being decoded as a bogus member-reference token. Java.Lang.Object.FromJniHandleOwnership already performed exactly this JniHandleOwnership -> JniObjectReferenceOptions conversion, so the new helper had duplicated it. Drop it and route SetHandle through JNIEnv.ToJniObjectReferenceOptions; both produce Copy (1) or CopyAndDoNotRegister (5), so behavior is unchanged. Also restore a blank line removed by accident in EmitMemberReferences. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
simonrozsival
commented
Sep 3, 2026
/review |
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
⚠️ Needs Changes
No blocking code defect found; the ownership propagation and delayed peer registration are consistent with the registry semantics. I left one 💡 testing suggestion to make the concurrent regression reproduce the intended eviction ordering deterministically.
Findings: 0 errors · 0 warnings · 1 suggestion
The latest Azure DevOps build (1580767) is still in progress with no failures observed so far, so this cannot be marked LGTM until CI completes.
Generated by Android PR Reviewer for #12650 · gpt56 · 471.6 AIC · ⌖ 19 AIC · ⊞ 25.7K
Comment /review to run again
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Context
Fixes#10973.
GetObjectArraywas exposing a peer-registry bug rather than an array-conversion bug: a caller could cache one managed wrapper while the registry retained a different wrapper for the same Java object.Root cause
Trimmable type-map proxies registered their implicit peer from the activation constructor (
ConstructPeerCore) beforeTrimmableTypeMapmarked itReplaceable.AddPeer()deliberately lets a non-replaceable peer evict an existing replaceable one, so every newly created intermediary evicted the peer an earlier caller was already holding — leaving that caller with a wrapper the runtime no longer knew about.Java.Interop-style generated proxies also hardcoded
JniObjectReferenceOptions.Copy, discardingJniHandleOwnership.DoNotRegister, so they always self-registered.Fix
DoNotTransfer | DoNotRegister, and register them only afterMarkCreatedPeer()has appliedReplaceable/Activatable.DoNotRegisterthrough generated Java.Interop-style activation IL.The ownership → options mapping lives in
JNIEnv.ToJniObjectReferenceOptions(), an aggressively inlineable helper, so it reads as a conditional between two named enum members rather than bit arithmetic emitted inline. The generated stub simply loadsownershipand calls it.Deliberately unchanged
JniRuntime.JniValueManageris untouched. An earlier revision canonicalized peers inside the sharedGetPeer(), but that base implementation is used by every value manager — and its only production caller,Java.Lang.Object.GetObject(), is on the hot path for Mono, llvm-ir CoreCLR, and NativeAOT too. That changed peer creation for three runtimes which never had this bug.Substituting the registered peer also conflicts with the documented contract:
JniRuntimeJniValueManagerContract.CreatePeer_ReplaceableDoesNotReplacerequiresCreatePeer()to return a new peer even when a compatible one is already registered.Neither was necessary. A caller that loses a creation race simply keeps its own peer as an unregistered alias; the fix only stops that peer from evicting the registered one.
Validation
Verified the regression tests genuinely fail without the fix: reverting only
ImplicitPeerOwnershiptoDoNotTransferfails all three (AfterStalePeerLookup,AfterConcurrentPeerLookup,DuringConcurrentPeerLookup); restoring it passes all three.GetPeer()andGetObjectArray()against a fresh Java object each iteration, asserting on every iteration that all 20 workers raced through activation, that exactly one of their peers is the registered peer, and thatGetObjectArray()agrees with the registry