Device runtime: run pushed Codename One apps on a phone - #5561
Device runtime: run pushed Codename One apps on a phone#5561shai-almog wants to merge 130 commits into
Conversation
ddc43de to
7623680
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ddc43de0d2
ℹ️ 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".
Codename One apps run two ways today: the JavaSE simulator, which is not a device, or a cloud device build, which costs minutes per iteration. This adds a third: install one app on a phone, and from then on push a project to it from an IDE and watch it run natively in seconds. The app is not a shell around a compiled build. Pushed classes are interpreted on the device against the framework already compiled into it, so nothing is built, signed or installed between edits. How the pieces fit ------------------ com.codename1.interp is the interpreter: one interpreted frame per real frame, so Display.invokeAndBlock and every blocking idiom built on it still work. A per-thread fuel counter bounds runaway code, and the budget is per entry into the interpreter rather than per session -- measuring it per session kills every callback that arrives later than the budget, which in an application whose whole life is callbacks is every button press. Interpreted classes reach the framework through InterpLinker: invoke thunks on iOS, reflection on Android. A linker must dispatch on the receiver's class, not the call site's declared type -- list.add(x) names java.util.List, and resolving from there finds AbstractList.add, whose body throws. Extending a framework class needs an object the framework accepts, which neither platform can define at run time. Generated shims provide it: every public, non-final, constructible class and every public interface the device exposes, derived by scanning the framework jar and codenameone-java-runtime rather than curated. A hand-maintained list is a promise that applications only subclass what somebody anticipated, and its failure mode is not an error but an override that is silently never called. Lambdas and method references are rewritten into real classes when the bundle is written, since neither target has a runtime invokedynamic. Enums are answered by the interpreter, java.lang.Enum having no shim and needing none. Store compliance is built in rather than bolted on: the runtime refuses to load a bundle whose sources it cannot show, and shows them. Verified -------- 4798 core tests, 506 translator tests, 43 interpreter tests, SpotBugs at zero, and a 20-program device battery (scripts/devruntime-probes) passing on both an Android emulator and the iOS simulator -- including a four-file, three-package application entered through Lifecycle rather than main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Listing text in fastlane's layout so supply and deliver can consume it, a privacy statement for both stores' data forms, and a scheduled workflow. The weekly job uploads to Play internal testing and TestFlight. It does not promote to production and does not submit for App Store review, which is a decision rather than an omission: a weekly automatic release would put unread builds in front of the public and queue an iOS review every week whether or not anything changed. Promotion stays one command, taken deliberately. Without publishing credentials the job reports which secrets are missing and stops, rather than publishing half a release. None of them exist yet. The review risk is written down rather than discovered later. This app runs code it did not ship with, which is squarely Guideline 2.5.2 -- permitted for tools that develop or test code, and only while the source stays viewable and editable on the device. That is why the runtime refuses a bundle it cannot show the source for. 4.7.2 is the sharper edge and the argument to make is that this is point to point developer tooling rather than a mini-app platform. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ate every push Three things the review asked for, and two defects found on the way. The interpreter moves from com.codename1.interp to com.codename1.impl.interp. It is an implementation detail of one app, not public API, and the impl hierarchy is what keeps it out of the javadoc. Note the package name is not only a Java name: ParparVM's dead-code pass recognises the runtime's own classes by their C-mangled prefix, so Parser.isLoadBearingForInterp moved with it. Missing that would have stubbed out InterpRuntime.run in an interp-host build, which fails by succeeding -- every pushed program "runs" instantly and executes nothing. The ~1000 generated shims leave git. They are a mechanical function of the framework jar, so the build generates them: a tools module builds the generator, exec-maven-plugin runs it into target/generated-sources/shims, and build-helper adds that as a source root. scripts/generate-interp-shims.sh keeps the three properties the build takes on faith -- every shim compiles, the load-bearing ones exist, generating twice is identical -- and now asserts them against a scratch tree instead of writing into src. Pairing no longer hands out a bearer token. v2 authorised a push with a peer id sent in the clear, so capturing one frame on a LAN meant pushing arbitrary code to somebody's phone forever. v3 derives a 256-bit secret on both ends from the typed code, the peer id and the device id -- never transmitted, 20k HMAC iterations so grinding six digits costs something -- and every connection answers a fresh challenge whose MAC covers the bundle. Authentication happens before the approval prompt, so nobody can raise dialogs on a stranger's phone until they tap Approve to stop them. What this still does not defeat is a passive observer of the pairing exchange itself, which the docs now say plainly. There are two implementations of the derivation, since ParparVM has no javax.crypto; InterpPairingSecretTest runs both and compares. Also fixed: - A class initializer that threw left the class marked initialized, so later reads returned whatever half of it had been assigned. Four states and an owning thread now, per JLS 12.4.2. - Sources were keyed by file name, so two Util.java in different packages collided and the runtime refused the program with "missing the source file Util.java" for a file it had been handed. Keyed by package now. - The iOS release job resolved ExportOptions.plist relative to the generated Xcode project, which is not where it lives. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
7623680 to
c6c6ada
Compare
check-copyright-headers gates every added source file, and the probes and the IDE sample are ours -- not third-party, so the exclusions file (which is for provenance, and rejects anything else) is the wrong place for them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
💡 Codex Review Register the Android linker before checking support
On every Android launch this support check is false because no code installs the newly added  Stop the previous program before replacing its runtime
When the normal “Push again … to replace it” workflow loads a second bundle, this assignment discards the service's reference to the previous runtime without requesting cancellation or invoking the previous Propagate failed LAN pushes as process failures
For an already-paired LAN push, Validate a discovered peer before remembering its address
If any unrelated service happens to accept this port during the subnet sweep, the callback marks it as found before ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3118d715e7
ℹ️ 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".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three were real: a SecureRandom built per call (worse and slower than one seeded once, and what it generates is the pairing code), two Files.createDirectories calls on a getParent() that SpotBugs cannot prove non-null, and an ExecutorService.submit whose Future was never going to be read -- execute() says what the scan actually wants. The other two are recorded in spotbugs-exclude.xml with their reasons: a command-line tool exits, and a failure while enumerating this machine's interfaces must be answered with 'no device found' rather than by killing the push. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
💡 Codex Review Tear down the previous runtime before replacing it
When a second bundle is pushed, this assignment only drops the service's reference to the previous runtime; it neither requests cancellation nor invokes any lifecycle cleanup. Peers, background threads, timers, and framework listeners retain references to the old runtime, so the supposedly replaced application can continue executing and mutate the UI or shared resources while the new application runs. Add a runtime deactivation/cleanup path and call it before publishing the replacement. CodenameOne/CodenameOne/src/com/codename1/impl/interp/InterpRuntime.java Lines 1347 to 1350 in 7e92c49 Route Object monitor methods to interpreted-object monitors
For a peerless interpreted object, calls inherited from Include Kotlin files in pushed source bundles
When compiled output contains Kotlin classes, even explicitly passing a Kotlin source directory to Discover Lifecycle subclasses through the class hierarchy
Entry-point discovery recognizes only classes whose immediate superclass is ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4d7ad9bda
ℹ️ 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".
Cloudflare Preview
|
Seven findings, all real. The interpreter: - A class literal for a pushed type puts an InterpClass on the stack, because there is no host class object to hand back -- and then the bytecode goes on calling java.lang.Class methods on it, which no linker can serve. The part of Class that means anything here (naming, identity, isInterface, isInstance, getSuperclass) is answered by the interpreter; anything else is refused by name rather than answered wrongly. - `new Entry[1][]` names its component `[LEntry;`, not `Entry`, so the bundle-membership test missed it and asked the host loader for a class only the bundle has. It looks through the brackets now, and multianewarray builds the nested Object[] itself rather than delegating. - JLS 12.4.1: initializing a class initializes the superinterfaces that declare a default method. Only those -- initializing all of them would run initializers Java never runs, which is as wrong as running them late. The push tool: - The Lifecycle entry point was chosen by direct superclass only, so a project whose app extends its own BaseApp entered BaseApp: an abstract class that was never meant to be instantiated. It walks the hierarchy now and takes the deepest concrete descendant. - A subnet scan treated any host that accepted TCP on the port as the device, and then failed the push against it while the real device sat unqueried. There is a PING frame now; only an answer in our own protocol wins. - cn1-push.sh still spoke v2, which nothing accepts any more. Its paired mode is gone rather than ported: it is a loopback helper, and pushing to a phone over Wi-Fi is DevicePush's job. A third copy of the derivation in a shell script would only drift from the two that have to agree. The release workflow now checks every secret the job will consume, not the two that name the store, so a half-configured store says so in preflight instead of half an hour later in the signing step. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1356d055bd
ℹ️ 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".
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CLDC11 keeps AbstractMethodError's constructors package-private, so the framework cannot throw one with a message and the Ant leg would not compile. IncompatibleClassChangeError carries the message, and a message naming the method is worth more here than the exactly right type. The three inline source blocks in the device runtime chapter move into docs/demos and are included by tag, which is what the guide validator asks of every other chapter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47fd057b7e
ℹ️ 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".
The interpreter's depth cap throws one, and it is the right type: ParparVM's java.lang has StackOverflowError and so does every JVM the simulator runs on. It was simply missing from this compile-time stub, so the Ant leg could not compile the framework while the Maven leg could -- the two disagree because only the Ant build puts CLDC11 on the bootclasspath. Reproducing that locally needs the same -bootclasspath; compiling core and CLDC11 together against a full JDK resolves java.lang from the JDK and reports nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
💡 Codex ReviewCodenameOne/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java Lines 1565 to 1567 in 3155e0b Register the Android linker during port initialization
On every Android runtime build, CodenameOne/CodenameOne/src/com/codename1/impl/interp/InterpRuntime.java Lines 677 to 679 in 3155e0b Preserve interpreted array component types
When the leaf type is interpreted, allocating every reference array as a plain ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Compared 12 screenshots: 12 matched. |
|
Developer Guide build artifacts are available for download from this workflow run:
Developer Guide quality checks:
|
The chapter was written before the rebase brought in the vale gate and had never been through it: 141 findings, all in this one file. The bulk is the guide's house style of contractions. The rest is adverbs that carried no weight, two sentences opening with 'So', and three quotations of literal text -- a runtime message, a device dialog, Google Play's policy -- where moving the period inside the quotes would misquote the source, so those carry a vale-skip naming the reason. Where an adverb was load-bearing, the sentence says the thing instead: 'which silently dropped Runnable' is now 'which dropped Runnable with no diagnostic'. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a54e8f350
ℹ️ 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".
The rebase reconstruction rewrote the file with LF where master has CRLF, so a 63-line change showed up as a 14,000-line rewrite. That is not only noise: CodeQL reports alerts for code a PR changed, and a whole-file diff re-reported twelve alerts that master already has and this branch did not introduce. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f86bfb26ac
ℹ️ 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".
The chapter used British spellings in a US-English guide -- which the cross-document coherency rule catches, not just the dictionary -- and a vocabulary LanguageTool has never heard of. Spellings are now US; the vocabulary (vtable, clazz, dex, desugar, devirtualize, supertype, cmake, thebaselab) is in the accept list with a line saying what each one is. Two sentences were rephrased rather than allowlisted: LanguageTool reads 'An interpreted X has to be an object...' as a typo for 'and' once the code spans are stripped, and the rule is right that the sentence was hard to parse. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79950c65f5
ℹ️ 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".
Codex reposted the `Object.clone()` thread. It's the same known non-goal from prior rounds -- Codename One doesn't carry `Cloneable`, and the device runtime inherits that stance -- so no behaviour change lands here. What was missing was a written answer to the finding so codex and the tree agree on the outcome. Added a paragraph at the dispatch site in `InterpRuntime.hostCall` that walks through why the fromHost hop unwraps a shim clone back to the original wrapper, and why the fix is not a runtime intercept: cloning a pushed object honestly needs a fresh InterpObject beside a fresh peer wired to each other, and adding that on the device runtime alone would put a Cloneable implementation on one target that no other Codename One target ships. Also a matching entry in the developer guide's known-deviations section under `Device-Runtime.asciidoc`, so the answer sits with the ArrayStoreException entry rather than only in the code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb2b3588d9
ℹ️ 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".
| } | ||
| } | ||
|
|
||
| private volatile Failure lastFailure; //NOPMD AvoidUsingVolatile - written from another thread on purpose |
There was a problem hiding this comment.
When two threads enter this runtime concurrently, any throw on thread B overwrites thread A's failure record; if A then executes throw e from its catch block, toThrowable() no longer recognizes the rethrow and replaces the original interpreted stack with the rethrow site, while cross-thread reporting can lose A's stack entirely. Fresh evidence beyond the earlier single-thread rethrow issue is that failure identity is still stored in this one runtime-wide slot even though the adjacent ThreadState explicitly supports concurrent entries; retain associations by throwable identity (with thread-local state for rethrow detection) instead.
Useful? React with 👍 / 👎.
P2 from codex: `lastFailure` was a runtime-wide slot even though the adjacent `ThreadState` already exists for exactly this reason. A throw on the event thread would overwrite what the pusher thread was about to rethrow, breaking rethrow-detection and losing cross- thread stack reporting. Failure identity now lives on `ThreadState.lastFailure` for the rethrow-detection path, so per-thread throws don't stomp on each other. Cross-thread lookup for `interpretedStackFor` and `hostCallFor` uses a new `WeakHashMap` keyed by throwable identity (Throwable inherits Object.hashCode/equals, so identity == equality here). `Collections.synchronizedMap` makes both entry and lookup safe from any thread, and weak keys mean a caught throwable can be released without keeping the runtime alive. Every write site (`toThrowable` and `hostCall`'s catch) routes through a single `recordFailure(st, f)` helper that sets the thread's slot and registers both `f.thrown` and (when distinct) `f.original` in the map, so a caller catching either the wrapper or the InterpObject can still ask for the interpreted stack. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82db26feb0
ℹ️ 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".
| private final java.util.Map failuresByThrown = | ||
| java.util.Collections.synchronizedMap(new java.util.WeakHashMap()); |
There was a problem hiding this comment.
When a host throwable subclass overrides equals() or hashCode(), this WeakHashMap is not identity-keyed: recording a distinct but equal exception can replace another exception's failure metadata, so interpretedStackFor() and hostCallFor() return the wrong throw site, and a mutable hash can make the metadata unreachable altogether. Throwable subclasses are free to override these methods, so the cross-thread registry needs weak identity keys rather than relying on ordinary map equality.
Useful? React with 👍 / 👎.
| failuresByThrown.put(f.thrown, f); | ||
| if (f.original != f.thrown && f.original instanceof Throwable) { //NOPMD CompareObjectsWithEquals - identity is the point | ||
| failuresByThrown.put(f.original, f); |
There was a problem hiding this comment.
Whenever a throwable is recorded, the WeakHashMap value is the Failure that strongly retains the same throwable in f.thrown (and sometimes f.original). The map therefore keeps its own weak key alive through the value, so completed exceptions are never reclaimed and a long-running device runtime accumulates every recorded failure. Store metadata that does not retain the key, or otherwise make both the identity key and back-references weak.
Useful? React with 👍 / 👎.
Two P2s on round 93's cross-thread failure registry. Identity keys: `WeakHashMap` uses equals/hashCode, and a Throwable subclass overriding either could collapse two distinct exceptions to one map entry (or make an entry unreachable when a mutable hash changed). Replaced with a plain `HashMap` keyed by `IdentityWeakRef`: a `WeakReference` subclass whose `hashCode` is captured `System.identityHashCode(referent)` at construction and whose `equals` is `==` on the referents. Two exceptions that `equals` each other now still get separate entries; a `hashCode()` mutation after recording can't hide the entry. No strong ref cycle: the previous `Failure` value strongly retained its own key (`f.thrown`, `f.original`), so `WeakHashMap` kept its own weak keys alive through the value and the map leaked every exception. Split off `FailureInfo` -- just `stack` and `hostCall` -- as the map's value; the full `Failure` with its throwable references stays only on `ThreadState.lastFailure` for rethrow detection. The reference queue drains on every put/get so cleared entries don't accumulate either. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 878685f073
ℹ️ 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".
| emitDispatch(w, m, "$runtime == null ? InterpRuntime.NOT_OVERRIDDEN\n" | ||
| + " : $runtime.dispatch($interp, \"" + m.getName() + "\", \"" | ||
| + descriptorOf(params, ret) + "\", new Object[]{" + boxed + "})"); |
There was a problem hiding this comment.
When a host superclass constructor invokes an overridable method implemented by the pushed subclass, this null guard reports NOT_OVERRIDDEN and runs the host superclass implementation instead, although Java dispatches to the subclass even before its fields are initialized. This affects an ordinary Form subclass overriding initGlobalToolbar(), because Form(Layout) invokes that protected hook during construction; the interpreted override is silently skipped, and an abstract constructor-time hook would instead produce AbstractMethodError. Make the interpreter/runtime context available to shim dispatch during the super(...) call rather than treating that window as unoverridden.
Useful? React with 👍 / 👎.
P2 from codex: a framework superclass constructor that invokes an overridable method during initialisation -- Form(Layout) calls initGlobalToolbar() -- reached the shim's override while `$runtime` and `$interp` were still null (Java assigns subclass fields only after super() returns). The old fall-through to super silently skipped the interpreted override; an abstract constructor-time hook would even hit AbstractMethodError. The shim now captures its (runtime, interp) context onto a thread- local stack in the arg-evaluation phase that precedes super(), and the dispatch helper falls back to that context when the fields aren't set yet. Concrete steps: * `InterpRuntime` gains a thread-local `PENDING_SHIM_CONTEXT` stack, `push/popPendingContext` helpers, and `dispatchOrDeferred(rt, io, name, desc, args)` which uses the stack top when `rt == null`. * Every override the generator emits (including toString / hashCode / equals) now routes through `dispatchOrDeferred` -- same number of lines, cleaner call, and it does the right thing during super(). * Each shim gets a `$captureCtx(rt, io) -> Void` helper. Every public constructor is a `this()` chain that pushes the context via `$captureCtx` in argument-evaluation (before super()); the private companion runs `super(...)`, assigns the fields, and pops. The chain shape stays uniform across zero-arg and primitive-first-arg constructors, so there is no per-first-type overload to emit. * `generate-interp-shims.sh` still emits 842 class + 279 interface shims, all compiling and reproducible. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e31810d348
ℹ️ 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".
| w.println(" super(" + call + ");"); | ||
| w.println(" this.$runtime = runtime;"); | ||
| w.println(" this.$interp = interp;"); | ||
| w.println(" InterpRuntime.popPendingContext();"); |
There was a problem hiding this comment.
When a framework superclass constructor throws—either because its arguments are invalid or because a constructor-time interpreted override fails—execution never reaches this popPendingContext() call. The static ThreadLocal stack therefore retains the InterpRuntime and InterpObject for the lifetime of the framework thread, so repeated failed constructions or pushes on the long-lived EDT accumulate entire bundle graphs; the stale top entry can also be used by a later null-field deferred dispatch. Clean up the matching pending context on the exceptional return from peer construction as well as on this success path.
Useful? React with 👍 / 👎.
P2 from codex: a super() that threw -- invalid args, or a
constructor-time interpreted override that failed -- never reached
the constructor body's `popPendingContext()`, so the shim's
`$captureCtx` push was stranded on the thread-local for the rest
of the thread's life. Repeated failed constructions (or the EDT
retrying) accumulated whole bundle graphs there and later
null-field deferred dispatch could pick up the stale top entry.
`InterpRuntime` gains `pendingContextDepth()` and
`trimPendingContext(depth)` helpers, and the shim generator now
wraps each `create()` body in `try { ... } finally { trim to the
depth captured before the constructor call }`. The success path
still runs the constructor's own pop first and the trim becomes
a no-op; the exception path (constructor abrupt completion)
brings the stack back where it was so nothing accumulates.
Shims regenerate at 842 class + 279 interface, all compile,
double-run reproducible.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aff96d3b56
ℹ️ 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".
| rt = (InterpRuntime) ctx[0]; | ||
| io = (InterpObject) ctx[1]; | ||
| } | ||
| return rt.dispatch(io, name, descriptor, args); |
There was a problem hiding this comment.
When a framework constructor invokes an interpreted override and that override calls an inherited host method on this—for example, Form.initGlobalToolbar() calling setToolbar()—this dispatch enters interpreted code while io.hostPeer is still null, because createPeer() assigns it only after the factory constructor returns. The inherited call therefore cannot run against the partially constructed shim and ends in the peerless fallback with an IncompatibleClassChangeError or null-target failure. Fresh evidence after the deferred-dispatch fix is that the pending context carries only the runtime and wrapper, not the actual shim receiver; expose that receiver as the temporary peer during super(...).
Useful? React with 👍 / 👎.
| Object thrown = it.getThrown(); | ||
| int handler = findHandler(m, insn, thrown, false); | ||
| if (handler < 0) { |
There was a problem hiding this comment.
When pushed code wraps a retry loop in catch (Throwable)—for example, for (;;) { try { workForever(); } catch (Throwable t) {} }—routing InterpCancelled into this handler lets the loop resume, and the still-set flag merely raises another catchable cancellation at a later checkpoint. Consequently the Stop request or EDT budget never unwinds the frame and the event thread can remain pegged indefinitely. Allow cleanup handlers to execute, but keep cancellation pending so a handler that completes normally cannot resume the program.
Useful? React with 👍 / 👎.
| if ("package".equals(token)) { | ||
| // Terminated by `;` (Java) or end-of-line (Kotlin, whose |
There was a problem hiding this comment.
When a Kotlin file has a file annotation whose qualified name contains an escaped keyword before the real declaration, such as @file:com.foo.package.Ann followed by package real.pkg, this scanner treats the package text inside the backticks as the declaration and returns the remainder of the annotation line. The source is then stored under the wrong key, so requireSourcesFor() rejects the generated real/pkg/... classes as missing their source; package keywords should only be recognized outside backtick-escaped identifiers.
Useful? React with 👍 / 👎.
Three P2s from codex.
Constructor-time overrides can now reach inherited host methods on
`this`. Round 95's `dispatchOrDeferred` only carried (runtime,
interp) through the pending context, so an override that called
back into `super.setToolbar(...)` during `super()` reached hostCall
with `io.hostPeer == null` and ended in the peerless fallback with
IncompatibleClassChangeError. The dispatch helper now takes the
shim (`this` inside the generated override), binds it as
`io.hostPeer` when hostPeer is null, and sets `io.hostPeerOwner`
via `factory.peerClassName(shim)` so the host-side dispatch path
finds the right owner. `createPeer` assigns the same reference
back once the factory returns; the write is idempotent.
`InterpCancelled` no longer heartbeats through a retry loop. A
`for(;;) { try { work(); } catch (Throwable t) {} }` used to keep
catching every re-raised cancellation and running forever, because
the next checkpoint's throw was catchable by the same handler.
`ThreadState.cancelCaughtOnce` now flips once any handler matches
InterpCancelled; `findHandler` from then on only lets catch-all
(finally) entries match a subsequent InterpCancelled, so cleanup
runs but the retry loop unwinds. Cleared with `cancelRequested` at
the start of every `runMain`.
`packageOf` now skips backtick-escaped identifiers as opaque
tokens. A Kotlin annotation naming an escaped keyword before the
declaration -- `@file:com.foo.`package`.Ann` followed by `package
real.pkg` -- was reading the word inside the backticks as the
declaration; the source got the wrong key and the reader refused
the whole push. New assertion covers the case.
Shims regenerate at 842 class + 279 interface, all compile; 5274
tests pass, SpotBugs 0 on core-unittests and ByteCodeTranslator.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6138a08007
ℹ️ 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".
| if (thrown instanceof InterpCancelled) { | ||
| state().cancelCaughtOnce = true; |
There was a problem hiding this comment.
Fresh evidence after the catch-once change is that this flag is set when a cancellation traverses a typed cleanup handler but is reset only by runMain(). If that cancellation escapes a callback, a later callback on the same EDT starts with cancelCaughtOnce still true; its next budget cancellation therefore skips every typed catch (Throwable), including javac's try-with-resources cleanup handler, so resources acquired by later callbacks are not closed. Reset this state when a cancellation leaves a top-level interpreter entry while preserving it during that cancellation's unwind.
Useful? React with 👍 / 👎.
Cancellation state is now per-interpreter-entry, not per-thread lifetime. Round 97 set `ThreadState.cancelCaughtOnce` when a typed handler swallowed an `InterpCancelled` and only cleared it in `runMain`. On the EDT that meant a Stop that traversed a try-with-resources cleanup in one listener left the flag set for every subsequent listener called on the same thread; the next budget cancellation those saw skipped their own typed `catch (Throwable)`, so the resource the callback opened was never closed. The reset moves to the fresh-entry branch of `invokeInterpreted`, next to the runStart/fuel/hostCallDepth reset. A nested (non-fresh) entry -- a callback invoked from inside another interpreted frame -- shares the enclosing entry's cancellation unwind and does not reset; a fresh entry that follows the unwind's return to depth 0 starts clean. `runMain`'s clear stays, harmless but redundant, so the direct-run path never observes stale state either. 5274 tests pass, SpotBugs 0 on core-unittests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efc37768b1
ℹ️ 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".
| if (!Modifier.isStatic(mods) && !Modifier.isPrivate(mods) | ||
| && !Modifier.isAbstract(mods)) { |
There was a problem hiding this comment.
When pushed code invokes a method through a host abstract class that merely inherits the method from an interface (for example, abstract class A implements I {} with a concrete B extends A), javac emits invokevirtual A.m, but this filter discards I.m because it is abstract. lookupMethod() then throws NoSuchMethodException instead of letting Method.invoke dispatch to B.m. The same filter exists in the inspected JavaSE linker, so abstract interface declarations must remain available for class-owner method resolution even though they need separate handling from competing defaults.
Useful? React with 👍 / 👎.
| // pick one arbitrarily. The host will reject the mismatched slot | ||
| // -- but that mismatch is present in the pushed program, not | ||
| // introduced here. | ||
| return a; |
There was a problem hiding this comment.
After the superclass/subclass alias fix, an interpreted Both[] whose element type implements two unrelated host interfaces still fails when the same array is passed as both A[] and B[]. Neither interface descriptor is more specific, so this fallback chooses A[], assigns that one materialization to both arguments, and reflective invocation rejects it for the B[] parameter even though a real Both[] is valid for both. Preserve the interpreted component type or derive a materialization from the common peer type rather than arbitrarily choosing one sibling descriptor.
Useful? React with 👍 / 👎.
| // line of perfectly ordinary Java, and reading it as the default | ||
| // package stored the source under a key the runtime never looks up -- | ||
| // so the push was refused for missing source that had been supplied. | ||
| String code = stripComments(decodeUnicodeEscapes(text)); |
There was a problem hiding this comment.
When a Kotlin file has a pre-package file annotation containing a Unicode escape that produces a quote, such as @file:Suppress("\u0022 package fake") followed by package real, the escape is part of the Kotlin string value and does not terminate the literal lexically. Decoding it before blanking literals creates a synthetic quote delimiter, which can expose the fake package token or consume the real declaration; the source is then keyed under the wrong path and requireSourcesFor() rejects the push as missing source. Apply Java's pre-lexing Unicode expansion only to Java sources, or scan Kotlin literals before interpreting their escapes.
Useful? React with 👍 / 👎.
Three P2s from codex.
Class-owner resolution keeps abstract interface methods in the
candidate pool. `invokevirtual A.m` on `abstract class A implements
I {}` -- with a concrete B extends A -- resolves to I.m even when
I.m is abstract; reflection's Method.invoke dispatches virtually to
B.m. The old filter dropped abstracts before the maximally-specific
pass, so lookupMethod threw NoSuchMethodException and every push
that touched such a method died with "not on host". Concrete
defaults still win when both are present: `findInInterfaces`
partitions the candidates and runs the JVMS 5.4.3.3 pass on the
concrete pool when it is non-empty, falls back to the abstract pool
otherwise, and only raises IncompatibleClassChangeError for
concrete conflicts (all-abstract pools cannot conflict -- every
entry dispatches through the receiver). Android, JavaSE and the
test-only ReflectionInterpLinker are aligned.
Aliased arrays no longer collapse across unrelated sibling
descriptors. When the same interpreted `Both[]` (elements
implementing two disjoint host interfaces) landed in one A[] slot
and one B[] slot, the arbitrary A pick was assigned to both slots
and reflection rejected the A[] where a B[] was declared. Per-src
now clusters descriptors by covariance -- Component[] and Button[]
of the same Button[] src still share one Button[] dst -- and
materialises one array per cluster, keying by (src, descriptor).
Unrelated siblings form separate clusters and each slot receives
the array that satisfies its own declared type.
`moreSpecificElement` returns null on unrelated so the caller can
recognise the split rather than silently pick.
packageOf no longer runs Java's backslash-u preprocessing on
Kotlin sources. Kotlin does not preprocess -- an escape lives
inside the string, not the source -- so decoding it before
stripComments would turn a Kotlin annotation like
`@file:Suppress("... package fake")` into a synthetic quote that
swallows the real `package` line and refuses the push. The
overload takes the file name and gates decoding on `.java`; the
old single-arg entry delegates for back-compat. New test verifies
the Kotlin escape stays inert.
Shims regenerate at 843 class + 279 interface, all compile; 5275
tests pass, SpotBugs 0 on core-unittests and ByteCodeTranslator.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e031a4d69d
ℹ️ 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".
| f.pushRef(isInterpretedLeaf(comp) || isClassLeaf(comp) | ||
| ? new Object[count] |
There was a problem hiding this comment.
When Foo is interpreted, legal code such as Foo[] values = new Foo[1]; Object[] alias = values; alias[0] = "wrong"; must throw ArrayStoreException, but this allocation erases Foo[] to an untyped Object[], and the AASTORE path consequently accepts the string. Later reads can therefore return a value that is not a Foo and fail at an unrelated invocation; retain the interpreted component descriptor with the array and enforce it on stores rather than using an indistinguishable plain Object[].
Useful? React with 👍 / 👎.
| // back -- the array is the caller's now, and interpreted code | ||
| // reading an element converts on the way in. | ||
| toHostElements(result, new Vector()); | ||
| } | ||
| return result; |
There was a problem hiding this comment.
When an interpreted override of a host method returning Component[] returns new MyButton[] where MyButton is interpreted, the result here is still the plain Object[] representation; converting its elements does not make the array assignable to Component[]. The generated shim then casts $r to the declared return type and throws ClassCastException, so return arrays need to be materialized from m's return descriptor at this host boundary just as argument arrays are materialized before host calls.
Useful? React with 👍 / 👎.
Two P2s from codex, plus the CI leg that only the Ant core catches. Reference-tracked failures no longer depend on ReferenceQueue. CLDC's `java.lang.ref` ships Reference and WeakReference but no queue, so the Ant core (compiled under `-bootclasspath Ports/CLDC11/dist/CLDC11.jar`) failed with three "cannot find symbol" errors on the field, the constructor and the argument to IdentityWeakRef -- the whole simulator screenshot leg went red for lack of a class Maven happens to see. The map is walked directly now: `IdentityWeakRef` takes only the referent, and `sweepClearedEntries` iterates `failuresByThrown.keySet()` under the map's monitor and removes any entry whose weak `get()` has returned null. Sweep runs on every put -- exceptions are rare relative to invocations, so the linear scan is cheap and bounded by the small live set. Return arrays materialise to their declared type at the `invoke(InterpMethod, ...)` boundary. An interpreted override of `Component[] getSelected()` returned the interpreter's plain `Object[]`; the generated shim's implicit cast to `Component[]` then failed because `[Ljava.lang.Object;` is not assignable to `[Lcom.codename1.ui.Component;`. The host boundary now reads the declared return descriptor and rebuilds through `materializeTypedArray` when it names a reference-element array whose leaf class is host-visible. Interpreted-only leaves stay Object[] -- that is the same known deviation documented for ArrayStoreException. The `ArrayStoreException`-known-deviation code comment now points at that same section of `Device-Runtime.asciidoc` from the `ANEWARRAY` site so the review thread has an in-source landmark; the developer-guide contractions and one adverb the doc-quality gate flagged are rewritten to match the Microsoft style pass, and "synthesising" is now the American "synthesizing" that LanguageTool wanted. 5275 tests pass, SpotBugs 0 on core-unittests and BCT; CLDC-only compile of the interp package under `-bootclasspath Ports/CLDC11/dist/CLDC11.jar` also passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f67c2452c2
ℹ️ 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".
An interpreted PUTFIELD into a compiled object wrote the reference straight through the field offset, skipping both barriers the translator emits around every generated reference setter. Each omission is its own bug. Without CN1_WRITE_BARRIER a nursery value stored into a heap object never gets promoted, so the nursery frees an object the host object still points at. Without CN1_SATB_DELETE the reference being overwritten is not handed to the collector, so a reference that was in the start-of-cycle snapshot can be dropped by a thread the mark has already scanned and swept while still live. Both are silent at the store: it succeeds, and the damage surfaces a cycle later in a mark walking a field that no longer points at a live object -- the shape this collector's hardest bugs have taken before. Both barriers now run in the same order the generated setter uses. Statics were already covered: setStaticById dispatches through set_static_*, which pays them. Reads need neither. Type-checked in both the nursery and no-nursery configurations of the macros. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ec35be841
ℹ️ 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".
| if (kind == K_OBJECT) { | ||
| return *(JAVA_OBJECT*)slot; |
There was a problem hiding this comment.
When interpreted code reads or writes a volatile instance field declared by a compiled host class on iOS, this raw pointer access bypasses the generated field accessors' atomic_load_explicit(..., memory_order_acquire) and atomic_store_explicit(..., memory_order_release). Since volatile fields are emitted as _Atomic storage but cn1_field_entry does not record volatility, concurrent host and interpreted threads can observe stale state or enter a C data race; carry the volatile flag in the field metadata and use atomic loads/stores for these slots (while retaining the reference write barriers).
Useful? React with 👍 / 👎.
One P2 from codex: preserve volatile semantics for iOS host fields accessed from interpreted code. The translator emits `volatile` instance fields as `_Atomic` storage and generates host accessors that use `atomic_load_explicit(..., memory_order_acquire)` and `atomic_store_explicit(..., memory_order_release)`. Interpreted GETFIELD/PUTFIELD reach the same slot through the generic `cn1_reflect_field_for` table and dereferenced the pointer plainly, so a host thread and an interpreted thread reading the same volatile could tear the read, reorder it, or miss a happens-before established by a host write. `cn1_field_entry` now carries an `isVolatile` char, the translator populates it from `ByteCodeField.isVolatile()` when it emits the per-class table, and the two iOS interp accessors -- `getFieldById` and `setFieldById` -- dispatch through `atomic_load_explicit(memory_order_acquire)` / `atomic_store_explicit(memory_order_release)` on `_Atomic(T)*` for each JVM type character when the flag is set, matching the ordering the generated host accessors already use. The reference write barriers (`CN1_WRITE_BARRIER`, `CN1_SATB_DELETE`) still fire either way -- those are GC bookkeeping, not memory ordering. `<stdatomic.h>` is now included by `cn1_interp_ios.m` for the atomic intrinsics. 5275 tests pass, SpotBugs 0 on core-unittests and BCT. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63547f227e
ℹ️ 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".
| CORE_JAR="$(ls "$REPO_ROOT"/.m2-local/com/codenameone/codenameone-core/*/codenameone-core-*.jar 2>/dev/null | head -1 || true)" | ||
| if [ -z "$CORE_JAR" ]; then | ||
| CORE_JAR="$(ls "$HOME"/.m2/repository/com/codenameone/codenameone-core/*/codenameone-core-*.jar 2>/dev/null | head -1 || true)" |
There was a problem hiding this comment.
When the local Maven repository contains multiple core versions or attached source/Javadoc artifacts, this glob followed by head -1 can select the oldest version or a classified JAR (classified names commonly sort before the binary JAR). The subsequent javac invocation then either misses all Codename One classes or compiles against a stale API, so cn1-push.sh fails or packages against the wrong runtime; filter classifiers and select the intended/current project version explicitly.
Useful? React with 👍 / 👎.
| OutputStream out = Files.newOutputStream(f); | ||
| try { | ||
| p.store(out, "Codename One device runtime -- shared secrets, one per paired device"); |
There was a problem hiding this comment.
On the first pairing on a POSIX multi-user machine, Files.newOutputStream(f) creates this predictable file using the process umask (commonly resulting in mode 0644), writes the phone-authorizing secret, and only afterward changes it to 0600. Another local user watching ~/.codenameone can read the secret during that window and subsequently authenticate arbitrary pushes to the paired device; create the file or a replacement temporary file with owner-only permissions before writing any secret bytes.
Useful? React with 👍 / 👎.
Two P2s from codex, plus a PMD gate the last round failed. The pairing secret file is now created owner-only from the start, not chmodded to 0600 after the write. Files.newOutputStream(f) opened the file with the process umask (0644 on most POSIX defaults) and only later called setPosixFilePermissions; another local user on the machine could read the phone-authorising secret during that window and subsequently authenticate arbitrary pushes. Write goes to a sibling temp file created with a FileAttribute carrying "rw-------" (Windows falls back to the plain temp file, whose default ACL is already user-only), and an ATOMIC_MOVE with REPLACE_EXISTING renames it into place -- so the target ends up owning the temp's mode and an existing file with laxer permissions is replaced rather than reopened. AtomicMoveNotSupportedException falls back to a plain move. cn1-push.sh picks the intended core / parparvm jar rather than whichever entry `ls | head -1` happens to land on. A local repo that also has -sources / -javadoc / -bundle classifiers (they sort before the plain jar) or older versions alongside the intended one would previously feed javac an attached artifact or a stale API, and the push either failed or packaged against the wrong runtime. `pick_latest_artifact` walks the per-version directory in `sort -V -r` order and returns the first `<prefix>-<v>.jar` that exists, so classifiers are ignored by construction and the newest version wins. The Ant PMD gate flagged `hostThrown instanceof InterpCancelled` inside a `catch (Throwable)` in `run()`. That arm was dead -- `InterpCancelled`'s constructor is package-private and every creation site wraps it in `InterpThrowable`, which the catch above handles -- so the check is deleted, with a comment recording why no cancelCaughtOnce update is needed there. 5275 tests pass, SpotBugs 0 on core-unittests and BCT. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 679b765cc9
ℹ️ 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".
| bits = (uint64_t)atomic_load_explicit( | ||
| (_Atomic(uint64_t)*)slot, memory_order_acquire); |
There was a problem hiding this comment.
When interpreted code accesses a compiled volatile float or volatile double field on iOS, the generated storage is _Atomic(JAVA_FLOAT) or _Atomic(JAVA_DOUBLE), but this code accesses it through _Atomic(uint32_t)* or _Atomic(uint64_t)*. Those are incompatible atomic object types under C's aliasing rules, so the load and matching store have undefined behavior and may be miscompiled despite having the correct size. Atomically load/store the declared floating type and use memcpy only between the resulting local value and its raw bits.
Useful? React with 👍 / 👎.
| if (merged != null) { | ||
| clusterReps.setElementAt(merged, c); | ||
| mergedInto = c; | ||
| break; |
There was a problem hiding this comment.
When the same pushed MyBoth[] is passed to a host signature ordered as f(A[], B[], HostBoth[]), where HostBoth implements the unrelated interfaces A and B, processing first creates separate A and B clusters and then merges HostBoth into only the first cluster because of this break. All three arguments subsequently select the HostBoth[] destination, but the unused B[] destination remains in hostArrayPairs and is mirrored back after the call, overwriting mutations made through the array the host actually received. Fresh evidence after the sibling-interface fix is that a later descriptor can bridge previously separate clusters; merge all now-compatible representatives or omit unused materializations from mirroring.
Useful? React with 👍 / 👎.
| Failure prev = st.lastFailure; | ||
| boolean rethrow = prev != null //NOPMD CompareObjectsWithEquals - identity is the point | ||
| && (prev.thrown == t || prev.original == t); |
There was a problem hiding this comment.
When a catch block handles another exception before rethrowing its original exception—for example, catch (Exception e) { try { riskyLog(); } catch (Exception ignored) {} throw e; }—the nested failure replaces lastFailure, so this identity check no longer recognizes e. The rethrow then records the current site instead of preserving the original interpreted stack. Fresh evidence after making this slot thread-local is that multiple failures on the same thread still overwrite one another; consult the existing identity-keyed failure registry rather than only the most recent slot.
Useful? React with 👍 / 👎.
Three P2s from codex.
Volatile float / double on iOS now round-trip through their
declared atomic types. Round 101 dispatched through
`_Atomic(uint32_t)*` / `_Atomic(uint64_t)*` for `F` / `D` fields,
but the storage the translator declares is `_Atomic(JAVA_FLOAT)`
/ `_Atomic(JAVA_DOUBLE)` -- an incompatible atomic object type
under C aliasing, so clang is free to miscompile the pair even
though the widths match. The load / store now use the declared
floating type; `memcpy` moves between the local value and the raw
bits the interpreter's long slot uses.
Sibling-interface array clusters coalesce transitively. Round 99
formed one cluster per unrelated descriptor and merged each new
candidate into the first cluster it met, but a later descriptor
that bridges two previously separate clusters -- `HostBoth[]`
after `A[]` and `B[]`, where `HostBoth` implements both -- left
the other cluster stranded. The stranded cluster still
materialised an array that never reached the host, and the finally
mirror wrote its contents back over the mutations the host had
made through the array it did receive. After the initial pass a
fixed-point loop coalesces any two cluster reps that
`moreSpecificElement` can merge, so every pair that shares a
common subtype ends up in a single cluster.
Rethrow detection consults the identity-keyed failure registry as
well as `lastFailure`. `catch (Exception e) { try { risky(); }
catch (Exception ignored) {} throw e; }` overwrote the slot with
`ignored` between the outer catch and the rethrow, and the
identity match against the slot alone then missed `e`, replacing
its recorded interpreted stack with a snapshot of the rethrow
site. `toThrowable` now falls back to `getFailureInfo(t)` on the
Throwable path: if the registry still holds an entry for `t`, the
recorded stack lives on and the wrapper (for the InterpObject
path) reuses it too.
5275 tests pass, SpotBugs 0 on core-unittests.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47feded9ee
ℹ️ 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".
| boolean rethrow = prev != null //NOPMD CompareObjectsWithEquals - identity is the point | ||
| && (prev.thrown == t || prev.original == t); | ||
| String[] rethrowStack = rethrow ? prev.stack : null; | ||
| if (!rethrow && t instanceof Throwable) { |
There was a problem hiding this comment.
When the outer exception is declared in pushed code, the caught value is an InterpObject, not a Throwable, so this registry fallback is skipped. If its catch block handles another failure before throw e, that failure overwrites lastFailure; the rethrow then receives a new stack rooted at the rethrow site instead of the original throw site. Fresh evidence beyond the existing rethrow finding is that the new registry lookup is explicitly limited to host Throwable instances and recordFailure() cannot index the interpreted original. Track interpreted exception identities as well as host throwables.
Useful? React with 👍 / 👎.
One P2 from codex: the round-103 rethrow-registry fallback only
covered Throwable exceptions; an interpreted-only exception's
caught value is an InterpObject, so the registry lookup was
skipped and the outer rethrow after a nested catch still lost the
original stack.
`putFailureInfo` / `getFailureInfo` now take `Object` and the
`IdentityWeakRef` weak key accepts any Object identity.
`recordFailure` registers the `original` -- Throwable or
InterpObject -- alongside `thrown`, so a rethrow of an interpreted
exception whose `lastFailure` slot was overwritten by a nested
`catch (Exception ignored) {}` still resolves through the
registry to its recorded interpreted stack. Widening the key type
does not change what `interpretedStackFor(Throwable)` looks up --
its argument is still a Throwable -- but it does let the
InterpObject path in `toThrowable` reach the same registry.
5275 tests pass, SpotBugs 0 on core-unittests.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be1c8098e0
ℹ️ 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".
| if (!loopback) { | ||
| reject = "this app requires a paired computer; upgrade the push tool"; | ||
| } else { |
There was a problem hiding this comment.
When another Android app binds 127.0.0.1:18234 before this runtime starts, startListener() merely reports that the port is unavailable and the dialer connects to that app, but this branch then accepts its v1 bundle without pairing or approval because the configured address is loopback. Android apps share the device's TCP loopback namespace, so a local app that wins this predictable port can make the runtime execute an arbitrary bundle; require the authenticated protocol even on loopback or establish that the peer is specifically the USB tunnel rather than treating the address itself as authentication.
Useful? React with 👍 / 👎.
| Object[] typed = materializeTypedArray((Object[]) result, elem, null); | ||
| if (typed != null) { | ||
| return typed; |
There was a problem hiding this comment.
Fresh evidence after the return-type materialization fix is that it returns a new array with no ongoing association to the interpreted source. If an override retains Component[] items = new PushedButton[...] in a field and returns it, the host receives this typed copy; a later host mutation changes only the copy, while interpreted code continues reading the original Object[], and later interpreted mutations are likewise invisible to the host. Preserve shared array identity or synchronize the retained representations beyond this one-time conversion.
Useful? React with 👍 / 👎.
| // its budget expires. Nested (non-fresh) entries do NOT reset | ||
| // -- they participate in the same cancellation unwind as the | ||
| // enclosing entry. | ||
| st.cancelCaughtOnce = false; |
There was a problem hiding this comment.
Fresh evidence after resetting cancellation state per interpreter entry is that a fresh entry can also be a reentrant callback nested inside an outer entry's host call. If the outer cancellation has already traversed a typed handler, a short callback dispatched by that handler resets cancelCaughtOnce here and the enclosing state is never restored in the finally; the outer retry loop can consequently catch the same pending cancellation through catch (Throwable) again, allowing Stop or the EDT budget to be swallowed repeatedly. Save and restore this flag alongside the enclosing clock, fuel, and host-call depth.
Useful? React with 👍 / 👎.
One P1 and two P2s from codex. Loopback v1 pushes are no longer accepted on a real device. Codex noticed that Android apps share the TCP loopback namespace: another local app can connect to this listener over 127.0.0.1 and would otherwise get an unpaired v1 bundle run because `loopback == true` was treated as authentication. The physical-presence argument only holds inside the JavaSE simulator, where "loopback" really does mean this same process's peer; on device (Android in particular), require the paired v3 protocol. The v1 branch now gates on `Display.getInstance().isSimulator()` as well as `loopback`, and the comment on the enclosing block records why. Cancellation-in-flight state is now saved and restored across a reentrant fresh entry. Round 98 reset `cancelCaughtOnce` on every fresh entry so a stale flag from a prior EDT callback couldn't skip a later callback's typed cleanup; but a fresh entry can also be a reentrant callback dispatched inside an outer entry's host call. Resetting there and never restoring it let the outer retry loop's `catch (Throwable)` swallow the same pending cancellation each pass. `enclosingCancelCaughtOnce` is stashed alongside the enclosing clock / fuel / hostCallDepth before the reset and put back in the `finally` (and in the early-throw for a max-depth overflow), so the outer state is preserved through the callback. Returned typed arrays being one-time copies is now a documented known deviation. Codex followed the round-100 fix with a scenario -- an override retains `Component[] items = new PushedButton[...]` in a field and hands the same reference back -- where a later host mutation lives in the returned copy alone and a later interpreted write into the source stays invisible on the host side. Closing it means carrying an alias registry through every array opcode and every host boundary for a rare pattern against the host contract (these accessors are expected to hand back a snapshot, not an internal buffer). Recorded in `Device-Runtime.asciidoc` alongside the ArrayStoreException and `Object.clone()` deviations; the materialisation site now points at that section. 5275 tests pass, SpotBugs 0 on core-unittests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9d23a7350
ℹ️ 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".
| if (self == null || self.indexOf('.') < 0 || isLoopback(self)) { | ||
| return false; |
There was a problem hiding this comment.
On an IPv6-only Wi-Fi network, AndroidImplementation.getHostOrIP() deliberately falls back to a routable IPv6 address, but this condition rejects every address without a dot before attempting discovery. The device runtime then remains on the loopback default and cannot find the desktop, while getLocalAddress() similarly tells the UI that the device has no network address. Add an IPv6 discovery path or another way to dial the desktop rather than unconditionally refusing the only reachable address.
Useful? React with 👍 / 👎.
2 participants
Adds a device runtime: install one app on a phone, then push a project to it
from your IDE and watch it run natively in seconds. A third way to run a
Codename One app, alongside the simulator and a cloud device build.
Pushed classes are interpreted on the device against the framework already
compiled into it. Nothing is built, signed or installed between edits — the
edit-run loop measured 2.8 seconds end to end.
Try it
The desktop finds the phone on the local network, shows a six-digit pairing
code you type once, and from then on it is edit-and-run.
--device <address>is there for networks that block a scan.
What is here
CodenameOne/src/com/codename1/interp/Ports/{Android,iOSPort}vm/ByteCodeTranslatorDevicePushtoolscripts/cn1-device-runtime/scripts/devruntime-ide-project/scripts/devruntime-probes/docs/developer-guide/Device-Runtime.asciidocDecisions worth reviewing
Shims are generated over the whole API, never curated. A hand-maintained
list is a promise that applications only subclass what somebody anticipated, and
its failure mode is not an error message but an override that is silently never
called. The generator fails the build rather than pruning what will not compile
— a compile-and-drop loop once silently ate
Interp_ui_Form.Native-heavy subsystems are excluded from the shim set (
ai,ar,camera,surfaces,car,health, …). A shim is a compiled reference to theclass it extends, which is exactly what the build scans to decide what to link,
so generating the full API pulled 300MB of ML Kit, ARCore and CameraX natives
into an app that calls none of them. Cost: those types cannot be subclassed by
pushed code; calling them degrades to
isSupported() == false, which is theruntime's existing contract for a cn1lib without its native half.
iOS keeps shims rather than runtime vtable synthesis. Synthesis would make
14 more types extensible on iOS only, and Android cannot follow — so the usable
capability, the intersection, does not move.
InterpHostVtableSynthesisIntegrationTeststays for the day that changes.
synchronizeduses the real object monitor, not a private lock table, whichis what makes
wait/notifywork.Framework fixes that fell out
AndroidImplementation.getHostOrIP()returneddummy0's IPv6 link-localinstead of a usable IPv4 — affects any caller.
CodenameOneImplementation.getResourceAsStreamgained a local-resource hook,so a pushed program's
theme.resis found byResources.openLayered, whichnever passes through
Display.Verification
4798 core · 506 translator · 52 interpreter · SpotBugs 0 · 20-program device
battery green on an Android emulator and the iOS simulator, including a
four-file, three-package app entered through
Lifecyclerather thanmain.Every probe exists because something plausible turned out not to work; the
README records which defect each was written for.
Review rounds
Eleven findings from codex, all real, all fixed and each answered on its thread.
The two that mattered most:
every push and never rotated, so one captured frame authorised arbitrary code
on that phone forever. v2 is gone rather than deprecated. v3 derives a 256-bit
secret on both ends from
(typed code, peerId, deviceId)— never transmitted —and every connection answers a fresh challenge whose MAC covers the bundle.
Authentication happens before the approval prompt, so nobody can raise dialogs
on a stranger's phone until they tap Approve to stop them. What it still does
not defeat is a passive observer of the pairing exchange itself, and the docs
say so.
reads returned whatever half of it had been assigned. Four states and an owning
thread now, per JLS 12.4.2.
Shipping it
.github/workflows/device-runtime-store.ymlruns Mondays and on demand,uploading to Play internal testing and TestFlight. It does not promote to
production and does not submit for review — a weekly automatic release would
put unread builds in front of the public and queue an iOS review every week
whether anything changed or not. Promotion stays one deliberate command.
Without credentials the job names the missing secrets and stops rather than
publishing half a release; none exist yet, so today it is a no-op that says so.
Listing text is in fastlane's layout (
scripts/cn1-device-runtime/fastlane/) sosupplyanddeliverconsume it directly, withstore/privacy.mdfor bothstores' data forms and
store/README.mdfor the secrets, the pre-submissionchecklist and the review-risk assessment.
The compliance point that matters: this app runs code it did not ship with,
which is Guideline 2.5.2 — permitted for tools that develop or test code, and
only while the source is "completely viewable and editable by the user". The
runtime refuses to load a bundle whose sources it lacks, and shows them under
View source. Removing that screen makes the app unsubmittable, which is why
the code says so where the screen is defined.
Not done
NativeLookupstubbing covers the Java half of a cn1lib; the native halfreports unsupported. Resource push covers
theme.res, CSS and images.Screenshots for both stores, the Play content rating questionnaire, Apple's
privacy manifest and the console listings themselves are human steps, listed in
store/README.md.🤖 Generated with Claude Code