Fix the iOS on-device debugger crashing at breakpoints (issue #5333) - #5536
Merged
Conversation
A breakpoint in an event listener took the app down with "We had a signal 11". Three independent wild-pointer paths converged on handleGetLocals, which is what NetBeans calls the moment a breakpoint hits: - The locals side-table listed one row per declared local but only one address per JVM *slot*, picked last-writer-wins out of a HashSet. A slot reused by an int and a reference in disjoint scopes -- routine in any listener -- therefore had a reference row addressing a four-byte JAVA_INT. The runtime read eight bytes off it and dereferenced the result. There is now one address per row, so a row's type code and the storage it points at cannot disagree, and the rows are sorted rather than hash-ordered so the emitted table stops varying between builds. - callStackFrameInfo / callStackLocalsAddresses were written only by methods carrying a side-table, and never cleared. A native, eliminated or barebone frame inherited the previous occupant of that call depth, including a locals-address array pointing into a C frame that had already returned. CN1_DEBUG_FRAME_ENTER clears both before every push and compiles away in release builds. - Nothing validated a reference before dereferencing it, though every one is untrusted: the IDE echoes back objectIDs it was handed earlier, and a local slot holds whatever the branch that never ran left there. cn1_debugger_objects.c resolves a candidate through an exact classId -> clazz registry, reading it via vm_read_overwrite so an unmapped page returns an error instead of a signal. Every deref site goes through it. Also addresses the rest of what the issue reported: - Threads. CMD_GET_THREADS was a device stub replying empty and the proxy never sent it, so AllThreads could only report threads that had already hit a breakpoint. It now enumerates ParparVM's thread registry; suspension is tracked per thread rather than by a single "last suspended" field, and Thread.Name resolves the real name off the java.lang.Thread instance. - ClassPrepare (event kind 8) was accepted and never fired, so IntelliJ and NetBeans left every deferred breakpoint unarmed -- the "breakpoint in a listener never stops" half of the report. Matching classes are now replayed at registration, which is complete for ParparVM since the whole class set is linked in before a debugger can attach. - Locals carry their declaring scope, so a local is listed only on the lines it is live for. This also fixes a -g local being shown as "vN": the placeholder a store opcode synthesises no longer keeps the class file's own entry out of the set that holds them. - CapabilitiesNew reports canRequestVMDeathEvent, which the proxy does support. The rest stay false rather than having the IDE offer redefinition or frame popping and then fail. - Breakpoint arming logs the resolved Class.method:line, and warns when a request maps to no method or to a line outside the method's line table. It used to be accepted in silence. - NetBeans gets "Build for iOS On-Device Debug" and "Start iOS Debug Proxy" project actions, the configuration the reporter had to build by hand; IntelliJ was missing the build half and gains a run config for it. The build goal forces the onDeviceDebug hint itself, so the properties-file editing that started this issue is not needed. Tested by 66 tests. The translator's table and the frame-push coverage are asserted on emitted C; the runtime's validation and scope rules are compiled and run on the host through cn1_debugger_objects.c, which is plain C for that reason. Each guard was checked against the pre-fix code rather than assumed -- reverting the validation makes four of them exit 139, which is the reported signal 11 reproduced. Not verified on a device: the native changes are checked by type-check against the iOS SDK and by the extracted policy unit under test, not by an on-device session. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both carried a truncated header that predates the copyright gate, so modifying them tripped it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![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: 3fe98d8fbc
ℹ️ 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".
Contributor
✅ 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: c23ad3f8a1
ℹ️ 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".
Contributor
|
Developer Guide build artifacts are available for download from this workflow run:
Developer Guide quality checks: |
The frame side-table is emitted during code generation and the symbol table the IDE reads is written after every class has been generated, with optimize() in between -- and a scope is resolved by walking the instruction list that pass rewrites. It preserves the label and line-number entries the walk depends on, so the two agree either way today; nothing states that, and them disagreeing would not fail loudly, it would just drop locals from the IDE's variables view. Snapshotting in computeRawMethodPlans, next to the other plans that must be read off the raw list, removes the dependence and costs one walk per method rather than one per method per consumer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Vale flagged five contractions and a wordiness in the sections this PR added, and LanguageTool the same 'whether or not'. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
handleGetLocals and handleGetStack both built their reply into a malloc'd buffer without checking it, while holding the suspended thread's mutex. A null there would crash the app, which is the failure mode this whole change is about, and the other handlers in the file already check. Both now reply empty and unlock. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Cloudflare Preview
|
Contributor
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
Tagged ints (P2, and the one that mattered). Integer.valueOf() returns (v << 1) | 1 on every 64-bit target, which is every shipping iOS build. Those references are deliberately odd, so the alignment guard in front of the header read discarded all of them -- boxed Integers in locals, fields, object arrays, invocation arguments and receivers would all have read as null. They are now recognised before any read is attempted, and report Integer's class; CMD_GET_OBJECT_FIELDS serves the value from the tag rather than computing a field address from a value. End-of-method locals (P2). javac closes a method-wide scope with a label that no line number follows. Mapping it to the last line seen made the exclusive end land on that line, hiding "this" and every parameter at exactly the breakpoint most likely to be set -- the return or the closing brace. Those labels now produce an open-ended scope. Stale suspend state (P2). The thread snapshot is only as fresh as the last refresh, and a thread is normally enumerated while running and stopped a moment later. Status and SuspendCount therefore kept reporting the thread the IDE had just stopped at as running. Suspend and resume events now update the snapshot. Phantom dead threads (P2). Event-derived ids accumulate for the session, and unioning them into every answer kept a row in the IDE for every worker that had ever hit a breakpoint. They are now a fallback for a device that cannot answer, not an addition to one that can, and an authoritative list prunes them. That distinction also corrected a test of mine that modelled an empty list as "cannot enumerate" when it actually means "nothing to show". Each is covered: the tagged-int paths and the misalignment case they reinterpret in DebuggerObjectValidationTest, the scope in OnDeviceDebugFrameTableTest (reproducing the pre-fix behaviour fails it), and both thread behaviours in JdwpThreadListTest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CN1_UNTAG_INT only exists when CN1_TAGGED_ACTIVE is set, so the value accessor broke -DCN1_DISABLE_TAGGED_INT -- a documented opt-out -- and any 32-bit target. Guarded; both configurations compile. 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: 40bdfb4ee0
ℹ️ 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".
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 149 screenshots: 149 matched. |
Collaborator
Author
|
Compared 149 screenshots: 149 matched. |
Both said On-Device-Debugging.adoc; the file is .asciidoc. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Same pre-gate truncation as the two debugger files. 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: 06ac1bd30b
ℹ️ 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".
Claiming canRequestVMDeathEvent while ignoring VM_DEATH in EventRequest.Set meant an IDE got a request id back that nothing would ever match: onVmDeath emitted only the spec's automatic event, id 0. A debugger waiting on its own id would wait for a session end that had already happened. Registered requests are now tracked and matched, in one composite with the automatic event, at the strongest suspend policy any of them asked for -- a weaker one would leave a debugger that asked to suspend running on. Cleared on EventRequest.Clear and on detach. The test client had to learn to split a composite into its events; it read only the first, which is why this looked correct at first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Suspend state was published after the event that announces it. The proxy can have a thread-list request in flight, and its reply is built from that flag, so the wire order could be BP_HIT followed by a snapshot taken while the thread still read as running -- putting the IDE back to showing the thread it had just stopped at as running. The flag is now set before the event goes out, and sampled under the mutex that publishes it, so any snapshot taken after the event sees the suspension. The debugger's own listener appeared in the thread list. It is a raw pthread, but servicing CMD_GET_STRING calls toNSString with getThreadLocalData(), which registers whatever thread asks into allThreads and never unregisters it. That left a permanent synthetic entry with no java.lang.Thread behind it -- and it only showed up once the IDE resolved a thread name, so it was a side effect of the feature this branch added. The listener now registers itself up front and enumeration skips it. 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: 4a654c366c
ℹ️ 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".
Racing VM thread-state init. My own previous fix -- registering the listener's ThreadLocalData at thread start so enumeration could skip it -- created this: getThreadLocalData() initialises threadIdKey, threadKeyCounter and the allThreads array with no synchronisation, and on a non-wait launch the listener spawns just as the main thread makes its first call. Competing TLS keys or duplicate thread ids were possible. The pointer is now captured lazily at the one place the listener really needs a Java context, servicing CMD_GET_STRING, by which time a proxy and an IDE are attached and the VM initialised long ago. No call is made any earlier than the code already made one. Accepting a class header as proof of liveness. A registered class word survives reclamation -- full-page BiBOP reset does not clear class words, and a legacy object reaches free() with its header intact -- so an objectID the IDE held across a resume passed validation and was read. Every reference handed to the proxy is now recorded, the set is dropped on every resume path, and an id arriving from the wire must be one issued since the last resume or it is refused. That bounds the reported window rather than proving liveness, and the code says so: objects shown to the IDE come from live frames, fields and arrays and so are reachable, but the concurrent collector can still run while a thread is parked. Closing it completely means rooting issued ids until the IDE disposes them, which belongs in the collector, not here. Worth weighing against the starting point -- before this branch, wire ids were dereferenced with no validation at all. 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: b9cfea00de
ℹ️ 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".
Both are consequences of the table I added for the liveness finding. A per-thread resume cleared the whole table. With two threads parked and the IDE resuming or stepping only one, ids already handed out for the other -- still stopped, still being inspected -- were invalidated, so its locals, fields and arrays started reporting unavailable. The set is now kept while any thread remains suspended and dropped when the last one wakes, which still denies ids to an app that has run on. Overflow was silent. Past the fixed 4096 entries the table stopped recording but callers kept sending those references, so every one after that was visible to the IDE and impossible to expand -- reachable by inspecting a single large object array. The table now grows, and recording reports failure so a reference that cannot be recorded is reported as null instead: one the IDE never sees beats one it sees and cannot use. Covered in DebuggerObjectValidationTest; pinning the old fixed table fails both new cases at exactly the 4096 boundary. 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: 6319dd476f
ℹ️ 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".
Count was parsed for its width and discarded, so a ClassPrepare request asking for one event got one per matching class and stayed armed afterwards. The count is retained now: the first N-1 matches are swallowed, the Nth is reported, and the request is deleted, which is what the spec asks for. Thread names were cached for the life of the thread. Pools rename their workers per task, so a name resolved on the first query mislabelled the thread from then on. The cache is dropped on every authoritative thread list rather than pruned to live ids, so a rename shows up on the next refresh; resolution costs a field read and a string read per thread the IDE actually asks about. Thread naming had no test of its resolution path at all -- it needs a device to answer three round trips, and the existing thread tests drive the proxy's listener callbacks directly. Added one that speaks the wire protocol through a stand-in device, covering the resolution, the rename, and the fallback when a thread has no java.lang.Thread yet. 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: 1be85b6223
ℹ️ 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".
Hints were read only out of codenameone_settings.properties. A hint passed with -D was accepted by Maven, echoed in its own property dump, and then ignored: the builder asked request.getArg(...) and got the file's value or the default. This is the complaint that opened issue #5333. The reporter set -Dcodename1.arg.ios.onDeviceDebug=true exactly as documented and got "No cn1-symbols.txt found ... the translator only emits it when -Dcodename1.arg.ios.onDeviceDebug=true is set". It was never user error. Reproduced here against the local ios-source path: with the hint on the command line the translator was still invoked with -Dcn1.onDeviceDebug=false, so no symbol table was emitted and the debug listener was not linked. It also means cn1:buildIosOnDeviceDebug could not do what it claims. That goal sets the hint as a property on a nested Maven invocation, which hits the same wall -- so the IDE actions this branch added, and the guide text saying the goal forces the hint for one build, were both wrong. Maven's user properties -- the values actually passed with -D or set by an invoking build, not the whole system property table -- are now overlaid on the settings file, so an unrelated JVM property cannot become a build hint. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Wait until suspend primitives are initialized
When two threads first call susForThread() concurrently—for example, a breakpoint lands while the listener handles its first thread-list request—the winner publishes g_susInit = 1 before initializing the mutexes and condition variables. The loser therefore returns immediately and can lock or signal a partially initialized sus_state, causing undefined behavior, crashes, or missed wakeups; use pthread_once or a distinct completed state that other callers wait for.
Store suspend state by the full thread ID
After the process has allocated more than 1024 cumulative threads, the monotonic thread IDs can alias here even though no more than 1024 threads are alive simultaneously—for example, a long-lived thread 1 and a later worker 1025 use the same slot. Suspending the worker then also makes thread 1 appear suspended and exposes the worker's tsd as thread 1's stack; resuming either ID can wake the wrong waiter or leave one parked indefinitely. The table needs to key and verify the complete ID rather than masking it directly.
ℹ️ 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".
Building the sample app for the iOS simulator failed to compile: the watchOS slice's generated classes call cn1_debugger_register_class from their constructors, and there it resolves against an older copy of cn1_debugger.h that predates the function. cn1_debugger_objects.c is not in that source set either, so it would not have linked even with a declaration. Generated code is compiled in more contexts than the iOS port's own headers reach, so a hook it calls cannot depend on them. Declared in cn1_globals.h, which every generated file includes, and given a weak no-op in cn1_globals.m alongside cn1_debugger_check and cn1_debugger_mark_issued_roots -- the same arrangement those already use for the same reason. A target that links the debugger runtime gets the real one. The test pins the contract for all three hooks rather than this instance alone, since the next one added would fail the same way. Found by building for the simulator. Nothing in the host tests reaches it: they compile the debugger unit against the working tree in isolation, never as the translator emits calls into it. 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: f6acda51d9
ℹ️ 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".
Building the sample app with on-device debugging on failed the watch target: cn1_debugger.m draws the "waiting for debugger" overlay with UIKit, which does not exist on watchOS, and the watch slice compiles the same file. So enabling the feature on any project that has a watch target did not build at all. The overlay is the only UIKit in the file. It is compiled out for watchOS, leaving no-op install and dismiss entry points -- the watch app has no debugger UI to show, while the rest of the runtime, the listener, the wire protocol and the object validation, is plain C and unaffected. Verified by syntax-checking the file against both SDKs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Preserve ThreadOnly on class-prepare requests
When a client adds a valid ThreadOnly modifier to a CLASS_PREPARE request, this branch consumes the thread ID without retaining it, and replayClassPrepare() subsequently emits every matching preparation with either lastSuspendedThread or dummy thread 1. Thread-filtered requests therefore receive events attributed to a thread they explicitly excluded; retain and apply the requested thread ID, or reject this unsupported modifier rather than silently broadening the request.
ℹ️ 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".
ParparVM thread ids are small integers and this proxy hands them to the IDE as thread ids unchanged. jdb asks ObjectReference.ReferenceType on them while drawing the Threads panel, and those arrived here as 1, 3, 5 rather than heap addresses. Forwarding them to the device as object pointers is wrong twice: they address nothing, and an odd one cannot be told apart from a tagged int -- so every odd-numbered thread rendered as a java.lang.Integer. Object queries now resolve a known thread id to the java.lang.Thread behind it. Heap references are aligned addresses far above the thread-id range, so nothing else is ambiguous. Found by attaching jdb to the app running in the simulator: the thread list came back as (java.lang.Integer)0x1 Thread-1 0x2 GC Thread (java.lang.Integer)0x3 EDT with the pattern following odd ids exactly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
CodenameOne/Ports/iOSPort/nativeSources/cn1_debugger.m
Lines 219 to 225 in dc9d939
Suppress SIGPIPE on the debugger socket
When the proxy disconnects while the debug build is still printing, the new stdout/stderr capture threads can reach this sendEvent() call with a stale connected descriptor. The debugger socket is not configured with SO_NOSIGPIPE, while the iOS runtime explicitly installs a SIGPIPE handler; therefore send() can raise a signal instead of returning the EPIPE that sendAll() expects, potentially crashing or throwing on the raw capture pthread. Configure the socket to suppress SIGPIPE, as the repository's normal iOS socket implementation does, before forwarding output.
ℹ️ 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".
A debugger resolves a frame against the line table it was handed for that frame's method and treats an index the table cannot describe as corruption rather than as missing detail. jdb raises InternalError: Location with invalid code index out of `where` and the session ends -- so a single frame the proxy described badly took down the whole stack view, not just its own row. Two ways the proxy produced such a frame: Method.LineTable answered "success, zero entries" for a method it had no lines for. That reads as an authoritative empty table, which makes every location in the method unresolvable. It now answers ABSENT_INFORMATION, which is what the spec reserves for this and what debuggers already handle -- the frame simply shows no line number. ThreadReference.Frames passed the device's line through unchanged. It is normally a line the table has, since ParparVM's code index is the source line, but not for a line the optimiser moved or for 0 from a frame that has not reached a tracked line. Frames now snap to the nearest tracked line at or before the reported one, naming the last line known to have started, and a frame with no line yet shows the method's first. Declining an unimplemented command is also no longer silent. A debugger reports "Unexpected JDWP Error: 100" without naming what it asked for, so neither side of the connection could tell which command was missing; the proxy now logs the command set and command once per pair. Found with jdb attached to an app in the simulator: a breakpoint in Form.show() stopped and printed three frames, including a lambda, before `where` threw on the fourth. 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: 11f7787933
ℹ️ 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".
Review findings, in the order they bite a session: Suspension-table init raced its own publication. ensureSusInit set the "initialised" flag before constructing the mutexes, so a caller that lost the race returned early and locked a slot that did not exist yet. It needs the IDE's first AllThreads to race a thread reaching its first breakpoint -- the opening seconds of a session. Now pthread_once. An invocation could arrive before the target parked and hang the session. suspendCurrent waited before testing for queued work, and a condition variable does not queue signals, so an invocation published in that window had already been signalled: the listener waited for a result nobody was running. The predicate is now tested before each wait. Invocation results were rooted after the target went inactive. A thunk returning or throwing a fresh object left the only reference to it in the result struct, which the collector does not scan; the ID handed over could already name reclaimed storage. Rooted on the target thread before it goes inactive. Descendant IDs died on every thread-list refresh. A refresh re-issues only top-level Thread objects, so a field the IDE had expanded off one carried no claim and was dropped -- while still reachable from a live thread. Claims now carry a generation and a parent link, and a refresh reconciles instead of clearing, keeping whatever hangs off a thread it still advertises. A thread that stopped while the thread list was in flight vanished from it. The device samples at request time, so the reply predates the stop, and rebuilding from it dropped the thread the IDE had just been told about. Suspensions and list requests are now ordered against each other: a suspension newer than the request survives the snapshot, an older one absent from it is still a thread that died. ClassMatch modifiers were combined disjunctively. JDWP ANDs them, so "com.example.*" plus "*Test" asks for the test classes in that package; we replayed everything matching either. A malformed thread-list frame could take the proxy down. count * 17 overflows to a negative for a large count, turning the bounds check into a pass, and the allocation that followed threw OutOfMemoryError. Checked by division now -- and the same defect was in the stack, locals and object-fields frames, which are fixed too. Found by running it against the simulator: ReferenceType.SignatureWithGeneric was unimplemented. jdb asks for it while caching a reference type, which it first does when rendering an array, so printing a frame holding one threw and took the rest of that locals listing with it. 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: 4f63a22fc2
ℹ️ 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".
A debugger may ask about a thread ID as an object -- jdb does it while drawing its thread panel -- so resolving such a query to the thread's java.lang.Thread was right, but recognising which IDs were threads by their small numeric range was not. Heap references are aligned and therefore even, and the earlier reasoning stopped there. Tagged ints are not references at all: ParparVM encodes int v as (v << 1) | 1, so they are odd and occupy exactly the small values thread IDs use. Integer 0 encodes to 1, which was also thread 1, and expanding that boxed Integer in the IDE read the thread object instead. Thread IDs now travel tagged into the top of the ID space and shifted even: above any address the process can hold, and never odd, so no tagged int can equal one. The mapping is applied at each boundary -- AllThreads, the thread group's children, breakpoint and step events, every ThreadReference and StackFrame command, and the thread argument of both invocation forms. Separately, a VM_DEATH request with a Count above one now stays silent. A VM dies once, so such a request is asking never to be told; the count was parsed and then dropped, and the death reported anyway. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The device reports an array by naming its component class and setting a flag, so an array and its component type shared one reference-type ID. The type the IDE had been told was an array then answered with the component's own signature -- one with no leading '['. A debugger reads the component type back out by removing the first character, so it does not detect the missing bracket, it just eats the 'L' and parses what is left: printing an array local ended in IllegalArgumentException: Invalid JNI signature character 'j' the 'j' being the start of "java/lang/...". Array reference types are now tagged into their own ID range carrying the component class, and report the component's signature with a '[' in front. From review: A hold outlived the session that set it. An IDE detaching between HoldEvents and ReleaseEvents left the flag and the queue in place, so the next attach queued its own VM_START instead of sending it and a later release delivered events carrying the previous IDE's request ids. Cleared in teardown. A descendant reached through two threads kept only the most recently recorded parent. When that thread went away the id went with it, while the IDE was still displaying the same id under the thread that remained. Parents are a set now, like the owners, with the same overflow rule. A thread-list request that timed out could have its reply applied to the next request. The reply describes the list as of the request it answers, so using it later judged suspensions against the wrong epoch and rebuilt from a list older than the one asked for. Abandoned replies are counted and discarded on arrival. 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: 3bebcc3963
ℹ️ 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 stack view no longer stops at a frame whose method carries no line information, which is worth stating because such frames are common -- synthetic lambda bodies and Thread.runImpl both appear in an ordinary EDT stack. The Error 100 troubleshooting entry promised the proxy logs the command it declined. It does now, so the entry shows what that looks like. 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: e4ac02cc22
ℹ️ 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".
Two events name a thread the proxy has to invent rather than one it learned from the device, and both still wrote it raw: VM_START, and a CLASS_PREPARE replayed to an IDE that registered the request. Every ThreadReference handler now decodes what it is given, so a raw value came back as a different device thread -- raw 1 as thread 0 -- and a small raw value is exactly what a tagged int looks like, which is the collision the namespace exists to prevent. Also stop inferring "the device answered" from "the request is no longer outstanding". Those were the same thing until a timeout began clearing the pending flag; since then a device that never answers, or a send that threw, read as a successful refresh. Receipt is tracked separately now. That second change is not observable today: the fallback it guards adds threads from knownThreads, which only ever gains an id alongside deviceThreads and is pruned to a subset of it on every snapshot, so the loop cannot contribute one. The accompanying test says so rather than implying it covers a live defect -- it passes with or without the fix. The flag is still worth correcting, because it now means what its name says and is the right guard if that invariant changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The runtime gives every array its own class, made up as needed for the element type, so those classes carry no entry in the symbol table -- that table describes what the translator compiled. Reporting an array's own class ID left the proxy with nothing to resolve, so it fell back to Object and every array in the IDE read as Object[] whatever it held. A single is-array flag could not have said otherwise: it has no room for the depth, so int[][] was unrepresentable either way. The device now walks arrayType down to the first class that is not itself an array -- the one the symbol table knows -- counting the hops, and sends that component with the depth alongside the flag. The proxy carries the depth in the reference-type ID and repeats the '[' that many times. The walk is bounded rather than open, since the chain is read out of the target's memory and a corrupt link must not spin. The reply grows a sixth byte; proxies reading four or five are unaffected, and a device that sends neither is read as one dimension, which is all such a device could have meant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two describe the same object-class reply, and reading them in separate synchronized blocks let a second object's answer land in between -- pairing one object's array flag with another's depth. 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: 7937a1bdc3
ℹ️ 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".
Suspension state was indexed by a hash of the thread ID into a fixed table, and the runtime hands out IDs from a counter that only ever climbs. Once an app has created as many threads as the table has slots, two live threads share one: either thread's suspend flag and frame pointer overwrote the other's, so a stack or locals request could answer about the wrong thread and resuming one woke the other. The slot now records which thread owns it and is found by probing, so a collision moves the newcomer along instead of overwriting. A table with nothing free reclaims a slot that is idle -- one still in its initial state carries nothing, and its owner claims a fresh slot next time it asks -- and failing even that returns the hashed slot rather than nothing, since every caller dereferences the result. The array depth was counted rather than read. The translator points arrayType straight at the scalar for every depth and puts the real one in dimensions, so following the link and counting called int[][] one-dimensional and the IDE showed it as int[]. The depth now comes from the class; a chain is still walked in case a layout ever nests. A resume dropped descendants of live threads. An object reached through a Thread object carries no owner of its own and a generation stamp from before the last refresh, so judged on its own account it looks unclaimed. The refresh knew to follow the parent links; the resume rebuild did not, and dropped the ID the moment any thread ran again -- while the IDE was still showing it under a thread the current list advertises. Both paths share the reachability pass now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Making the slot table probe for its owner put a process-wide lock and a scan of the table on the path cn1_debugger_check takes -- which runs at every source line of every method, and is the whole cost the debugger adds to a running app. Correct, and far too expensive to reach a device. The thread's own slot is remembered in thread-local storage, so the common case is a pointer compare and no lock at all. That holds only because a claimed slot never changes hands, so slots are no longer reclaimed: a full table falls back to the shared hashed slot, which is where every thread started before any of this. Reclaiming would let a slot change owner underneath the thread holding it -- the very defect being fixed -- and would cost the fast path. Only a caller running on the thread it is asking about may use the cache; the listener asks about other threads and takes the slow path, which is not hot. Verified on the simulator with the rebuilt binary: breakpoint hit, locals read in the stopped frame, app still running. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
shai-almog
added a commit
that referenced
this pull request
Aug 8, 2026
The packaging job started failing on master, and the failure was not a flake: the last green run mentions GoogleSignIn zero times and the first red one 517. #5536 fixed build-hint plumbing, so ios.gplus.clientId -- which ios-packaging.yml has passed for months -- finally reached IPhoneBuilder for the first time and switched on the pod. The coverage that workflow describes, compiling GoogleConnectImpl.m, had never actually run, and the moment it did it took the build down: GoogleSignIn 5.x vendors a binary framework whose arm64 slice is device-only, so it cannot link into an arm64 simulator build, which is every simulator build on an Apple Silicon machine. 7.1 ships as a source pod, so there is no prebuilt slice to mismatch. Its API is different in the three places this port touches: the client id rides on a GIDConfiguration, sign-in reports to a completion block rather than a delegate, and the token hangs off the user rather than an authentication object. The GIDSignInDelegate conformance and its two callbacks are gone with it, and the pre-configuration in initGoogleConnect goes with them -- there is nothing to set up ahead of a call that carries everything. The GPPSignIn branches are untouched. Verified by building the sample the way the packaging job does, with the same dependency arguments: the workspace compiles and links for the arm64 simulator with the pod in it. What that does not cover is the sign-in flow itself, which needs real Google credentials and is no better covered today than before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the outstanding items on #5333.
The crash
A breakpoint in an event listener took the app down with
We had a signal 11. Three independent wild-pointer paths converged onhandleGetLocals, which is what NetBeans calls the moment a breakpoint hits.1. The locals side-table described the wrong storage. It listed one row per declared local but only one address per JVM slot, picked last-writer-wins out of a
HashSet. A slot reused by anintand a reference in disjoint scopes — routine in any listener — therefore had a reference row addressing a four-byteJAVA_INT:There is now one address per row, so a row's type code and the storage it points at cannot disagree. Rows are sorted rather than hash-ordered, so the emitted table also stops varying between builds of the same input — which is why this reproduced on some breakpoints and not others.
2. Frames inherited a dangling locals pointer.
callStackFrameInfo/callStackLocalsAddresseswere written only by methods carrying a side-table, and never cleared. A native, eliminated or barebone frame inherited whatever the previous occupant of that call depth left — including an address array pointing into a C frame that had already returned.CN1_DEBUG_FRAME_ENTERclears both before every push and compiles away in release builds.3. Nothing validated a reference before dereferencing it. Every reference the debugger handles is untrusted: the IDE echoes back objectIDs it was handed earlier, and a local slot holds whatever the branch that never ran left there.
cn1_debugger_objects.cresolves a candidate through an exactclassId -> clazzregistry, reading it viavm_read_overwriteso an unmapped page returns an error instead of a signal. Every deref site goes through it — locals, object class/fields/string, invoke receiver and args, array length/values, and references handed back to the IDE.The rest of the report
CMD_GET_THREADSwas a device stub replying empty and the proxy never sent it, soAllThreadscould only report threads that had already hit a breakpoint. It now enumerates ParparVM's thread registry. Suspension is tracked per thread rather than by a single "last suspended" field, andThread.Nameresolves the real name off thejava.lang.Threadinstance, so the panel showsEDTandmain.ClassPrepare(event kind 8) was accepted and never fired, so IntelliJ and NetBeans left every deferred breakpoint unarmed — the "breakpoint in a listener never stops" half of the report. Matching classes are replayed at registration, which is complete for ParparVM since the whole class set is linked in before a debugger can attach.-glocal being shown asvN: the placeholder a store opcode synthesises no longer keeps the class file's own entry out of the set that holds them.CapabilitiesNewreportscanRequestVMDeathEvent, which the proxy does support. The rest stay false rather than having the IDE offer redefinition or frame popping and then fail.Class.method:line, and warns when a request maps to no method or to a line outside the method's line table. It used to be accepted in silence, which is why this thread ran as long as it did.onDeviceDebughint itself, so the properties-file editing that started this issue is not needed.Tests
66 tests, in the places the behaviour actually lives:
OnDeviceDebugFrameTableTest(10)DebugFramePushCoverageTest(3)DebuggerObjectValidationTest(8)DebuggerLocalScopeTest(5)JdwpThreadListTest(6) /DeviceConnectionThreadListTest(5)JdwpClassPrepareTest(7)JdwpVariableScopeTest(6)Cn1AppArchetypeCertificateWizardTest(+1)Each guard was checked against the pre-fix code rather than assumed. Reverting the per-row table fails four frame-table tests with exactly the per-slot symptom; removing one
CN1_DEBUG_FRAME_ENTERnames the file and line; reverting the validation to a raw deref fails five tests, four of them with exit 139 — the reportedsignal 11, reproduced and then prevented.cn1_debugger_objects.cis plain C rather than part of the Objective-C file precisely so these rules can be compiled and run on a host. The tests are macOS-only, sincevm_read_overwriteis Darwin-only and iOS debugging is a macOS activity regardless.Verification limits
cn1_debugger.mandcn1_debugger_objects.ctype-check against the real iOS SDK (xcrun --sdk iphonesimulator clang -fsyntax-only, only three pre-existing deprecation warnings), the frame macro compiles in both build modes, and SpotBugs is at zero onByteCodeTranslator.This has not been through an on-device debugging session. The device-side changes are verified by type-check and by the extracted policy unit under test, not by running against a tethered iPhone — worth doing before telling the reporter it's fixed.
Two things are deliberately left, and documented rather than papered over: two locals of the same type sharing a slot still collapse to one entry (reuse across different types, the case that caused the crash, is fully distinguished), and a local compiled without
-ghas no scope so it is listed for the whole method.🤖 Generated with Claude Code