Fix JAB freeze when navigating in JetBrains IDEs#19934
Fix JAB freeze when navigating in JetBrains IDEs#19934christopherpross wants to merge 5 commits intonvaccess:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes NVDA freezes triggered by rapid Java Access Bridge (JAB) event storms in JetBrains IDEs by moving expensive property-change processing off the JAB callback thread, reducing cross-process parent traversal costs, and ensuring callback deregistration symmetry.
Changes:
- Queue
name/description/valueproperty-change handling viainternalQueueFunctionto avoid heavy work on the JAB callback thread. - Optimize
findOverlayClassestable-cell detection by avoiding recursive parent NVDAObject creation (lightweight immediate-parent role check). - Deregister all registered JAB callbacks (including the previously-missed property change callbacks) in
terminate().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| user_docs/en/changes.md | Adds a user-facing changelog entry for the JetBrains freeze fix. |
| source/NVDAObjects/JAB/init.py | Avoids recursive parent traversal when determining whether to apply TableCell overlays. |
| source/JABHandler.py | Queues property-change callbacks and improves callback cleanup; adds bounded-queue overflow handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ss#16741) - Three event callbacks ran directly in the JVM callback thread, causing cross-process SendMessage deadlocks in JetBrains IDEs - Now queued to the main thread via internalQueueFunction, matching the existing pattern used by event_stateChange and event_caret
…16741) - Table cell detection created a full parent NVDAObject, recursively triggering findOverlayClasses up the entire UI hierarchy - New _hasTableParent() checks the parent role via lightweight context lookup first, only creating the full object when the parent is a table
- Name, description, and value change callbacks were never deregistered, leaving dangling function pointers after shutdown - Now matches the full set of callbacks registered in initialize()
When internal_hasFocus returned False, the newDescendant JOBJECT64 handle was never released since it was neither queued for processing nor explicitly freed. Add an else branch to release the handle.
christopherpross
force-pushed
the
fix/jab-queue-direct-callbacks
branch
from
7ee6989 to
e2638e8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pass | ||
| try: | ||
| internalFunctionQueue.put_nowait((func, args, kwargs)) | ||
| except queue.Full: |
There was a problem hiding this comment.
In the overflow path, if put_nowait still raises queue.Full after evicting one item, the function returns without releasing the JOBJECT64 handle owned by the new (dropped) event. Since queued handlers generally treat args[1] as the owned accContext reference, this can leak JNI refs under contention/multiple producer threads. Consider releasing the dropped event’s handle on this failure path (excluding enterJavaWindow_helper), similar to the eviction cleanup.
| except queue.Full: | |
| except queue.Full: | |
| # The new event is being dropped; release its owned JOBJECT64 accContext | |
| # handle to avoid leaking JNI refs. | |
| if func is not enterJavaWindow_helper and len(args) >= 2: | |
| bridgeDll.releaseJavaObject(args[0], args[1]) |
| def internalQueueFunction(func, *args, **kwargs): | ||
| internalFunctionQueue.put_nowait((func, args, kwargs)) | ||
| """Queue a function for execution on the main thread. | ||
|
|
||
| When the queue is full, the oldest event is evicted to make room. | ||
| JOBJECT64 handles owned by evicted events are released to avoid leaks. | ||
|
|
||
| .. note:: | ||
| All queued handler functions except :func:`enterJavaWindow_helper` | ||
| follow the convention ``(vmID, accContext, ...)``, where ``args[1]`` | ||
| is a JOBJECT64 handle. If a new handler with a different signature | ||
| is added, the eviction cleanup below must be updated. | ||
| """ | ||
| try: | ||
| internalFunctionQueue.put_nowait((func, args, kwargs)) | ||
| except queue.Full: | ||
| try: | ||
| evictedFunc, evictedArgs, _evictedKwargs = internalFunctionQueue.get_nowait() | ||
| # Release the JOBJECT64 accContext handle from the evicted event. | ||
| # All handlers except enterJavaWindow_helper have args[1] as accContext. | ||
| if evictedFunc is not enterJavaWindow_helper and len(evictedArgs) >= 2: | ||
| bridgeDll.releaseJavaObject(evictedArgs[0], evictedArgs[1]) | ||
| except queue.Empty: | ||
| pass | ||
| try: | ||
| internalFunctionQueue.put_nowait((func, args, kwargs)) | ||
| except queue.Full: | ||
| log.debugWarning("JAB internal function queue full, failed to re-queue after eviction") | ||
| return | ||
| log.debugWarning("JAB internal function queue full, evicted oldest event") | ||
| core.requestPump() |
There was a problem hiding this comment.
The new queue overflow/eviction behavior (including handle release for evicted/dropped events) isn’t covered by unit tests. Since this is subtle resource-management logic and the repo already has unit tests for Java Access Bridge behavior, adding a focused test around internalQueueFunction’s full-queue behavior would help prevent regressions.
| def _hasTableParent(self) -> bool: | ||
| """Lightweight check if the immediate parent is a Table. | ||
|
|
||
| On the fast path (parent already cached), returns immediately | ||
| without any bridge calls. On the first call, checks the parent's | ||
| role via a lightweight bridge call and only creates a full NVDAObject | ||
| when the role is "table" (which involves additional bridge calls). | ||
| """ | ||
| if hasattr(self, "_parent"): | ||
| parent = self._parent | ||
| return parent is not None and isinstance(parent, Table) and parent._jabTableInfo |
There was a problem hiding this comment.
The _hasTableParent docstring claims the cached-parent “fast path” returns without any bridge calls, but the check still evaluates parent._jabTableInfo, which can trigger getAccessibleTableInfo() if not already cached. Consider adjusting the wording to reflect that it avoids parent traversal bridge calls, but may still query table info.
Link to issue number:
Closes #16741
Summary of the issue:
NVDA freezes for 10-15 seconds when navigating in JetBrains IDEs. Holding arrow keys in the editor causes the watchdog to trigger and eventually force recovery.
Description of user facing changes:
Navigation in JetBrains IDEs no longer causes NVDA to freeze. This may also help with other rapid interaction patterns like scrolling with the mouse and clicking on code repeatedly.
Description of developer facing changes:
None.
Description of development approach:
Two compounding issues in JABHandler caused the freeze:
Synchronous property change callbacks:
event_nameChange,event_descriptionChange, andevent_valueChangewere registered directly as JAB callbacks, doing heavy work (NVDAObject creation, bridge queries, event queuing) on the callback thread. This blocked the main thread duringwx.Yield(). The fix wraps them ininternalQueueFunction, matching the pattern already used byevent_focusGained,event_stateChange, andevent_caretChange.Recursive parent traversal in
findOverlayClasses: The table-cell checkself.parent and isinstance(self.parent, Table)called_get_parent(), which created a full NVDAObject for the parent, recursively triggeringfindOverlayClassesup the entire component tree. Each level makes at least 2 cross-process bridge calls (getAccessibleParentFromContext+getAccessibleContextInfo). In JetBrains Rider, the editor text field sits 19 levels deep in the Java component hierarchy, so a singlefindOverlayClassescall caused 19 recursive NVDAObject instantiations with ~38 bridge calls. The fix introduces_hasTableParent()which checks the immediate parent's role via a lightweight bridge call and only creates a full NVDAObject when the role is "table" (which involves additional bridge calls).Incomplete callback cleanup: 4 of 7 registered JAB callbacks were deregistered in
terminate(), the 3 property change callbacks (name, description, value) were missing. All 7 are now symmetrically deregistered.Evict-oldest queue overflow strategy: When the internal event queue (1000 items) is full, the oldest event is evicted to make room for the newest. JOBJECT64 handles owned by evicted events are released to prevent JNI reference leaks. This also fixes a pre-existing leak in
internal_event_activeDescendantChangewherenewDescendantwas never released when the source object did not have focus.This PR was developed with AI assistance (Claude Code) with human review and manual testing.
Testing strategy:
Also tested alt-tabbing in/out of the IDE, rapid Ctrl+Home/End navigation, and navigating tables in Rider's settings dialog with no regressions.
Known issues with pull request:
Moving the three callbacks onto the internal queue initially caused
queue.Fullexceptions under rapid key repeat, producing error sounds. Catching the exception and dropping the newest event fixed the errors, but the final caret/name event was silently discarded, so the user never heard the line they landed on. Adding handle cleanup for dropped events (from Copilot AI review feedback) introduced additional bridge calls in the overflow path, which made the overflow worse. Making the queue unbounded was considered but rejected because a chatty Java app could grow the queue without limit. Switching to an evict-oldest strategy solved the announcement problem, but initially leaked the JOBJECT64 handles of evicted events. The current solution evicts the oldest event and releases its handle using a positional convention (args[0]= vmID,args[1]= accContext for all handlers exceptenterJavaWindow_helper). This convention is documented in theinternalQueueFunctiondocstring but not enforced at the type level. Open to suggestions if anyone sees a cleaner approach.Code Review Checklist: