Skip to content

SysListView32 fully in process for cases where allocating memory in a process would otherwise be required#13271

Merged
feerrenrut merged 14 commits into
nvaccess:masterfrom
LeonarddeR:moreSysListViewInProc
Feb 16, 2022
Merged

SysListView32 fully in process for cases where allocating memory in a process would otherwise be required#13271
feerrenrut merged 14 commits into
nvaccess:masterfrom
LeonarddeR:moreSysListViewInProc

Conversation

@LeonarddeR

@LeonarddeR LeonarddeR commented Jan 24, 2022

Copy link
Copy Markdown
Collaborator

Link to issue number:

Fixes #8175
Follow up of #11745
Fixes regression from #11469, #9873

Summary of the issue:

Getting list item content on 64 bit systems is sometimes problematic.

Description of how this pull request fixes the issue:

Moving text content fetching to in process in #11745 already fixed some but not all issues. The current pr adds:

  1. Location fetching
  2. Column header text fetching
  3. Column order array fetching

In short, everything that required allocating memory within a process using VirtualAlloc

Testing strategy:

  1. Tested all new functions using the python console to ensure they return the same as their outproc variants
  2. Tested the test case provided in No item names for some listviews (64-bit) #8175 (comment)

Known issues with pull request:

How likely are cases where injection doesn't work and the outproc variants succeed? I guess this is pretty unlikely, unless we're running under Windows Store.

Change log entries:

Bug fixes

- Fixed several cases where list items could not be reported in 64 bit applications, such as REAPER. (#8175)

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English

@LeonarddeR LeonarddeR requested a review from a team as a code owner January 24, 2022 12:34
@LeonarddeR LeonarddeR requested a review from seanbudd January 24, 2022 12:34
@LeonarddeR LeonarddeR changed the title SysListView32 fully in process SysListView32 fully in process for cases where allocating memory in a process would otherwise be required Jan 24, 2022
@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit 6ac854916e

@LeonarddeR

Copy link
Copy Markdown
Collaborator Author

@seanbudd seanbudd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach generally looks good to me, requesting @feerrenrut as a second reviewer due to C++ changes

def _get__columnOrderArray(self):
coa=(c_int *self.columnCount)()
def _getColumnOrderArrayRaw(self, columnCount: int):
coa = (ctypes.c_int * columnCount)()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this variable be renamed to be clearer?

Suggested change
coa = (ctypes.c_int * columnCount)()
columnOrderArray = (ctypes.c_int * columnCount)()


def _get__columnOrderArray(self):
coa=(c_int *self.columnCount)()
def _getColumnOrderArrayRaw(self, columnCount: int):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this (and other getColumnOrderArray functions) get a return type added?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how since we're using a dynamic python class (ctypes array) as the return type.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ctypes.Array works, however I could stand corrected.
Note that the following evaluates to True

import ctypes
columnOrderArray = (ctypes.c_int * 7)()
isinstance(columnOrderArray, ctypes.Array)

@seanbudd seanbudd requested a review from feerrenrut January 24, 2022 23:45
@AppVeyorBot

This comment was marked as outdated.

Comment thread nvdaHelper/remote/sysListView32.cpp
Comment thread nvdaHelper/remote/sysListView32.cpp Outdated
Comment thread nvdaHelper/remote/sysListView32.cpp
Comment thread nvdaHelper/remote/sysListView32.cpp Outdated
Comment thread nvdaHelper/remote/sysListView32.cpp Outdated
Comment thread nvdaHelper/remote/sysListView32.cpp Outdated
Comment thread nvdaHelper/remote/sysListView32.cpp Outdated
Comment thread nvdaHelper/remote/sysListView32.cpp Outdated
Comment thread nvdaHelper/remote/sysListView32.cpp Outdated
Comment thread nvdaHelper/interfaces/nvdaInProcUtils/nvdaInProcUtils.idl
@LeonarddeR LeonarddeR requested a review from feerrenrut January 28, 2022 07:40
Comment thread nvdaHelper/remote/sysListView32.cpp Outdated
@LeonarddeR

Copy link
Copy Markdown
Collaborator Author

@feerrenrut I think I've addressed all your comments, not sure though.

Comment thread nvdaHelper/remote/sysListView32.cpp Outdated
Comment thread nvdaHelper/remote/sysListView32.cpp Outdated
@feerrenrut feerrenrut merged commit af3b844 into nvaccess:master Feb 16, 2022
@nvaccessAuto nvaccessAuto added this to the 2022.1 milestone Feb 16, 2022
seanbudd pushed a commit that referenced this pull request Mar 31, 2022
Link to issue number:
Fix-up of PR #13271

Summary of the issue:
PR #13271 moved most of the logic for retrieving content in syslistview32 controls in process. As part of this work _getColumnLocationRaw was changed to return an instance of ctypes.wintypes.RECT rather than one of locationHelper classes, additionally the returned coordinates were no longer normalized. Some places in NVDA expects _getColumnLocationRaw to return normalized coordinates - in particular this affects the list control in Tween and my add-on for Becky! Internet Mail. The error when trying to navigate the list was as follows:

ERROR - scriptHandler.executeScript (21:29:59.073) - MainThread (1152):
error executing script: <bound method GlobalCommands.script_reportCurrentFocus of <globalCommands.GlobalCommands object at 0x064ECAD0>> with gesture 'NVDA+tab'
Traceback (most recent call last):
  File "scriptHandler.pyc", line 212, in executeScript
  File "globalCommands.pyc", line 2032, in script_reportCurrentFocus
  File "speech\speech.pyc", line 561, in speakObject
  File "speech\speech.pyc", line 604, in getObjectSpeech
  File "speech\speech.pyc", line 453, in getObjectPropertiesSpeech
  File "baseObject.pyc", line 42, in __get__
  File "baseObject.pyc", line 146, in _getPropertyViaCache
  File "appModules\tween.pyc", line 26, in _get_name
  File "baseObject.pyc", line 42, in __get__
  File "baseObject.pyc", line 146, in _getPropertyViaCache
  File "NVDAObjects\IAccessible\sysListView32.pyc", line 531, in _get_name
  File "NVDAObjects\IAccessible\sysListView32.pyc", line 459, in _getColumnContent
  File "appModules\tween.pyc", line 42, in _getColumnContentRaw
TypeError: cannot unpack non-iterable RECT object

Note that for these controls we cannot just use location returned from _getColumnLocation since indexes provided to it are not constant for a given column - they can change for example when user reorders columns.

Description of how this pull request fixes the issue:
Renames raw getters to rawInProc to clarify that these methods are used in-process
Add docstrings to clarify when and why these methods are used
The location normalization is once again performed by _getColumnLocationRaw
In the Tween app Module annoying decorational Unicode symbol is removed from the columns header - this is not strictly related to the main issue.
seanbudd added a commit that referenced this pull request May 6, 2022
Fix up of #13560, #13271

Summary of the issue:

When out-of-process failed, an empty array was returned instead of None.
This could have led to incorrect reporting of list items.
For example, trying to get the contents of the second column of the list/table/report, the array is initialized with all values of zero, if used without being filled with valid data all columns will report the same thing.

#13271 notes:
>How likely are cases where injection doesn't work and the outproc variants succeed? I guess this is pretty unlikely, unless we're running under Windows Store.

- If injection doesn't work there will be no `helperLocalBindingHandle`, out of process approach will be tried.
- If an application like StartIsBack/StartAllBack doesn't handle the Windows Message properly then it doesn't matter if the message is sent in-process or out-of-process.

Description of how this pull request fixes the issue:

- When possible, use in process approach to get the column order array. It should be possible if there is a `helperLocalBindingHandle`
- Don't return an array filled with zeros, return None when out-of-process fails.
seanbudd added a commit that referenced this pull request May 26, 2022
Fix up of #13560, #13271

Summary of the issue:

When out-of-process failed, an empty array was returned instead of None.
This could have led to incorrect reporting of list items.
For example, trying to get the contents of the second column of the list/table/report, the array is initialized with all values of zero, if used without being filled with valid data all columns will report the same thing.

#13271 notes:
>How likely are cases where injection doesn't work and the outproc variants succeed? I guess this is pretty unlikely, unless we're running under Windows Store.

- If injection doesn't work there will be no `helperLocalBindingHandle`, out of process approach will be tried.
- If an application like StartIsBack/StartAllBack doesn't handle the Windows Message properly then it doesn't matter if the message is sent in-process or out-of-process.

Description of how this pull request fixes the issue:

- When possible, use in process approach to get the column order array. It should be possible if there is a `helperLocalBindingHandle`
- Don't return an array filled with zeros, return None when out-of-process fails.
@LeonarddeR LeonarddeR deleted the moreSysListViewInProc branch August 23, 2025 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No item names for some listviews (64-bit)

5 participants