Drop global Instance of JavaMarshalValueManager#11524
Conversation
simonrozsival
added
Area: CoreCLR
There was a problem hiding this comment.
Pull request overview
This PR removes the managed JavaMarshalValueManager.Instance singleton lookup and passes the active value-manager handle through the CoreCLR GC bridge so bridge completion processing can operate on the correct instance.
Changes:
- Threads a
JavaMarshalValueManagerGC handle through managed/native GC bridge initialization and completion callbacks. - Converts bridge completion context processing from static singleton lookup to instance-based processing.
- Updates NativeAOT/CoreCLR value-manager creation paths to instantiate
JavaMarshalValueManagerdirectly.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/native/clr/include/runtime-base/internal-pinvokes.hh |
Updates native P/Invoke declaration to accept the value-manager handle. |
src/native/clr/include/host/gc-bridge.hh |
Stores the value-manager handle and updates the finished callback signature. |
src/native/clr/host/internal-pinvokes-shared.cc |
Forwards the value-manager handle into GC bridge initialization. |
src/native/clr/host/gc-bridge.cc |
Passes the stored handle when invoking bridge completion. |
src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalValueManager.cs |
Removes singleton access and resolves bridge completion through a GC handle. |
src/Mono.Android/Android.Runtime/RuntimeNativeMethods.cs |
Updates managed native method and callback signatures. |
src/Mono.Android/Android.Runtime/JNIEnvInit.cs |
Splits CoreCLR and NativeAOT value-manager creation branches. |
src/Microsoft.Android.Runtime.NativeAOT/Java.Interop/JreRuntime.cs |
Replaces singleton fallback with direct JavaMarshalValueManager construction. |
|
/review |
|
✅ Android PR Reviewer completed successfully! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ LGTM (with minor suggestions)
Good cleanup — removing the static singleton and threading the instance handle through the native GC bridge callback is a solid improvement for testability and clarity.
Summary of findings:
⚠️ (1) TheGCHandlecreated in the constructor is never stored/freed — likely fine for a process-lifetime singleton but worth making explicit- 💡 (2) Minor formatting/organization nits
The native ↔ managed boundary changes are consistent across all files (C#, C++, headers, pinvoke declarations). The callback signature updates match correctly.
Generated by Android PR Reviewer for issue #11524 · ● 8.1M
|
|
||
| ReadOnlySpan<GCHandle> handlesToFree = instance.ProcessCollectedContexts (mcr); | ||
|
|
||
|
|
There was a problem hiding this comment.
🤖 💡 Formatting — Extra blank line here (two consecutive blank lines).
Rule: Minimal diffs
| if (RuntimeFeature.IsCoreClrRuntime) { | ||
| return new JavaMarshalValueManager (); | ||
| } | ||
|
|
||
| if (RuntimeFeature.IsNativeAotRuntime) { | ||
| return new JavaMarshalValueManager (); |
There was a problem hiding this comment.
🤖 💡 Code organization — These two branches have identical bodies. Unless there's a planned divergence (different JavaMarshalValueManager configuration per runtime), this could stay as the original if (RuntimeFeature.IsCoreClrRuntime || RuntimeFeature.IsNativeAotRuntime). If there is a plan to diverge, a comment explaining why they're split would help future readers.
Rule: Remove unused code / YAGNI
|
|
||
| // There can only be one instance because JavaMarshal.Initialize can only be called once. | ||
| var mark_cross_references_ftn = RuntimeNativeMethods.clr_initialize_gc_bridge (&BridgeProcessingStarted, &BridgeProcessingFinished); | ||
| var javaMarshalValueManagerHandle = new GCHandle<JavaMarshalValueManager> (this); |
There was a problem hiding this comment.
🤖 GCHandle<JavaMarshalValueManager> is created here but never stored as a field, so it can never be freed in Dispose(). This keeps the managed object pinned for the entire process lifetime.
Since there's only ever one JavaMarshalValueManager and it lives for the app's lifetime, this is probably acceptable — but it should be explicit. Consider storing it as a field and freeing it in Dispose(bool), or at minimum adding a comment explaining why the handle is intentionally leaked (e.g., "the native side holds this handle for the process lifetime; freeing it in Dispose would race with GC bridge callbacks").
Rule: Resource management (Postmortem #11)
There was a problem hiding this comment.
This is the same as before, we could do this in Dispose(), but that will never get called anyway.
jonathanpeppers
deleted the
dev/simonrozsival/java-marshal-value-manager-cleanup
branch
Summary