[Mono.Android] Handle JNI failures during CoreCLR GC bridge processing

[simonrozsival]

Note

This pull request was created with the assistance of GitHub Copilot. The code and description were AI-generated and reviewed by the author.

Summary

Bridge processing in the CoreCLR/NativeAOT shared native host
(src/native/clr/host/bridge-processing.cc) called several JNI functions
without checking for failure. This could leave a Java exception pending —
making the next JNI call undefined behaviour (a CheckJNI abort on debug
builds) — or silently mis-handle an out-of-memory condition as a collected
object, tearing down a peer that is still alive. This follows the pattern
already used by GCBridge::trigger_java_gc and fails fast with a clear
diagnostic.

Changes

• add_reference / clear_references — check for a pending exception
  after the CallVoidMethod invocations of monodroidAddReference /
  monodroidClearReferences. The generated monodroidAddReference lazily
  allocates and appends to an ArrayList, so it can throw
  OutOfMemoryError under the same memory pressure that triggers a GC.
  Previously the exception was left pending (undefined behaviour on the next
  JNI call) and the keep-alive edge was silently dropped, risking premature
  collection of a still-referenced peer.

• take_weak_global_ref — NewWeakGlobalRef of a valid strong global
  reference only returns null when the VM is out of memory. The old code
  stored the null and then deleted the strong reference, losing the object
  (a subsequent NewGlobalRef of a null weak reference looks like a
  collected peer). A null result now fails fast.

• take_global_ref — a null NewGlobalRef result normally means the
  weak reference's target was collected, but it can also indicate a resource
  failure (null with a pending exception). Treating the latter as "collected"
  would tear down a live peer, so the two cases are now distinguished.

These are fault paths (out-of-memory, or a peer callback throwing); normal
bridge processing is unaffected.

Testing

Built the CoreCLR android-arm64 runtime with these changes and ran the full
Mono.Android.NET-Tests suite on an API 36 arm64-v8a emulator with
-p:UseMonoRuntime=false (CoreCLR):

• 915 passed, 0 failed, 55 skipped.
• With debug.mono.log=gc, the GC bridge ran 12 times during the suite,
  exercising the hardened add_reference / clear_references /
  take_global_ref / take_weak_global_ref paths, with no CheckJNI / JNI
  errors and none of the new fail-fast paths triggered (CheckJNI is active
  for the debuggable test app).
• One unrelated timing-sensitive test (JavaObjectTest.UnregisterFromRuntime,
  a registered-peer count assertion) flaked once under heavy GC-spew logging
  and passed on a clean run; it is not affected by these changes.

The new abort paths are only reachable under out-of-memory / a throwing peer
callback and were not exercised by fault injection.

Note

This touches the same file as #12040 (GC bridge JNI-overhead cleanup); the two
changes are independent but edit adjacent code, so a trivial merge conflict is
expected depending on merge order.

[Copilot]

Pull request overview

Hardens CoreCLR/NativeAOT GC bridge processing against JNI failure modes by failing fast (with diagnostics) when peer callbacks or reference promotions leave a pending Java exception or indicate an OOM/resource failure. This prevents undefined JNI behavior and avoids incorrectly treating resource failures as “object collected,” which could tear down still-live peers.

Changes:

• Add abort_on_pending_java_exception() helper and use it after monodroidAddReference / monodroidClearReferences JNI calls.
• Distinguish “weak target collected” vs “JNI failure w/ pending exception” when promoting weak refs via NewGlobalRef.
• Fail fast when NewWeakGlobalRef returns null (OOM/resource failure) to avoid losing the object.

Show a summary per file

File	Description
src/native/clr/include/host/bridge-processing-shared.hh	Adds a shared helper declaration for detecting/logging pending Java exceptions during bridge processing.
src/native/clr/host/bridge-processing.cc	Implements fail-fast exception handling and tightens null-ref handling for global/weak-global ref transitions.

Copilot's findings

• Files reviewed: 2/2 changed files
• Comments generated: 1

[jonathanpeppers]

/review

[github-actions]

✅ Android PR Reviewer completed successfully!

[github-actions]

🤖 Code Review — ✅ LGTM

Verdict: ✅ LGTM · Issues: 0 ❌ · 0 ⚠️ · 2 💡

This is a focused, well-reasoned hardening of the CoreCLR/NativeAOT GC bridge JNI paths. Each CallVoidMethod / NewGlobalRef / NewWeakGlobalRef failure is now handled explicitly and fails fast rather than leaving a pending exception (UB on the next JNI call) or silently mishandling an OOM as a collected peer. The code follows the existing abort_on_pending_java_exception convention, the comments explain the why clearly, and the asymmetry between take_global_ref (null may be legitimate collection) and take_weak_global_ref (null always = failure) is correct and documented.

Verified

• add_reference / clear_references — exception check placed after CallVoidMethod; ExceptionDescribe/ExceptionClear before abort matches GCBridge::trigger_java_gc. ✔️
• take_global_ref — abort only on a genuine pending exception; genuine collection (null, no exception) still stores null → marked collected, and DeleteWeakGlobalRef(weak) still runs. No leak. ✔️
• take_weak_global_ref — aborts before DeleteGlobalRef(handle), so the strong reference is never lost on OOM. ✔️
• Header: abort_on_pending_java_exception declared private, <string_view> include added. ✔️
• CI: all 42 checks green on Azure DevOps dnceng-public.

Notes

• 2 inline 💡 suggestions posted (documentation of the null-handle "collected" convention; a robustness note on the double-abort). Both are optional.
• Fault paths (OOM / throwing peer callback) are, by nature, not exercisable without fault injection — the PR is transparent about this and the manual 915-test CoreCLR run with debug.mono.log=gc (12 bridge invocations, no CheckJNI errors) is reasonable validation for the non-fault path.

Nice, careful work. 👍

Generated by Android PR Reviewer for #12041 · 75.2 AIC · ⌖ 13.2 AIC · ⊞ 6.8K
_{Comment /review to run again}

Comments that could not be inline-anchored

src/native/clr/host/bridge-processing.cc:254

🤖 💡 Documentation — The new fail-fast code makes the two handle == nullptr cases explicit (genuine collection vs. resource failure), which is great. Since a reader now sees abort_on_pending_java_exception right next to context.control_block->handle = handle; where handle may legitimately be nullptr, consider a one-line comment noting that a null handle here encodes the "collected" state (what is_collected() later keys off), and that handle_type is intentionally still set to …

src/native/clr/host/bridge-processing.cc:277

🤖 💡 Native C++ — Nice defensive handling: abort_on_pending_java_exception aborts when the OOM exception is pending, and the unconditional Helpers::abort_application below covers the (spec-unlikely) case where NewWeakGlobalRef returned null without raising one. Since both branches terminate, the strong handle is never lost — correct. One small robustness thought: the unconditional abort will call abort_application with the same message even after abort_on_pending_java_exception…