Recover safely if the add-on store cache is invalid by nvdaes · Pull Request #16461 · nvaccess/nvda

nvdaes · 2024-04-29T16:46:11Z

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:

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.

codeofdusk · 2024-04-29T21:47:23Z

 			return None
 		try:
 			data = cacheData["data"]
+			cachedAddonData = _createStoreCollectionFromJson(data)


I'd like to see better exception handling in _createStoreCollectionFromJson. In particular, if we get a JSONEncodeError or KeyError due to malformed data, invalidate the cache.

Isn't that already occurring here for KeyError? Also I think you mean JSONDecodeError

except (KeyError, JSONDecodeError):

please change the exception handling below on line 189 to include JSONDecodeError

@seanbudd , I've included JSONDecodeError in the exception as you requested.

have you tested this? It doesnt seem like JSONDecodeError is imported from anywhere

No, sorry, I haven't tested it and in fact VS Code is raising a warning. I cannot test this now. I'll try to import JSONDecodeError

@seanbudd , I've imported JSONDecodeError and VS Code doesn't show warnings.

please let me know when you have tested the code - we do not review or merge untested code

nvdaes · 2024-04-30T04:38:48Z

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.

AppVeyorBot · 2024-04-30T05:11:35Z

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 · 2024-04-30T05:18:41Z

 			with open(cacheFilePath, 'r', encoding='utf-8') as cacheFile:
 				cacheData = json.load(cacheFile)
-		except Exception:
+		except (KeyError, JSONDecodeError):


No, I think the bare except was okay here (back out this change). I was referring to guarding the json.loads call in _createStoreCollectionFromJson itself with a try/except, or ensure appropriate exception handling at all callsites.

I don't think that's necessary, an error being raised outside of loading the cached data from file on startup will fail safely.

nvdaes · 2024-04-30T05:23:04Z

@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.

AppVeyorBot · 2024-04-30T05:59:23Z

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 · 2024-04-30T06:20:54Z

System tests are failing, I don't now why.

seanbudd · 2024-05-01T23:23:38Z

@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.

contributing docs

nvdaes · 2024-05-02T13:32:49Z

@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.

seanbudd · 2024-05-06T05:38:53Z

 			if NVDAState.shouldWriteToDisk():
 				os.remove(cacheFilePath)
 			return None
+		from json import JSONDecodeError


please import this from the base file, there is no need for this to be a lazy import

seanbudd · 2024-05-08T03:27:48Z

@nvdaes - can you resolve merge conflicts here please?

seanbudd · 2024-05-08T03:28:12Z

Note that this is targeting beta - so please merge in origin/beta and move the change log entry to changes.md

seanbudd · 2024-05-08T03:56:27Z

It appears miscDeps is mistakenly updated in this PR

nvdaes · 2024-05-08T04:16:00Z

@seanbudd , I have fixed conflicts and moved changelog entry to readme. Also, I run git submodule update and miscDeps are updated.

seanbudd · 2024-05-08T04:25:00Z

@nvdaes - this Pr is now only the miscdeps changes

seanbudd

Thanks @nvdaes

nvdaes mentioned this pull request Apr 29, 2024

When the JSON cache file in the add-on store is incorrect, starting NVDA afterwards will always restart. #16362

Closed

codeofdusk reviewed Apr 29, 2024

View reviewed changes

seanbudd changed the title ~~fixBadCachedAddons~~ Recover safely if the add-on store cache is invalid Apr 29, 2024

seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Apr 30, 2024

nvdaes marked this pull request as ready for review April 30, 2024 04:39

nvdaes requested a review from a team as a code owner April 30, 2024 04:39

nvdaes requested a review from michaelDCurran April 30, 2024 04:39

codeofdusk reviewed Apr 30, 2024

View reviewed changes

seanbudd added this to the 2024.2 milestone May 1, 2024

seanbudd reviewed May 6, 2024

View reviewed changes

seanbudd marked this pull request as draft May 6, 2024 05:39

nvdaes marked this pull request as ready for review May 7, 2024 03:12

seanbudd changed the base branch from master to beta May 7, 2024 05:44

seanbudd added the queued for merge label May 7, 2024

seanbudd previously approved these changes May 7, 2024

View reviewed changes

nvdaes dismissed seanbudd’s stale review via 2e311c4 May 8, 2024 03:54

Revert miscDeps updated by mistake

8c50619

nvdaes force-pushed the fixBadCachedAddons branch from 2e311c4 to 8c50619 Compare May 8, 2024 04:22

nvdaes added 2 commits May 8, 2024 06:26

Revert changes

8f3bcf7

Reset miscDeps

f13c47f

seanbudd approved these changes May 8, 2024

View reviewed changes

seanbudd merged commit 28ce9b8 into nvaccess:beta May 10, 2024

Uh oh!

Conversation

nvdaes commented Apr 29, 2024

Link to issue number:

Summary of the issue:

Description of user facing changes

Description of development approach

Testing strategy:

Known issues with pull request:

Code Review Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nvdaes commented Apr 30, 2024

Uh oh!

AppVeyorBot commented Apr 30, 2024

Uh oh!

codeofdusk Apr 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nvdaes commented Apr 30, 2024

Uh oh!

AppVeyorBot commented Apr 30, 2024

Uh oh!

nvdaes commented Apr 30, 2024

Uh oh!

seanbudd commented May 1, 2024

Uh oh!

nvdaes commented May 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanbudd commented May 8, 2024

Uh oh!

seanbudd commented May 8, 2024

Uh oh!

seanbudd commented May 8, 2024

Uh oh!

nvdaes commented May 8, 2024

Uh oh!

seanbudd commented May 8, 2024

Uh oh!

seanbudd left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codeofdusk Apr 30, 2024 •

edited

Loading