fix: guard a11y NPE via uncaught-exception handler path on macOS#645
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughA11yCrashGuard centralizes Compose accessibility NPE detection and suppresses matching NPEs on macOS via two guarded paths: the AWT EventQueue (FilteringEventQueue) and the JVM uncaught-exception handler, with per-path one-time logging and an extracted isComposeA11yNpe classifier. ChangesmacOS Compose A11y NPE Suppression with Dual Propagation Paths
Sequence Diagram(s)sequenceDiagram
participant Installer as A11yCrashGuard.install()
participant SysEQ as System EventQueue
participant FEQ as FilteringEventQueue
participant Uncaught as JVM UncaughtExceptionHandler
participant Original as PreviousHandler
rect rgba(100,150,200,0.5)
Installer->>FEQ: set as system EventQueue
Installer->>Uncaught: wrap default handler (save previous)
Uncaught->>Original: delegate non-matching exceptions
end
rect rgba(150,100,200,0.5)
SysEQ->>FEQ: dispatchEvent(Event)
FEQ->>FEQ: isComposeA11yNpe(Throwable)?
alt match
FEQ-->>FEQ: suppress NPE, log once (warnedEdt)
else no match
FEQ->>SysEQ: rethrow NPE
end
end
rect rgba(200,150,100,0.5)
Uncaught->>Uncaught: onUncaught(Throwable)
Uncaught->>Uncaught: isComposeA11yNpe(Throwable)?
alt match
Uncaught-->>Uncaught: suppress NPE, log once (warnedUncaught)
else no match
Uncaught->>Original: forward throwable
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR extends
Confidence Score: 5/5Safe to merge — the change is a narrowly scoped exception filter that explicitly forwards all non-a11y exceptions and includes a documented fallback for edge cases. All three concerns from the prior review round have been resolved: separate per-path flags, a non-null fallback with documented installation-order contract, and a tighter stack fingerprint. The single changed file is a targeted workaround with no new business logic, and the installation order in DesktopApp.kt (CrashReporter first, A11yCrashGuard second) is correct and stable. No files require special attention. DesktopApp.kt preserves the required installation order and was not changed. Important Files Changed
Sequence DiagramsequenceDiagram
participant Compose as Compose a11y loop
participant Coroutine as BaseContinuationImpl
participant EDT as AWT EventQueue
participant FEQ as FilteringEventQueue
participant UCH as UncaughtExceptionHandler (A11yCrashGuard)
participant CR as CrashReporter handler
Note over Compose: accessibleParentOf NPE
alt EDT path (Path 1)
Compose->>EDT: NPE propagates up dispatch chain
EDT->>FEQ: dispatchEvent(event) throws NPE
FEQ->>FEQ: isComposeA11yNpe? true
FEQ-->>FEQ: suppress + log warnedEdt (once)
else Coroutine-failure path (Path 2)
Compose->>Coroutine: resumeWith catches NPE
Coroutine->>UCH: handleCoroutineException to defaultUncaughtExceptionHandler
UCH->>UCH: isComposeA11yNpe? true
UCH-->>UCH: suppress + log warnedUncaught (once)
end
Note over UCH,CR: Non-a11y exceptions forwarded to CrashReporter unchanged
Reviews (3): Last reviewed commit: "fix: address Greptile review — split war..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…, guard null handler
|
Addressed all three P2 concerns in the follow-up commit (f937a70): Shared Implicit ordering dependency — added a comment on
|
|
@greptile-apps re-review |
|
@nitheeshdr Thanks for contribution 🩵 |
2 participants
What
Extends
A11yCrashGuardto cover a second propagation path for the Compose MP 1.10.x accessibility NPE on macOS.The existing
FilteringEventQueueonly intercepts the NPE when it escapesDispatchedTask.runand propagates up through the AWT event queue chain. WhenBaseContinuationImpl.resumeWithcatches the NPE first (which happens depending on the coroutine call depth), the failure is routed throughhandleCoroutineExceptiondirectly to the default uncaught-exception handler — bypassingFilteringEventQueueentirely.CrashReporterthen writes a crash dump and the a11y sync loop coroutine dies, leaving the app unresponsive.Changes:
warnedandisComposeA11yNpelifted toA11yCrashGuardobject so both paths share theminstall()wraps the existing uncaught-exception handler (installed byCrashReporter) with an a11y NPE filter; non-a11y exceptions are forwarded to the previous handler unchangedisComposeA11yNpeaddssun.lwawt.macosx.CAccessibleas a secondary stack-frame fingerprint (it appears in every reported trace for this bug)Why
Users on macOS 15 running GitHub Store v1.8.2 see the app crash when navigating from a search result to the details screen. The stack trace is the same Compose MP
SemanticsOwnerAccessibility.accessibleParentOfNPE that #330 addressed, but in this trigger path the coroutine machinery intercepts the exception before theFilteringEventQueuesees it.How to test
Happ, select macOS filter → click more details on a result.crash-*.login~/Library/Logs/GitHub-Store/.session.logshows one[A11yCrashGuard] Suppressed Compose a11y NPE via uncaught-exception pathline and no crash dump is written.Fixes #640
Summary by CodeRabbit
#330and#640) and expanded notes on behavior and trade-offs.