fix(jni): clear a pending exception thrown by app-supplied router callbacks - #743
fix(jni): clear a pending exception thrown by app-supplied router callbacks#743ayaangazali wants to merge 1 commit into
Conversation
…lbacks Four JNI callbacks invoke app code and never check for a pending exception: rac_hybrid_custom_filter_jni.cpp:102 predicate.evaluate(modelId) rac_hybrid_device_state_jni.cpp:84 provider.isOnline() rac_hybrid_device_state_jni.cpp:96 provider.batteryPercent() rac_hybrid_device_state_jni.cpp:108 provider.isThermalThrottled() Both objects come from the host app: a routing predicate and a device-state provider. When one throws, Call*Method returns a default and the exception stays pending on that thread. The immediate cost is a garbage routing input, but the real cost is the next JNI call made by whoever we return into, which aborts the process with "JNI called with pending exception". The router calls these on ordinary routing decisions, so an NPE inside an app's isOnline() takes the process down somewhere unrelated. Check and clear, then return the value each function already returns when it cannot reach the JVM at all: keep the candidate, online, 100 percent battery, not throttled. That keeps a throwing provider from changing routing behaviour beyond what an unreachable JVM already does. The idiom matches okhttp_transport_adapter.cpp, which guards its own Call*Method sites the same way (and runanywhere_commons_jni.cpp has 35 such checks); these two files were the only JNI sources calling into app code with none.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughJNI callbacks now detect and clear exceptions from Kotlin or Java methods before returning. Custom filters retain candidates on predicate failure. Device-state callbacks return defined fallback values for online status, battery percentage, and thermal throttling. ChangesJNI exception handling
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change clears exceptions from app-supplied JNI callbacks and preserves existing fallback behavior; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Update on the verification caveat in the description: CI has now compiled these files for real, so the syntax-check limitation no longer applies.
|
What is wrong
Four JNI callbacks invoke app-supplied Java and never check for a pending exception:
rac_hybrid_custom_filter_jni.cpp:102predicate.evaluate(modelId)rac_hybrid_device_state_jni.cpp:84provider.isOnline()rac_hybrid_device_state_jni.cpp:96provider.batteryPercent()rac_hybrid_device_state_jni.cpp:108provider.isThermalThrottled()Both objects are registered by the host app: a routing predicate and a device-state provider. For example:
If that method throws,
CallBooleanMethodreturns a default and the exception stays pending on the thread. The garbage routing input is the small part. The real cost is that the next JNI call made by whoever we return into aborts the process withJNI called with pending exception. The hybrid router invokes these on ordinary routing decisions, so an NPE inside an app'sisOnline()brings the process down somewhere unrelated to the app code that caused it.Why these four and not others
I checked every JNI source that calls into Java. These two files were the only ones with
Call*Methodand zero exception checks:Call*MethodExceptionCheck/ExceptionOccurredokhttp_transport_adapter.cpprunanywhere_commons_jni.cpprac_cloud_stt_provider_jni.cpprac_hybrid_custom_filter_jni.cpprac_hybrid_device_state_jni.cppSo this is a gap against the convention the rest of the JNI layer already follows, not a new policy.
What this does
Checks and clears, using the same idiom as
okhttp_transport_adapter.cpp:The fallback per site is the value the function already returns when it cannot reach the JVM at all: keep the candidate, online, 100 percent battery, not throttled. So a throwing provider degrades exactly as far as an unreachable JVM already does, and no further.
Verification, and one limit I want to be straight about
These files are Android-only and are not part of the macOS commons build, so I could not run the usual
cmake/ctestgate over them. What I did run is a real type check rather than a parse, using the JDK's ownjni.h:I confirmed that check actually bites by introducing a deliberate typo, which it caught (
error: use of undeclared identifier 'JNI_FALSE_TYPO'), so rc=0 means the edits type-check and not that the compiler skipped the file. Thekotlin-androidjob is the authoritative gate here and will compile them for real.No test added: reproducing this needs a Java provider that throws, driven through a live JNI attach on a device, which the commons suite cannot host.
One judgement call worth flagging: I clear the exception without logging it, matching the existing okhttp idiom, because neither file currently includes the logger and I did not want to widen the diff into a hot routing path. If you would rather these log once before clearing, say so and I will add it.
Summary by CodeRabbit