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

Aberta
#2,019 0 comentários 0 reações 0 responsáveis Ver no GitHub

Ninguém assumiu esta issue ainda.

Avaliação

Dificuldade
5/5
Tempo estimado
Mais de uma semana
Facilidade para iniciantes
42/100
Tipo de issue
Bug
Clareza
Razoavelmente clara
Status de atividade
Pouca atividade
Stack de tecnologia
android, cpp, java
Domínio
mobile-dev

Direção de pesquisa

Comece por runtime-binding-generator/Dump.java, DexFactory.java e ClassStorageServiceImpl.retrieveClass e, em seguida, rastreie os caminhos de ClassResolver e JEnv mencionados no relatório. Revise extendClassNameTests.js em busca dos casos de validação comentados. O trabalho estará concluído quando a geração concorrente e o acesso do loader forem seguros, artefatos incompletos não forem persistidos e os defeitos de nomenclatura e cache listados tiverem cobertura de regressão.

Escrita pelo modelo de indexação a partir do texto da issue.

Descrição

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.

Linguagem predominante
C++
Estrelas
563
Forks
144
Merge médio
10h 46min
PRs com merge (30d)
14

Guia de contribuição

Abrir o guia de contribuição

Primeiros passos

  1. Leia a issue inteira e depois o guia de contribuição do projeto.
  2. Comente na issue dizendo que vai assumir — evita que duas pessoas façam o mesmo trabalho.
  3. Faça um fork do repositório e trabalhe em uma branch.
  4. Abra um pull request que referencie o número da issue.

Mais de NativeScript/android

Todas as issues de NativeScript/android

Issues semelhantes

Mais issues de C++

Receba novas issues na sua caixa de entrada

Um resumo curto de issues do GitHub para quem está começando.