NativeScript / NativeScript/android

Proxy dex generation: thread-safety and latent naming bugs (follow-up to #2016)

Open
#2,019 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C++
Stars
563
Forks
144
Avg merge
10h 46m
Merged PRs (30d)
14

Description

Follow-up to #2016, which makes runtime proxy dex generation a routine path (dev servers keeping @nativescript/core off disk) instead of a rare one. None of the items below block that PR — they are pre-existing defects in the generation path whose exposure it raises, plus small latent bugs found while reviewing it. Intended to be picked up after the ESM/loader work lands.

Concurrency (the substantive part)

Proxy generation has no synchronization, but it is reachable from every runtime's thread (extend works on workers; each Runtime has its own DexFactory, but they share one dexDir and the static state below):

  • Silent dex corruption: Dump.methodDescriptorBuilder is a static final StringBuffer used as setLength(0) → append → toString() (runtime-binding-generator, Dump.java:27). Two concurrent generations interleave and bake wrong method descriptors into a dex — no error, just a wrong class. Dump.interfaceImplementedInterfaces[1] = classSignature (Dump.java:810,820) is the same hazard on a static array.
  • EACCES on the loser thread: jarFile.exists() / setReadOnly() is a check-then-act pair (DexFactory.java jar assembly), and setReadOnly() runs on every resolve including cache hits, widening the window. Two threads resolving the same class can leave one opening a 0444 file for write.
  • Truncated jar persisted read-only: fi.read(dexData, 0, dexData.length) is a single unchecked read (DexFactory.java, jar assembly). A short read — e.g. racing a concurrent write of the same dex — zero-pads the jar, which is then made read-only and reused on subsequent launches within the install.
  • ConcurrentModificationException window: ClassStorageServiceImpl.retrieveClass iterates the loaders collection (an unmodifiableCollection over a synchronizedSet) without holding its lock while storeClassaddClassLoader mutates it. Every runtime-generated proxy adds a loader, so #2016 directly raises the hit rate (and makes the miss path O(loaders)).

Suggested shape: make Dump's scratch state instance-local (it already is instantiated per ProxyGenerator); loop the read or use Files.readAllBytes; write the jar to a temp name and atomically rename; synchronize the loaders iteration on the underlying set.

Latent bugs / nits

  • dexFile.getPath().replace(".dex", ".jar") replaces all occurrences, not the suffix — a package segment containing .dex (e.g. com.example.dexter… does not, but ….dext shapes can) mangles both names identically, so it works until two distinct classes mangle to the same jar. Use a suffix strip.
  • $_ normalization is applied to className but never to baseClassName, so Interface.extend({...}) on a nested interface computes classNameToLoad = com.tns.gen.…$… while the generator emits …_…ClassNotFoundException. Pre-existing; sits on the exact line #2016 guards.
  • The two prefix predicates disagree: ClassResolver tests startsWith("com.tns.gen"), DexFactory tests "com.tns.gen." (trailing dot). A name like com.tns.generated.Foo is a binding class to one and a named proxy to the other.
  • com.tns.tests.* is excluded from isBindingClass, so a missing test class now falls through to runtime generation instead of throwing — an unintended widening from #2016's fallthrough.
  • There is no name validation at all for dotted extend names (ValidateExtendArguments is skipped on the hasDot branch), and the extend-name validation specs in extendClassNameTests.js are commented out. A named proxy colliding with a derived anonymous name fails with a bare CNFE.
  • JEnv::InsertClassIntoCache caches nullptr on a failed resolve, and the cache read treats that as a miss forever — a name that fails once and succeeds later re-crosses JNI on every lookup (perf only).

Explicitly not included

A "migration sweep" for legacy un-thumbed cache files was considered and rejected: dexDir lives under the app's code_cache, which the platform wipes on every app upgrade — the same event that changes the thumb — so pre-#2016 files cannot survive into a post-#2016 install. The only residue is the rare fallback dir (files/secondary-dexes, used when code_cache is unusable), which is not platform-wiped; not worth machinery.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with runtime-binding-generator/Dump.java, DexFactory.java, and ClassStorageServiceImpl.retrieveClass, then trace the ClassResolver and JEnv paths named in the report. Review extendClassNameTests.js for the commented validation cases. Done means concurrent generation and loader access are safe, incomplete artifacts are not persisted, and the listed naming and cache defects have regression coverage.

Written by the indexing model from the issue text.

Assessment

Tech stack
android, cpp, java
Domain
mobile-dev
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.