New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NVDA crashes Java Swing application when transferring focus between JTable cells. #6992

Closed
legle opened this Issue Mar 20, 2017 · 14 comments

Comments

Projects
None yet
4 participants
@legle

legle commented Mar 20, 2017

In a java Swing application that uses a JTable component with two columns large enough to activate the HorizontalScrollBar, transferring the focus between these cells causes a crash.

To reproduce the bug:

  1. Given that there’s a Java Swing application with a JTable component with at least two columns and cells with a width large enough to active the HorizontalScrollBar (get reference application in https://www.dropbox.com/s/xc8oy5m4e0m476a/NVDAJTableProblem.jar?dl=1).
  2. When transferring the focus between the cells there’s a crash in the application and NVDA stays freezed for a few seconds.

Additionally, notice that there’s NO BUG if the cells are not large enough to activate the HorizontalScrollBar.

Here’s the configurations:
NVDA 2016.2 or higher;
Windows 10 X64 (Noticed the same problem in Windows 7);
Language: PT-BR;
Java: JDK 1.8.0_121;
Accessbridge: 2.0.1;

Java and NVDA logs at Logs.zip

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Mar 20, 2017

Collaborator
Collaborator

leonardder commented Mar 20, 2017

josephsl added a commit to josephsl/nvda that referenced this issue Mar 21, 2017

Fix errors or extraneous spaces when retrieving UIA keyboard shortcut…
…s. (PR nvaccess#6992)

- NVDA will no longer fail to navigate to or report certain (UIA) controls where a keyboard shortcut is not defined. (nvaccess#6779)
- Two empty spaces are no longer added in keyboard shortcut information for certain (UIA) controls. (nvaccess#6790)
@feerrenrut

This comment has been minimized.

Show comment
Hide comment
@feerrenrut

feerrenrut Mar 28, 2017

Contributor

@josephsl are you working on this issue? or is the reference in the commit (josephsl@f55a3f5) a mistake? If you are I will assign it to you.

Either way. This issue seems serious enough that we should investigate the cause of it. Assigning priority 2.

Contributor

feerrenrut commented Mar 28, 2017

@josephsl are you working on this issue? or is the reference in the commit (josephsl@f55a3f5) a mistake? If you are I will assign it to you.

Either way. This issue seems serious enough that we should investigate the cause of it. Assigning priority 2.

@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Apr 5, 2017

Contributor

I suspect this is a bug in Java/the Java Access Bridge, in which case there's probably nothing we can do. We access Java apps via the Java Access Bridge out-of-process. Because our code isn't running in the Java process, it cannot be responsible for the crash. Either way, the Java process needs to be debugged so we know for sure.

It'd also be good to know exactly what JAB call from NVDA triggers the crash. If it is indeed related to headers as #6992 (comment) suggests, I'd suggest starting with _get_columnHeaderText and _get_rowHeaderText in NVDAObjects.JAB.TableCell.

Contributor

jcsteh commented Apr 5, 2017

I suspect this is a bug in Java/the Java Access Bridge, in which case there's probably nothing we can do. We access Java apps via the Java Access Bridge out-of-process. Because our code isn't running in the Java process, it cannot be responsible for the crash. Either way, the Java process needs to be debugged so we know for sure.

It'd also be good to know exactly what JAB call from NVDA triggers the crash. If it is indeed related to headers as #6992 (comment) suggests, I'd suggest starting with _get_columnHeaderText and _get_rowHeaderText in NVDAObjects.JAB.TableCell.

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Apr 11, 2017

Collaborator

Also reproduced with SQuirreL SQL Client

Collaborator

leonardder commented Apr 11, 2017

Also reproduced with SQuirreL SQL Client

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Apr 12, 2017

Collaborator

Here is an additional log file which contains information about the freezes I'm having with the SQuirreL SQL Client. Also, these freezes don't occur when table row/column header reporting is off.

Collaborator

leonardder commented Apr 12, 2017

Here is an additional log file which contains information about the freezes I'm having with the SQuirreL SQL Client. Also, these freezes don't occur when table row/column header reporting is off.

@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Apr 13, 2017

Contributor

These crashes/freezes occur at two separate places as far as I can see, though both related to table column header fetching. They may or may not be related; I'm not sure.

In @legle's log, NVDA tries to fetch the cell for a column header. When NVDA calls JAB's getAccessibleTableCellInfo function, it returns false. However, I suspect that by this time, the app has already crashed.

In @leonardder's log, NVDA tries to fetch the table of column header cells. JAB's getAccessibleTableColumnHeader function returns the header table info. NVDA needs an HWND for any object it creates, so it tries to get this for the header table. It calls getTopLevelObject on the header table and then tries to call getHWNDFromAccessibleContext on that top level object. That is where it gets stuck; JAB blocks forever. No idea why yet.

I'm trying to run @legle's test case, but I just get an exception at the moment. I'm upgrading Java at the moment to see if this helps.

Contributor

jcsteh commented Apr 13, 2017

These crashes/freezes occur at two separate places as far as I can see, though both related to table column header fetching. They may or may not be related; I'm not sure.

In @legle's log, NVDA tries to fetch the cell for a column header. When NVDA calls JAB's getAccessibleTableCellInfo function, it returns false. However, I suspect that by this time, the app has already crashed.

In @leonardder's log, NVDA tries to fetch the table of column header cells. JAB's getAccessibleTableColumnHeader function returns the header table info. NVDA needs an HWND for any object it creates, so it tries to get this for the header table. It calls getTopLevelObject on the header table and then tries to call getHWNDFromAccessibleContext on that top level object. That is where it gets stuck; JAB blocks forever. No idea why yet.

I'm trying to run @legle's test case, but I just get an exception at the moment. I'm upgrading Java at the moment to see if this helps.

@jcsteh jcsteh self-assigned this Apr 13, 2017

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Apr 13, 2017

Collaborator

You might have more luck with SQuirrel. You need to download and install it, than throw the MariaDB connector in its lib directory to have access to MariaDB or MySQL databases, in the hope that you can contact any from your local pc. You will also have to change the layout of the application from the tabbed to the multi document interface to increase accessibility. If you connect to a database and do a select statement, the output will be in one of those crashing tables.

Collaborator

leonardder commented Apr 13, 2017

You might have more luck with SQuirrel. You need to download and install it, than throw the MariaDB connector in its lib directory to have access to MariaDB or MySQL databases, in the hope that you can contact any from your local pc. You will also have to change the layout of the application from the tabbed to the multi document interface to increase accessibility. If you connect to a database and do a select statement, the output will be in one of those crashing tables.

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder May 5, 2017

Collaborator

I Just tested @legle's application with both JAWS and SuperNova, and with those screen readers, all works great. This makes me think NVDA is responsible for the problem here.

Collaborator

leonardder commented May 5, 2017

I Just tested @legle's application with both JAWS and SuperNova, and with those screen readers, all works great. This makes me think NVDA is responsible for the problem here.

@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh May 5, 2017

Contributor
Contributor

jcsteh commented May 5, 2017

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder May 5, 2017

Collaborator

I just tested the application on a Windows 7 x86 vm with the newest JVM, which installs the legacy access bridge. IN this vm, all worked fine, no crashes at all.

Collaborator

leonardder commented May 5, 2017

I just tested the application on a Windows 7 x86 vm with the newest JVM, which installs the legacy access bridge. IN this vm, all worked fine, no crashes at all.

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder May 5, 2017

Collaborator

All right, I found a work around for this bug and am working on a pull request.

So, let's first give a technical background

  1. Nvda tries to retrieve the column header text from a table cell object, and thus _get_columnHeaderText is called on the object
  2. then, for the table's jabContext object, getAccessibleTableColumnHeader is called
  3. The problem is at JABHandler.py line 535. NVDA creates a new JABContext object, based on the vm id. Since no hwnd is provided, getWindowHandleFromAccContext is called to retrieve the hwnd, and that's where the process fails. To be specific, the application crashes when getTopLevelObject is called.

So, what I like to have confirmed is that we can safely assume the hwnd is the same across the whole table. If so, we can provide the hwnd upon JABContext creation, which will avoid the fatal getWindowHandleFromAccContext call.

There are several possibilities here. In JABHandler line 535: we can just add hwnd=self.hwnd to the JABContext constructor. However, if we can assume that the hwnd is equal, we can just pass the hwnd of the current context to every new JABContext constructor which is related to table information. This includes the following places:

  • getAccessibleTableInfo
  • getAccessibleTableCellInfo
  • getAccessibleTableRowHeader
  • getAccessibleTableRowDescription
  • getAccessibleTableColumnHeader
  • getAccessibleTableColumnDescription

Update: Oops, made some mistakes in this comment which are now fixed

Collaborator

leonardder commented May 5, 2017

All right, I found a work around for this bug and am working on a pull request.

So, let's first give a technical background

  1. Nvda tries to retrieve the column header text from a table cell object, and thus _get_columnHeaderText is called on the object
  2. then, for the table's jabContext object, getAccessibleTableColumnHeader is called
  3. The problem is at JABHandler.py line 535. NVDA creates a new JABContext object, based on the vm id. Since no hwnd is provided, getWindowHandleFromAccContext is called to retrieve the hwnd, and that's where the process fails. To be specific, the application crashes when getTopLevelObject is called.

So, what I like to have confirmed is that we can safely assume the hwnd is the same across the whole table. If so, we can provide the hwnd upon JABContext creation, which will avoid the fatal getWindowHandleFromAccContext call.

There are several possibilities here. In JABHandler line 535: we can just add hwnd=self.hwnd to the JABContext constructor. However, if we can assume that the hwnd is equal, we can just pass the hwnd of the current context to every new JABContext constructor which is related to table information. This includes the following places:

  • getAccessibleTableInfo
  • getAccessibleTableCellInfo
  • getAccessibleTableRowHeader
  • getAccessibleTableRowDescription
  • getAccessibleTableColumnHeader
  • getAccessibleTableColumnDescription

Update: Oops, made some mistakes in this comment which are now fixed

leonardder added a commit to BabbageCom/nvda that referenced this issue May 5, 2017

When retrieving table cell, row and column information from a JABHand…
…ler.JABContext object, the window handle of the current object is now provided when creating new JABContext objects. This avoids the need to call JABHandler.getWindowHandleFromAccContext, which sometimes causes a crash of the java application.

Fixes nvaccess#6992
@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh May 9, 2017

Contributor

@leonardder commented on 5 May 2017, 11:19 pm AEST:

So, what I like to have confirmed is that we can safely assume the hwnd is the same across the whole table.

I don't think we can be certain of this in all cases, but it's a reasonable assumption and it's far better than a crash.

There are several possibilities here. In JABHandler line 535: we can just add hwnd=self.hwnd to the JABContext constructor. However, if we can assume that the hwnd is equal, we can just pass the hwnd of the current context to every new JABContext constructor which is related to table information.

Does making this single change on line 535 fix the problem? I'd be inclined to go for the smallest change which fixes the crash at hand. Happy to make assumptions about the hwnd if it fixes a crash, but no point making assumptions we don't need to make in other functions, as we could just be hiding issues later.

Contributor

jcsteh commented May 9, 2017

@leonardder commented on 5 May 2017, 11:19 pm AEST:

So, what I like to have confirmed is that we can safely assume the hwnd is the same across the whole table.

I don't think we can be certain of this in all cases, but it's a reasonable assumption and it's far better than a crash.

There are several possibilities here. In JABHandler line 535: we can just add hwnd=self.hwnd to the JABContext constructor. However, if we can assume that the hwnd is equal, we can just pass the hwnd of the current context to every new JABContext constructor which is related to table information.

Does making this single change on line 535 fix the problem? I'd be inclined to go for the smallest change which fixes the crash at hand. Happy to make assumptions about the hwnd if it fixes a crash, but no point making assumptions we don't need to make in other functions, as we could just be hiding issues later.

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder May 16, 2017

Collaborator

@jcsteh: I'm sorry, I tried to answer your questions like a wekk ago, but I noticed the e-mail didn't come through properly.

Al though I see your point, I think that, since getTopLevelObject is very unstable, we should avoid calling this function whenever possible.

We should also consider these comments in JABHandler in this story:

Cache of the last active window handle for a given JVM ID. In theory, this cache should not be needed, as it should always be possible to retrieve the window handle of a given accessible context by calling getTopLevelObject then getHWNDFromAccessibleContext. However, getTopLevelObject sometimes returns accessible contexts that make getHWNDFromAccessibleContext fail. To workaround the issue, we use this cache as a fallback when either getTopLevelObject or getHWNDFromAccessibleContext fails.

One additional problem I have with only changing line 535 is that it is most likely that the current approach also crashes with row header retrieval. I haven't yet seen a Java table with row headers though.

Collaborator

leonardder commented May 16, 2017

@jcsteh: I'm sorry, I tried to answer your questions like a wekk ago, but I noticed the e-mail didn't come through properly.

Al though I see your point, I think that, since getTopLevelObject is very unstable, we should avoid calling this function whenever possible.

We should also consider these comments in JABHandler in this story:

Cache of the last active window handle for a given JVM ID. In theory, this cache should not be needed, as it should always be possible to retrieve the window handle of a given accessible context by calling getTopLevelObject then getHWNDFromAccessibleContext. However, getTopLevelObject sometimes returns accessible contexts that make getHWNDFromAccessibleContext fail. To workaround the issue, we use this cache as a fallback when either getTopLevelObject or getHWNDFromAccessibleContext fails.

One additional problem I have with only changing line 535 is that it is most likely that the current approach also crashes with row header retrieval. I haven't yet seen a Java table with row headers though.

jcsteh added a commit that referenced this issue Jul 18, 2017

Fix crashes when navigating tables in Java Swing applications. (PR #7150
, issue #6992)

When retrieving table cell, row and column information from a JABHandler.JABContext object, the window handle of the current object is now provided when creating new JABContext objects. This avoids the need to call JABHandler.getWindowHandleFromAccContext, which sometimes causes a crash of the java application.
@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Jul 18, 2017

Contributor

Fixed by #7150, which is now merged.

Contributor

jcsteh commented Jul 18, 2017

Fixed by #7150, which is now merged.

@jcsteh jcsteh closed this Jul 18, 2017

@jcsteh jcsteh added this to the 2017.3 milestone Jul 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment