fix(DexFactory): register injected proxy dex with a single class loader#1968
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 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
sequenceDiagram
participant Client
participant DexFactory
participant BaseDexClassLoader
participant DexClassLoader
participant DexPathList
Client->>DexFactory: request resolveClass(generatedName)
DexFactory->>BaseDexClassLoader: if injectIntoParentClassLoader true -> injectDexIntoClassLoader(jar)
alt API >= 24
DexFactory->>BaseDexClassLoader: reflectively call addDexPath(jarFilePath)
else API < 24
DexFactory->>DexPathList: build/append dexElements and splice into BaseDexClassLoader
end
DexFactory->>DexFactory: injectDexIntoClassLoader returns (true/false)
alt injection success
DexFactory->>BaseDexClassLoader: parent.loadClass(generatedName)
else injection failed
DexFactory->>DexClassLoader: create isolated DexClassLoader and load class
end
DexFactory->>Client: return Class or throw
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 |
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
edusperoni
force-pushed
the
fix/prod-inject-dex
branch
2 times, most recently
from
815dcb3 to
2f12361
Compare
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
2f12361 to
56a78de
Compare
NathanWalker
approved these changes
Jun 12, 2026
Contributor
|
I validated this on a real Pixel 6a device with Android 17 (latest beta) and it works now. Thanks! |
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
3 participants
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