nvaccess
Recover safely if the add-on store cache is invalid
- Don't restart if cache files in add-on store are not correct
- Update changelog
Link to issue number:
Fixes #16362
Summary of the issue:
When cache files of the add-on store contain invalid data, NVDA is restarted.
Description of user facing changes
None
Description of development approach
In the _getCachedAddonData function of the _DataManager class, a cachedAddonDatavariable will try to get the value of the _createStoreCollectionFromJson(data)`.
So, if data doesn't match that model, the same exception of other invalid required values will be raised, and NVDA won't be restarted.
Testing strategy:
- The addonId of the searchWith add-on has been removed from a cache file.
-
- Pressed NVDA+q to restart NVDA.
- Confirmed that NVDA was restarted befor aplying this fix, but not after.
Known issues with pull request:
None
Code Review Checklist:
- [x] Documentation:
- Change log entry
- User Documentation
- Developer / Technical Documentation
- Context sensitive help for GUI changes
- [x] Testing:
- Unit tests
- System (end to end) tests
- Manual testing
- [x] UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers
- Localization in other languages / culture than English
- [x] API is compatible with existing add-ons.
- [x] Security precautions taken.
I've used the suggestion provided by @gerald-hartig for handling exceptions. I think this is ready for review. Note that I've added the changelog entry in NVDA 2024.2, since 2024.3 section was not present.
- PASS: Translation comments check.
- PASS: Unit tests.
- FAIL: Lint check. See test results for more information.
- PASS: System tests (tags: installer NVDA).
- Build (for testing PR): https://ci.appveyor.com/api/buildjobs/1y7gjm9xpbvp524b/artifacts/output/nvda_snapshot_pr16461-31684,311ee62e.exe
- CI timing (mins): INIT 0.0, INSTALL_START 1.1, INSTALL_END 1.0, BUILD_START 0.0, BUILD_END 28.9, TESTSETUP_START 0.0, TESTSETUP_END 0.4, TEST_START 0.0, TEST_END 2.3, FINISH_END 0.2
See test results for failed build of commit 311ee62e77
@codeofdusk codeofdusk 2 minutes ago No, I think the bare except was okay here (back out that change). I was referring to guarding the json.loads call in _createStoreCollectionFromJson itself with a try/except.
I'll revert my last commit. For any reason unit tests are failing now.
About the better handling of exception in _createStoreCollectionFromJson, I'm not sure if should be done here or in a different PR.
- PASS: Translation comments check.
- PASS: Unit tests.
- PASS: Lint check.
- FAIL: System tests (tags: installer NVDA). See test results for more information.
- Build (for testing PR): https://ci.appveyor.com/api/buildjobs/c0qdlxq6ica4yarr/artifacts/output/nvda_snapshot_pr16461-31685,71c3f870.exe
- CI timing (mins): INIT 0.0, INSTALL_START 1.1, INSTALL_END 1.1, BUILD_START 0.0, BUILD_END 28.0, TESTSETUP_START 0.0, TESTSETUP_END 0.4, TEST_START 0.0, TEST_END 2.3, FINISH_END 0.2
See test results for failed build of commit 71c3f870d5
@nvdaes - don't need to worry about unexpected failures
Automated checks will be run against your PR. If these fail, please review them. Sometimes system tests fail unexpectedly. If you believe the failure is unrelated, feel free to ignore it unless it is raised by a reviewer.
@seanbudd, I've tested this now, trying to change the file encoding (without gettting actually errors), and removing an addonId. I think thisis ready for review.
Note that this is targeting beta - so please merge in origin/beta and move the change log entry to changes.md
@seanbudd , I have fixed conflicts and moved changelog entry to readme. Also, I run git submodule update and miscDeps are updated.