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.

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

HP Secure Browser support (#16377): Amend browseMode dead document detection to cope with VT_EMPTY runtimeID. #16393

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

tomaszw
Copy link
Contributor

@tomaszw tomaszw commented Apr 15, 2024

Link to issue number:

#16377

Summary of the issue:

When enough websites are open in separate tabs, SB will suspend some of the virtual machines hosting the websites to disk. This can cause UIA documents representing these websites to become dead.

In this scenario querying UIA_RuntimeIdPropertyId of the dead document returns VT_EMPTY variant, causing uncaught exception raised from UIABrowseModeDocument.contains. This keeps repeating, preventing NVDA from working correctly, if at all.

Description of user facing changes

NVDA works correctly regardless of vm suspensions or other scenarios where UIA documents can become dead.

Description of development approach

UIABrowseModeDocument.contains already has code to detect dead documents, however it relies on COMError being reported from UIA_RuntimeIdPropertyId property query.

In my testing, even when the server side reports an error HRESULT from IRawElementProviderFragment::GetRuntimeID() uia method, UIA core appears to translate it into a successful property query, yet with a VT_EMPTY variant as the property value. It's possible this behaviour is different between various UIA core versions, my testing is primarily on Windows 11 build 22631.

We amend the detection such that both COMError and VT_EMPTY variant are detected and treated equally, causing the dead document codepath to execute.

Testing strategy:

Manual testing

Known issues with pull request:

N/A

Code Review Checklist:

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

…ument detection to cope with VT_EMPTY runtimeID.

Summary of the issue:
When enough websites are open in separate tabs, SB will suspend some
of the virtual machines hosting the websites to disk. This can cause
UIA documents representing these websites to become dead.

In this scenario querying UIA_RuntimeIdPropertyId of the dead document
returns VT_EMPTY variant, causing uncaught exception raised from
UIABrowseModeDocument.__contains__. This keeps repeating, preventing
NVDA from working correctly, if at all.

Description of user facing changes:
NVDA works correctly regardless of vm suspensions or other scenarios
where UIA documents can become dead.

Description of development approach:
UIABrowseModeDocument.__contains__ already has code to detect dead documents,
however it relies on COMError being reported from UIA_RuntimeIdPropertyId
property query.

In my testing, even when the server side reports an error HRESULT from
IRawElementProviderFragment::GetRuntimeID() uia method, UIA core appears
to translate it into a successful property query, yet with a VT_EMPTY variant
as the property value. It's possible this behaviour is different between
various UIA core versions, my testing is primarily on Windows 11 build 22631.

We amend the detection such that both COMError and VT_EMPTY variant
are detected and treated equally, causing the dead document codepath to execute.
@tomaszw tomaszw mentioned this pull request Apr 15, 2024
5 tasks
@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Apr 16, 2024
@tomaszw tomaszw marked this pull request as ready for review April 16, 2024 06:25
@tomaszw tomaszw requested a review from a team as a code owner April 16, 2024 06:25
@seanbudd seanbudd merged commit e8d67bb into nvaccess:master Apr 16, 2024
1 check was pending
@nvaccessAuto nvaccessAuto added this to the 2024.2 milestone Apr 16, 2024
tomaszw pushed a commit to tomaszw/nvda-hp that referenced this pull request Apr 17, 2024
…access#16395

Summary of the issue:
Changelog entries are missing for nvaccess#16392 nvaccess#16393 nvaccess#16394 nvaccess#16395

Description of user facing changes:
Added Changelog entries

Description of development approach:
N/A
tomaszw pushed a commit to tomaszw/nvda-hp that referenced this pull request Apr 17, 2024
…access#16395

Summary of the issue:
Changelog entries are missing for nvaccess#16392 nvaccess#16393 nvaccess#16394 nvaccess#16395

Description of user facing changes:
Added Changelog entries

Description of development approach:
N/A
tomaszw pushed a commit to tomaszw/nvda-hp that referenced this pull request Apr 17, 2024
…access#16395

Summary of the issue:
Changelog entries are missing for nvaccess#16392 nvaccess#16393 nvaccess#16394 nvaccess#16395

Description of user facing changes:
Added Changelog entries

Description of development approach:
N/A
seanbudd added a commit that referenced this pull request Apr 18, 2024
* Changelog entries for #16392 #16393 #16394 #16395

Summary of the issue:
Changelog entries are missing for #16392 #16393 #16394 #16395

Description of user facing changes:
Added Changelog entries

Description of development approach:
N/A

* Apply suggestions from code review

---------

Co-authored-by: Tomasz Wroblewski <tomasz.wroblewski@hp.com>
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants