Remove previous add-on version before installing the next

[seanbudd]

Link to issue number:

Fixes #14991

Summary of the issue:

When installing an add-on from an external source, install tasks for the new add-on version are started before the previous add-on version is removed.
This causes the install to fail, as initialized DLLs as part of the install tasks may block the remove from occurring.
This is legacy behaviour (before #13985) so it is uncertain why this is a recent regression.

Description of user facing changes

Should allow updating add-ons with DLL files from an external source.

Description of development approach

Move install tasks to after removal

Testing strategy:

• Confirm manual testing with @cary-rowen

Known issues with pull request:

none

Change log entries:

N/A

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
• Security precautions taken.

[seanbudd]

@cary-rowen - can you please test #14991 with this try build and confirm you can no longer reproduce the issue?

Edit: nevermind

https://ci.appveyor.com/api/buildjobs/q3udp9sprnt9sywa/artifacts/output%2Fnvda_snapshot_try-removeAddonBeforeInstall-28455%2C622da650.exe

[LeonarddeR]

I wonder whether this should be treated as API breaking. Some add-ons might rely on the current order of install tasks being executed, e.g. they might check whether they're updating and perform a configuration update based on whether the previous version is installed.
Any thoughts about this @josephsl?

[seanbudd]

I wonder whether this should be treated as API breaking. Some add-ons might rely on the current order of install tasks being executed, e.g. they might check whether they're updating and perform a configuration update based on whether the previous version is installed.

If this is the case, we would also need to fix _addonStore.install.installAddon which follows the order that this PR changes to.

[josephsl]

Hi,

If I understand the proposed idea, the order is:

1. Remove a previous add-on version.
2. Extract and run install tasks module from the new add-on version.

This can create two issues:

1. The new add-on version may perform compatibility checks to see if it can be installed. This assumes a previous version of the add-on is installed and is needed by users. Now suppose the old and working version is removed and then the newer add-on release performs some checks and sees that the add-on should not be installed. In the best case, the old add-on would not be removed, and in the worst case scenario, an important add-on or two needed by users will be gone when NVDA restarts. Windows App Essentials add-on is one such case.
2. Suppose an add-on stores its options or data inside the package (say, creating new folders to store user data while the add-on runs). Some add-ons migrte data while upgrading, and removing the old add-on and then running the migration step would not make sense as the migratino routine (part of instal tasks) will not see the old data. StationPlaylist add-on is a good example.

In case of add-ons with DLL's, ultimately it is NVDA that loads these DLL's (through add-ons), so these DLL's will not be freed from memory (unloaded) unless NVDA exits.

Thanks.

[LeonarddeR]

The underlying issue is actually a bug in the store I just discovered. I will write down my findings in #14991.

[seanbudd]

Good to know there is a different cause. When addressing it, we should make both installAddon functions work the same way to prevent the issues Joseph outlined.