Skip to content

fix(android): resolve ArrayBuffer via app class loader on worker threads (JNI)#10

Merged
lopadova merged 3 commits into
mainfrom
fix/jni-arraybuffer-classloader
May 28, 2026
Merged

fix(android): resolve ArrayBuffer via app class loader on worker threads (JNI)#10
lopadova merged 3 commits into
mainfrom
fix/jni-arraybuffer-classloader

Conversation

@lopadova
Copy link
Copy Markdown
Contributor

Summary

  • Fixes a runtime java.lang.ClassNotFoundException: ...com.margelo.nitro.core.ArrayBuffer ... DexPathList[... /system/lib64 ...] thrown when calling any command on an Android device.
  • Root cause: the generated transport bridge resolves ArrayBuffer lazily in send() (JArrayBuffer::wrapFindClass) on a Nitro worker thread. A plain fbjni ThreadScope only attaches a JNIEnv; it does NOT install the app class loader, so FindClass hits the system loader and fails. PR fix(android): create transport HybridObject on JS thread (fix ClassNotFound) #8 only cached our transport's jclass on the JS thread — it doesn't cover NitroModules core classes resolved later on worker threads.
  • Fix: run all worker-thread C++→Kotlin JNI work under facebook::jni::ThreadScope::WithClassLoader, which attaches the thread AND installs fbjni's cached app class loader for the duration. New ECR17_RUN_ON_JVM_THREAD(body) macro wraps ensureConnected/runTransaction/runAckOnly (#ifdef __ANDROID__WithClassLoader; no-op inline on iOS), capturing the return value + exceptions (std::exception_ptr) so the money-safety try/catch logic (RetryPolicy / no blind financial retry) is unchanged.
  • Keeps the complementary PR fix(android): create transport HybridObject on JS thread (fix ClassNotFound) #8 fix (createHybridObject on the JS thread in configure()).

Touches only package/cpp/Ecr17Client/HybridEcr17Client.cpp and docs/LESSON.md.

Test plan

  • cpp-tests green (core unchanged)
  • ts-checks green (no TS touched)
  • android-build (manual dispatch) green — the ONLY verifier of the Nitro-integrated native code; cpp-tests does not compile the client
  • Device smoke: calling a command no longer throws ClassNotFoundException for ArrayBuffer

🤖 Generated with Claude Code

Calling a command threw ClassNotFoundException for
com.margelo.nitro.core.ArrayBuffer: the generated transport bridge looks
up ArrayBuffer lazily in send() (JArrayBuffer::wrap -> FindClass) on a
Nitro worker thread. A plain fbjni ThreadScope only attaches a JNIEnv; it
does NOT install the app class loader, so FindClass hit the system loader
(/system/lib64) and failed. The PR #8 fix only cached our transport's
jclass on the JS thread and doesn't cover NitroModules core classes
resolved later on worker threads.

Run all worker-thread C++->Kotlin JNI work under
facebook::jni::ThreadScope::WithClassLoader, which attaches the thread AND
installs fbjni's cached app class loader for the duration, so every
FindClass inside (incl. ArrayBuffer) resolves app classes. The new
ECR17_RUN_ON_JVM_THREAD(body) macro wraps ensureConnected/runTransaction/
runAckOnly (#ifdef __ANDROID__ -> WithClassLoader; no-op inline on iOS),
capturing the return value in an outer local and any exception via
std::exception_ptr so the money-safety try/catch semantics are unchanged.

Only reproducible by running the app; no build/CI catches it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a319953c09

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread package/cpp/Ecr17Client/HybridEcr17Client.cpp Outdated
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes an Android runtime ClassNotFoundException when Nitro worker threads perform JNI FindClass (e.g., lazy com.margelo.nitro.core.ArrayBuffer resolution) by ensuring worker-thread JNI work runs with the app class loader installed.

Changes:

  • Introduce ECR17_RUN_ON_JVM_THREAD(body) (Android: facebook::jni::ThreadScope::WithClassLoader, iOS: inline no-op) to run JNI work with the correct class loader.
  • Wrap ensureConnected(), runTransaction(), and runAckOnly() JNI-dependent code paths in the new macro while preserving existing retry / “money-safety” exception semantics.
  • Document the ArrayBuffer worker-thread class-loader failure mode and the WithClassLoader fix in docs/LESSON.md.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
package/cpp/Ecr17Client/HybridEcr17Client.cpp Adds a WithClassLoader-based macro and applies it to worker-thread JNI call sites to fix class resolution on Android.
docs/LESSON.md Adds a troubleshooting note describing the ArrayBuffer ClassNotFoundException root cause and the fix approach.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread package/cpp/Ecr17Client/HybridEcr17Client.cpp Outdated
Codex (P1) + Copilot review of PR #10:

- Codex P1: the previous ECR17_RUN_ON_JVM_THREAD *macro* expanded the body
  inline on iOS (no __ANDROID__), so the bare `return;` used to early-exit
  the Android WithClassLoader lambda was compiled inside the value-returning
  runTransaction -> invalid in a non-void function. Since Ecr17.podspec globs
  cpp/**, Apple builds would fail. Replace the macro with a template helper
  `runOnJvmThread(Fn)` that ALWAYS takes a callable: on Android it runs it via
  ThreadScope::WithClassLoader (capturing the result/exception, since
  WithClassLoader takes std::function<void()>), on iOS it just calls it.
  `return` inside the lambda now exits only the lambda on BOTH platforms, and
  value-returning callers `return runOnJvmThread(...)` directly. Verified both
  paths compile clean with g++ -std=c++20 -Wall -Wextra (with/without
  -D__ANDROID__).

- Copilot: renamed the reserved double-underscore identifier `__ecr17Err` to
  `err` (reserved identifiers are implementation-only in C++).

Money-safety try/catch semantics are unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@lopadova
Copy link
Copy Markdown
Contributor Author

Thanks for the reviews — both addressed in b532a0c:

  • Codex P1 (iOS bare return; in a non-void function): correct and important. The inline macro made the lambda's early return; compile inside the value-returning runTransaction on the iOS path (podspec globs cpp/**). Replaced the macro with a template helper runOnJvmThread(Fn) that always takes a callable — Android runs it under ThreadScope::WithClassLoader (capturing result/exception since WithClassLoader takes std::function<void()>), iOS calls it directly. return now exits only the lambda on both platforms; value-returning callers return runOnJvmThread(...). Verified both paths compile clean with g++ -std=c++20 -Wall -Wextra (with and without -D__ANDROID__).
  • Copilot (reserved __ecr17Err): renamed to err.

Money-safety try/catch semantics unchanged.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment thread docs/LESSON.md Outdated
…lassloader

# Conflicts:
#	package/cpp/Ecr17Client/HybridEcr17Client.cpp
@lopadova lopadova merged commit 64390ec into main May 28, 2026
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants