fix(DexFactory): register injected proxy dex with a single class loader#1968
Open
edusperoni wants to merge 2 commits into
Open
fix(DexFactory): register injected proxy dex with a single class loader#1968edusperoni wants to merge 2 commits into
edusperoni wants to merge 2 commits into
Conversation
…er fds (#1967) * feat(inspector): serve source maps to DevTools via Network.loadNetworkResource Chrome DevTools no longer fetches external source maps itself when debugging remote targets: it issues Network.loadNetworkResource to the target and reads the result back through IO.read/IO.close. None of these embedder-side CDP domains are implemented by V8's inspector, so external source maps failed and apps had to fall back to bloated inline-source-map builds. - Handle Network.loadNetworkResource natively: resolve the URL back to a file on disk and reply with a stream handle (success:false + net::ERR_FILE_NOT_FOUND when missing). - Implement IO.read (1MB base64 chunks; eof only on a final empty read, since the frontend discards data accompanying eof) and IO.close. - Reply with a JSON-RPC error for unsupported schemes (e.g. https) so DevTools keeps its existing fallback of fetching from the host. - Rewrite sourceMapURL in outgoing Debugger.scriptParsed / Debugger.scriptFailedToParse events from relative/file:// URLs to a custom nsruntime:// scheme. DevTools hard-excludes file:, data: and devtools: URLs from loading through the target, so without the rewrite it would never send Network.loadNetworkResource and instead try (and fail) to read device files from the host machine. data: and http(s) URLs are left untouched, keeping inline source maps working. - Allow opting out via nativescript.config.ts: android.disableSourceMapURLRewrite (or the same key at the top level). - Serve these messages on the websocket read thread (new native handleMessageOnSocketThread), since the main-thread queue is unavailable exactly when DevTools needs source maps: the pause loop bypasses dispatchMessage and a busy isolate never drains the queue. The handler is V8-free and returns the response for Java to send on the receiving socket. - Make Debugger.pause interrupt busy JS via Isolate::RequestInterrupt, skipped while already paused in the nested loop to avoid a spurious re-pause after resume. - Vendor nlohmann/json v3.12.0 (third_party/json.hpp, header-only) for CDP message handling outside V8. Ports NativeScript/ios#385 and NativeScript/ios#378 to Android. Refs: nodejs/node#58077 * fix: timers removed from wrong looper * fix(timers): order timers with the Java MessageQueue instead of ALooper fds Timers were delivered through a pipe fd registered with ALooper_addFd. Android services fd callbacks inside MessageQueue.nativePollOnce at every message boundary, so timer callbacks and Handler messages lived in two queues with no mutual ordering: a setTimeout(0) could fire before or interleave around an already-queued Handler.postDelayed(0) runnable. Timers now ride the Java MessageQueue itself via a dedicated per-runtime TimerHandler bound to the isolate's Looper: - Each scheduled timer enqueues one message with sendMessageAtTime, so timers share a single queue with Handler.post/postDelayed and fire in exact MessageQueue order. - Messages are anonymous "due tokens": a native list sorted by exact (sub-millisecond) due time picks the earliest due timer per token, preserving the previous relative ordering of JS timers despite the millisecond-quantized Java queue. - Due-now timers post at (long)now so they tie (FIFO) with a postDelayed(0) made in the same millisecond; future timers post at ceil(dueTime) so they never fire early. - The background watcher thread, pipe, mutex and condition variables are removed; the MessageQueue does all delayed scheduling and everything runs on the isolate's thread with no locking. Worker isolates get the same behavior on their own loopers. Also adds ordering regression tests (timers vs Handler posts), scheduled from a java-posted runnable to avoid the >=5-level nesting clamp that jasmine's spec chaining otherwise triggers. Fixes timer/Handler ordering so setTimeout(0) reliably yields behind already-queued main-thread work.
📝 WalkthroughWalkthroughDexFactory's dex injection now returns a boolean and is SDK-gated; resolveClass uses this to fall back to an isolated DexClassLoader when injection fails. The app startup now requires a new test suite that asserts Class.forName discovery and reflective instantiation of runtime-generated classes. ChangesDex Injection Redesign and Validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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 |
Injecting a generated proxy into the app's PathClassLoader by opening it
through a temporary DexClassLoader and splicing its dex element left the
same DexFile claimed by two class loaders. ART rejects this
unconditionally ("Attempt to register dex file ... with multiple class
loaders"), but the second registration only materializes on
non-debuggable builds, so release apps crashed on the first
runtime-generated proxy while debug builds worked.
Build the dex element through the target class loader itself instead, so
the DexFile only ever has one owner: on API 24+ via
BaseDexClassLoader.addDexPath, below that via DexPathList's static
makePathElements/makeDexElements factories spliced into dexElements (the
MultiDex technique). If injection fails, fall back to the isolated
DexClassLoader path (pre-#1951 behavior) instead of failing the
subsequent loadClass with ClassNotFoundException.
Adds tests covering the original FragmentFactory scenario: proxies
generated at runtime (hidden from the static binding generator) must be
resolvable via Class.forName through the app's class loader.
Fixes #1962
Refs #1951
edusperoni
force-pushed
the
fix/prod-inject-dex
branch
from
8cfb514 to
03c811d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test-app/runtime/src/main/java/com/tns/DexFactory.java`:
- Around line 409-424: The code wrongly assumes BaseDexClassLoader.addDexPath is
accessible; since it's a hidden non-SDK API its reflective invocation can fail
on Android 28+, so change injectDexIntoClassLoader to avoid calling the hidden
method: check Build.VERSION.SDK_INT and if >= 28 immediately return false (and
log that hidden-API restrictions prevent addDexPath use), remove the
setAccessible(...) attempt and the reflective invoke path for those versions,
and ensure callers of injectDexIntoClassLoader (and any code relying on its true
return) handle the false return (or switch to a supported alternative such as
creating a DexClassLoader and using it instead); reference symbols:
injectDexIntoClassLoader, BaseDexClassLoader.addDexPath, addDexPath.invoke,
setAccessible.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 30a8f9d7-1853-40d4-94d2-61096671615a
📒 Files selected for processing (3)
test-app/app/src/main/assets/app/mainpage.jstest-app/app/src/main/assets/app/tests/testClassForNameDiscovery.jstest-app/runtime/src/main/java/com/tns/DexFactory.java
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test-app/runtime/src/main/java/com/tns/DexFactory.java (1)
415-420:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftHidden API reflection here can break the Class.forName contract on Android 9+
At Line 417,
BaseDexClassLoader.addDexPathis invoked reflectively. If hidden-API enforcement blocks it, Line 426 returnsfalseand resolution falls back to isolatedDexClassLoader, which means framework lookups through the app class loader (the behavior this PR targets) can still fail on affected devices.Is `dalvik.system.BaseDexClassLoader.addDexPath(String)` currently classified as a non-SDK API on Android API 28+ and can reflective invocation from third-party apps be blocked by hidden-API enforcement? Please cite official Android docs/AOSP sources and note behavior differences by API level.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-app/runtime/src/main/java/com/tns/DexFactory.java` around lines 415 - 420, The reflective call to BaseDexClassLoader.addDexPath (invoked on targetClassLoader) can be blocked by Android hidden-API enforcement on Android 9+; update the logic to detect and handle that failure explicitly: wrap addDexPath invocation and catch IllegalAccessException/InaccessibleObjectException/InvocationTargetException, and if reflection is blocked or fails on API >= 28, do not silently return false and fall back to an isolated DexClassLoader — instead construct or choose a class loader that preserves app-classloader delegation (e.g., PathClassLoader or a DexClassLoader that uses the app/targetClassLoader as parent) so framework Class.forName lookups continue to work; ensure this behavior is implemented where addDexPath, targetClassLoader and the DexClassLoader fallback are managed in DexFactory.java.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@test-app/runtime/src/main/java/com/tns/DexFactory.java`:
- Around line 415-420: The reflective call to BaseDexClassLoader.addDexPath
(invoked on targetClassLoader) can be blocked by Android hidden-API enforcement
on Android 9+; update the logic to detect and handle that failure explicitly:
wrap addDexPath invocation and catch
IllegalAccessException/InaccessibleObjectException/InvocationTargetException,
and if reflection is blocked or fails on API >= 28, do not silently return false
and fall back to an isolated DexClassLoader — instead construct or choose a
class loader that preserves app-classloader delegation (e.g., PathClassLoader or
a DexClassLoader that uses the app/targetClassLoader as parent) so framework
Class.forName lookups continue to work; ensure this behavior is implemented
where addDexPath, targetClassLoader and the DexClassLoader fallback are managed
in DexFactory.java.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 16be212b-5cb6-43a6-9755-1aace4f86703
📒 Files selected for processing (3)
test-app/app/src/main/assets/app/mainpage.jstest-app/app/src/main/assets/app/tests/testClassForNameDiscovery.jstest-app/runtime/src/main/java/com/tns/DexFactory.java
🚧 Files skipped from review as they are similar to previous changes (1)
- test-app/app/src/main/assets/app/mainpage.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Injecting a generated proxy into the app's PathClassLoader by opening it
through a temporary DexClassLoader and splicing its dex element left the
same DexFile claimed by two class loaders. ART rejects this
unconditionally ("Attempt to register dex file ... with multiple class
loaders"), but the second registration only materializes on
non-debuggable builds, so release apps crashed on the first
runtime-generated proxy while debug builds worked.
Build the dex element through the target class loader itself instead, so
the DexFile only ever has one owner: on API 24+ via
BaseDexClassLoader.addDexPath, below that via DexPathList's static
makePathElements/makeDexElements factories spliced into dexElements (the
MultiDex technique). If injection fails, fall back to the isolated
DexClassLoader path (pre-#1951 behavior) instead of failing the
subsequent loadClass with ClassNotFoundException.
Adds tests covering the original FragmentFactory scenario: proxies
generated at runtime (hidden from the static binding generator) must be
resolvable via Class.forName through the app's class loader.
Fixes #1962
Refs #1951
Summary by CodeRabbit
Tests
Refactor