Skip to content
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.

Sign up for GitHub

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

Jump to bottom

UIA: crash at very end of buffer #7771

Closed
codeofdusk opened this issue Sep 28, 2020 · 6 comments · Fixed by #7792
Closed

UIA: crash at very end of buffer #7771

codeofdusk opened this issue Sep 28, 2020 · 6 comments · Fixed by #7792
Assignees
@carlos-zamora
Labels
Area-Accessibility Issues related to accessibility Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-0 Bugs that we consider release-blocking/recall-class (P0) Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Crash Crashes are real bad news.
Milestone
Windows vNext

Comments

@codeofdusk
Copy link
Contributor

codeofdusk commented Sep 28, 2020 •
edited
Loading

In the NVDA Python console (NVDA+control+z):

>>> from NVDAObjects.UIA import UIATextInfo
>>> ti=UIATextInfo(nav, "last")
>>> ti.move("character", 1)  # conhost crashes
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "NVDAObjects\UIA\__init__.pyc", line 752, in move
  File "comtypesMonkeyPatches.pyc", line 26, in __call__
_ctypes.COMError: (-2147220991, 'An event was unable to invoke any of the subscribers', (None, None, None, 0, None))

Note: fixing this bug may result in strange behaviour from NVDA (see here, around line 53). I'll remove the offending workaround once a fix is merged to master.

I suspect #6986 would fix this bug, but I think the root cause of this crash should be investigated before then. Cc @carlos-zamora.
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Sep 28, 2020
@DHowett
Copy link
Member

DHowett commented Sep 28, 2020

This is with main, not Carlos's PR #7770, yes?

@codeofdusk
Copy link
Contributor Author

Correct.

@carlos-zamora
Copy link
Member

@codeofdusk what does UIATextInfo(nav, "last") do?

@codeofdusk
Copy link
Contributor Author

See here (starting at around line 292), but in summary it gets a document range degenerate at end (i.e. moves start to where end is).

@DHowett DHowett added Area-Accessibility Issues related to accessibility Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-0 Bugs that we consider release-blocking/recall-class (P0) Product-Conhost For issues in the Console codebase Severity-Crash Crashes are real bad news. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Sep 29, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Sep 29, 2020
@DHowett DHowett added this to the Windows vNext milestone Sep 29, 2020
@carlos-zamora carlos-zamora mentioned this issue Oct 1, 2020
Merged
@ghost ghost added the In-PR This issue has a related PR label Oct 1, 2020
@codeofdusk codeofdusk mentioned this issue Oct 2, 2020
Closed
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Oct 5, 2020
DHowett pushed a commit that referenced this issue Oct 5, 2020
@carlos-zamora
Properly handle and test a11y movement at end of buffer (#7792)
e401edf 
The `MovementAtExclusiveEnd` test was improperly authored for the
following reasons:
- it should have used `TEST_METHOD_PROPERTY` to cover all of the
  TextUnits
- TextUnit::Document (arguably one of the most important) was ommitted
  accidentally (`!= TextUnit_Document` was used instead of `<=`)
- The created range was not `EndExclusive`, but rather, the last cell in
  the buffer (`EndInclusive`)

The first half of this PR fixes the test.

The second half of this PR expands the test and fixes any related issues
to make the test pass (i.e. #7771):
- `TEST_METHOD_PROPERTY` was added for it to be degenerate (start/end at
  `EndExclusive`) or not (last cell of buffer)
- `utr->_start` is now also validated after moving backwards

NOTE: `utr->_start` was not validated when moving forwards because
moving forwards should always fail when at/past the last chell in the
buffer.

Closes #7771
DHowett pushed a commit that referenced this issue Oct 19, 2020
@carlos-zamora @DHowett
Properly handle and test a11y movement at end of buffer (#7792)
afe5860 
The `MovementAtExclusiveEnd` test was improperly authored for the
following reasons:
- it should have used `TEST_METHOD_PROPERTY` to cover all of the
  TextUnits
- TextUnit::Document (arguably one of the most important) was ommitted
  accidentally (`!= TextUnit_Document` was used instead of `<=`)
- The created range was not `EndExclusive`, but rather, the last cell in
  the buffer (`EndInclusive`)

The first half of this PR fixes the test.

The second half of this PR expands the test and fixes any related issues
to make the test pass (i.e. #7771):
- `TEST_METHOD_PROPERTY` was added for it to be degenerate (start/end at
  `EndExclusive`) or not (last cell of buffer)
- `utr->_start` is now also validated after moving backwards

NOTE: `utr->_start` was not validated when moving forwards because
moving forwards should always fail when at/past the last chell in the
buffer.

Closes #7771

(cherry picked from commit e401edf)
@ghost
Copy link

ghost commented Nov 11, 2020

🎉This issue was addressed in #7792, which has now been successfully released as Windows Terminal v1.4.3141.0.:tada:

Handy links:

@ghost
Copy link

ghost commented Nov 11, 2020

🎉This issue was addressed in #7792, which has now been successfully released as Windows Terminal Preview v1.5.3142.0.:tada:

Handy links:

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

@carlos-zamora carlos-zamora

Labels
Area-Accessibility Issues related to accessibility Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-0 Bugs that we consider release-blocking/recall-class (P0) Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Crash Crashes are real bad news.
Projects
None yet
Milestone
Windows vNext
Development

Successfully merging a pull request may close this issue.

Properly handle and test a11y movement at end of buffer microsoft/terminal
3 participants
@DHowett @codeofdusk @carlos-zamora