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

Fast Diff Match Patch crashes and the calling thread deadlocks when astral characters are printed in Windows Terminal #16027

Closed
nstockton opened this issue Jan 10, 2024 · 5 comments
Labels
app/windows-terminal New terminal app, potentially supersedes app/windows-console (repo: microsoft/terminal) bug/regression p2 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority triaged Has been triaged, issue is waiting for implementation.
Milestone

Comments

@nstockton
Copy link

Steps to reproduce:

  • In the NVDA Advanced settings panel, insure that "Diff algorithm" is set to "Diff Match Patch" (default), and "Speak new text in Windows Terminal via" is set to "Diffing" (default).
  • Run the Python interpreter in Windows Terminal.
  • Execute the following Python print statement.
print("\N{SHORTCAKE}")

Actual behavior:

If running a recent build of NVDA that uses the Fast Diff Match Patch library:

  • The nvda_dmp.exe process will throw a silent exception and terminate, disappearing from Task Manager. No log info is generated, even when NVDA is logging in debug mode.
  • the thread that ran nvda_dmp.exe via subprocess will deadlock, because it is waiting for text which it will never receive.
  • NVDA will no longer automatically speak new output in terminal windows, or any other window that inherits from NVDAObjects.behaviors.LiveText until restarted.
  • NVDA will fail to play the exit sound when told to restart, because of the deadlocked thread.

Expected behavior:

if 'Include Unicode Consortium data is enabled in NVDA speech settings, the name of the emoji "Shortcake" should be spoken. If not, NVDA should not speak the name of the character. Either way, NVDA should be sanitizing the text before sending it to the nvda_dmp.exe process to prevent it from crashing, and to insure continued automatic speech output in terminal windows.

NVDA logs, crash dumps and other attachments:

dmp_fixer.nvda-addon.zip

System configuration

NVDA installed/portable/running from source:

installed

NVDA version:

2024.1beta2 (2024.1.0.30416)

Windows version:

Windows 11, version 23H2

Name and version of other software in use when reproducing the issue:

Windows Terminal, Version 1.18.3181.0

Other information about your system:

Not applicable.

Other questions

Does the issue still occur after restarting your computer?

yes if another astral character is printed to the terminal, or any window who's overlay class inherits from NVDAObjects.behaviors.LiveText.

Have you tried any other versions of NVDA? If so, please report their behaviors.

Bug is applicable to versions of NVDA that use Fast Diff Match Patch.

If NVDA add-ons are disabled, is your problem still occurring?

Not applicable.

Does the issue still occur after you run the COM Registration Fixing Tool in NVDA's tools menu?

Not applicable.

Some background

The Fast Diff Match Patch ReadMe says:

On Windows, an exception will be thrown if either of the two text strings has characters outside of the Basic Multilingual Plane because the native platform character type is a two-byte character.

Some Emoji characters (not all) fall outside of the Basic Multilingual Plane, in the so-called Astral Plane. Unfortunately, some console applications such as the Black formatter for Python output Emoji characters to the console, and in the case of Black, don't have the ability to disable this functionality.

When a recent build of NVDA which makes use of the new Fast Diff Match Patch library tries to automatically speak a character in the astral plane, the nvda_dmp.exe process terminates with an exception, and the NVDA thread that called nvda_dmp.exe is left blocking while it waits for the output of nvda_dmp.exe, which of course it will never receive. Because of the thread becoming deadlocked in this way, no log output is emitted, making the source of the issue tricky to track down.

I created an NVDA add-on that patches classes which inherit from NVDAObjects.behaviors.LiveText, in order to sanitize strings before Fast Diff Match Patch receives them and prevent nvda_dmp.exe from terminating unexpectedly. The add-on will speak astral character names if the user has enabled the 'Include Unicode Consortium data in the NVDA speech settings panel, otherwise it will omit speaking the character name, for consistency with how emoji and other unicode characters in the Basic Multilingual Plane are currently spoken. I've attached the add-on above, but a fix should really be integrated into NVDA its self, as this is an issue which affects NVDA's usage with at least one common CLI-based tool (I.E. Black) and probably several others.

@Danstiv
Copy link
Contributor

Danstiv commented Jan 10, 2024

See also #15850

@Adriani90
Copy link
Collaborator

cc: @codeofdusk

@lukaszgo1
Copy link
Contributor

This was introduced in JoshData/fast_diff_match_patch@06a9f4e. While this is an improvement in some cases for our purposes this clearly classifies as a regression. It seems fix from the Fast Diff Match Patch developer is quite unlikely, they stated in JoshData/fast_diff_match_patch#27 and JoshData/fast_diff_match_patch#26, that they're no longer actively maintaining the project. Has it been considered to use the version of diff match patch written in pure Python?

@seanbudd seanbudd added this to the 2024.1 milestone Jan 10, 2024
@codeofdusk
Copy link
Contributor

@nstockton Thanks for getting to the bottom of this! I'll look more into it soon.

@nstockton
Copy link
Author

No problem. Thanks for all your work with windows terminal accessibility. It made my transition from Legacy console fairly painless when I finally started using Windows terminal.

@seanbudd seanbudd added p2 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority triaged Has been triaged, issue is waiting for implementation. app/windows-terminal New terminal app, potentially supersedes app/windows-console (repo: microsoft/terminal) bug/regression labels Jan 15, 2024
Adriani90 pushed a commit to Adriani90/nvda that referenced this issue Mar 13, 2024
Fixes nvaccess#16027
Reverts nvaccess#15514
This reverts commit 69a16cc.

Summary of the issue:
PR nvaccess#15514 upgraded diff_match_patch from 1.0.2 to fast_diff_match_patch 2.0.1.
However, the newer version cannot handle particular unicode characters such as 🍰. The diff_match_patch process dies, NvDA can no longer report text changes, and NvDA hangs on exit.

Description of user facing changes
Printing unicode characters such as 🍰 in a terminal withn using diff_match_patch for speaking changes no longer causes NvDA to no longer report text changes and lock up on exit.

Description of development approach
Downgrade back to diff_match_patch 1.0.2.
seanbudd pushed a commit that referenced this issue May 9, 2024
Follow up to #16027
Fixes #15850

Summary of the issue:
Diff Match Patch proxy crashes and the calling thread deadlocks

Description of user facing changes
Diff Match Patch proxy will become more stable

Description of development approach
Refactored DiffMatchPatch diff handler.

Now, when reading from stdout of a proxy process, if not enough bytes are read, the return code is checked.

If a return code was received, an exception is raised and a fallback to difflib occurs.
XLTechie pushed a commit to XLTechie/xlnvda that referenced this issue May 10, 2024
Follow up to nvaccess#16027
Fixes nvaccess#15850

Summary of the issue:
Diff Match Patch proxy crashes and the calling thread deadlocks

Description of user facing changes
Diff Match Patch proxy will become more stable

Description of development approach
Refactored DiffMatchPatch diff handler.

Now, when reading from stdout of a proxy process, if not enough bytes are read, the return code is checked.

If a return code was received, an exception is raised and a fallback to difflib occurs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app/windows-terminal New terminal app, potentially supersedes app/windows-console (repo: microsoft/terminal) bug/regression p2 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority triaged Has been triaged, issue is waiting for implementation.
Projects
None yet
Development

No branches or pull requests

6 participants