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

Prevent crash from terminated ocSpeech #13634

Merged
merged 4 commits into from Apr 26, 2022
Merged

Prevent crash from terminated ocSpeech #13634

merged 4 commits into from Apr 26, 2022

Conversation

feerrenrut
Copy link
Contributor

@feerrenrut feerrenrut commented Apr 25, 2022 •
edited

Link to issue number:

Supersedes #13569

Summary of the issue:

NVDA was crashing when resetting the configuration to factory defaults.
NVDA terminates the ocSpeech native module when resetting the config, however there is a pending callback to a now deleted python function.

Steps to reproduce:

  1. Run NVDA
  2. In the Keyboard section of the Preferences dialog, turn "Speak command keys" on.
  3. Reset your configuration to factory defaults
  4. Note that NVDA crashes

Description of how this pull request fixes the issue:

  • Changes the design of the ocSpeech module.
  • Both initialization and termination are blocked if a callback is about to happen.
    The "tokens" for prior initialization of ocSpeech are collected by the ocSpeech system.
    When an async task reaches completion, the origin token is checked for validity, the callback is not called for a now invalid token.
  • Initialization now requires the callback to be specified.

Testing strategy:

Manual testing

  1. Run NVDA
  2. In the Keyboard section of the Preferences dialog, turn "Speak command keys" on.
  3. Reset your configuration to factory defaults
  4. Note that NVDA does not crash.

Known issues with pull request:

Other errors are logged during config reset See: #13580

Change log entries:

Bug fixes

- NVDA no longer crashes when resetting the NVDA configuration to factory defaults while speak command keys is on. ()

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

seanbudd and others added 2 commits April 25, 2022 21:13
@seanbudd @feerrenrut
Fix termination with pending callback error
c15add0 
When oneCoreSpeech has an async task to synthesize speech,
then pass the result to a callback running and terminate is called
a crash could occur.

This change makes the system more durable and easier to debug.
@feerrenrut
Fix several python errors on factory reset.
183f2a4
@feerrenrut feerrenrut requested a review from a team as a code owner April 25, 2022 13:14
@feerrenrut feerrenrut requested review from michaelDCurran and seanbudd and removed request for a team April 25, 2022 13:14
seanbudd
seanbudd previously approved these changes Apr 26, 2022
View reviewed changes
Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

Generally looks good to me, minor typo fixes.

nvdaHelper/localWin10/oneCoreSpeech.h Outdated Show resolved Hide resolved
nvdaHelper/localWin10/oneCoreSpeech.cpp Outdated Show resolved Hide resolved
nvdaHelper/localWin10/oneCoreSpeech.cpp Outdated Show resolved Hide resolved
seanbudd
seanbudd previously approved these changes Apr 26, 2022
View reviewed changes
@feerrenrut feerrenrut merged commit 13dff03 into beta Apr 26, 2022
@feerrenrut feerrenrut deleted the fixOcSpeechTerminateCrash branch April 26, 2022 09:48
@nvaccessAuto nvaccessAuto added this to the 2022.2 milestone Apr 26, 2022
@feerrenrut feerrenrut modified the milestones: 2022.2, 2022.1 Apr 26, 2022
@feerrenrut feerrenrut mentioned this pull request Apr 29, 2022
Closed
feerrenrut added a commit that referenced this pull request Apr 29, 2022
@feerrenrut
Fix say-all with oneCore synth
700f033 
Issue introduced with PR
#13634
@feerrenrut feerrenrut mentioned this pull request Apr 29, 2022
Merged
5 tasks
feerrenrut added a commit that referenced this pull request Apr 29, 2022
@feerrenrut
Fix say-all with oneCore synth (PR #13652)
67bb085 
Fixes #13651

Summary of the issue:
Starting say-all with synth set to onecore failed with an error.

A logic error in the PR #13634
Caused the "markerStr" to be constructed incorrectly.
The | character expected to separate entries was missing.
Example string: 20:1437521:18864375
Should be instead: 20:14375|21:18864375

Description of change:
Fix the logic error, every entry after the first should start with a | character.
@seanbudd seanbudd mentioned this pull request Jun 17, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanbudd seanbudd seanbudd left review comments

@michaelDCurran michaelDCurran Awaiting requested review from michaelDCurran michaelDCurran is a code owner automatically assigned from nvaccess/developers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2022.1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@feerrenrut @seanbudd @nvaccessAuto