Protect functions from using terminated OneCore instances#13569
Protect functions from using terminated OneCore instances#13569seanbudd wants to merge 11 commits into
Conversation
|
Is it necessary to apply a similar approach to the OCR code? |
Maybe, but for a given handle |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
See test results for failed build of commit 4b1193ab51 |
| using namespace winrt::Windows::Foundation::Collections; | ||
| using winrt::Windows::Foundation::Metadata::ApiInformation; | ||
|
|
||
| std::recursive_mutex InstanceManager::_instanceStateMutex; |
There was a problem hiding this comment.
It seems like it is only waitUntilReadyForInitialization / initializeNewInstance that requires this to be recursive?
| /* | ||
| When initialized, increases the count of active speech threads | ||
| waiting on a callback. | ||
| When exiting the scope of initialization, decreases the count | ||
| of active speech threads and deletes the OneCore instance if it has | ||
| been terminated and no speech callbacks are waiting. | ||
| */ |
There was a problem hiding this comment.
C++ docs (like python doc strings) are typically written on the line before the declaration, rather than within the definition.
It is also helpful to have reasoning for the implementation within the definition.
| class InstanceManager { | ||
| private: | ||
| static std::recursive_mutex _instanceStateMutex; |
There was a problem hiding this comment.
This and SpeakThreadGuard don't need to be in this header. They aren't used outside of the oneCoreSpeech.cpp compilation unit, and don't need to have their compilation tied to oneCoreSpeech. We'll likely need them separate for OCR.
| Waits until an instance has been fully terminated, | ||
| and then (once speech callbacks have finished) deleted. | ||
| */ | ||
| std::lock_guard g(_instanceStateMutex); |
There was a problem hiding this comment.
This can cause a deadlock. This is acquiring the lock potentially while there is an active SpeakThreadGuard due to OcSpeech::speak which can't terminate until it acquires the lock to delete the instance and signal _readyForInitialization
Instead, I'd make waitUntilReadyForInitialization transfer ownership of the lock back to the caller after it is acquired safely.
|
Closing as a new approach is required |
Supersedes PR #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: - Run NVDA - In the Keyboard section of the Preferences dialog, turn "Speak command keys" on. - Reset your configuration to factory defaults - Note that NVDA crashes Description of fix: 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.
Link to issue number:
N/A
Summary of the issue:
NVDA can crash when resetting the configuration to factory defaults.
This is because NVDA terminates OneCore when resetting the config.
However, a race condition exists with the async function
speak, where the python callback uses a terminated OneCore instance.Steps to reproduce:
Description of how this pull request fixes the issue:
Protects usages of the OneCore instance by confirming the instance has not been terminated.
Wait until speech callbacks are completed before re-initializing.
Testing strategy:
Manual testing
Known issues with pull request:
Other, somewhat related bugs, are logged as Errors during this process. See: #13580
Change log entries:
Bug fixes
Code Review Checklist: