Remove add-on modules imported in install tasks to avoid conflicts between add-ons

[lukaszgo1]

Link to issue number:

Fixes regression from PR #14481.
Discovered due to intermittent failures occurring when installing add-ons, when working on #15852 and #15937

Summary of the issue:

When installing and uninstalling add-ons bundling install tasks, installation either fails or behaves unexpectedly in the following scenarios:

1:

• User installs an add-on
• User removes it and restarts NVDA
• User tries to install the same add-on

In this scenario when installing the install tasks included in the version of add-on which was just removed are used, which is unexpected. Note that this worked well before #14481, not sure why pkgutil.ImpImporter behaves better (I haven't investigated, as it is deprecated).

2:

• User installs an add-on, which imports its sub module in install tasks using loadModule and also imports a sub module in the same way inside global plugin / app module during normal operation
• User upgrades the add-on to a different version

In this scenario after upgrade the add-on module from the uninstalled version is used during normal operation of NVDA.

3:

• User installs an add-on which imports one of its modules in install tasks by manipulating sys.path, for real life examples see Instant Translate and Mouse Wheel Scrolling
• User uninstalls this add-on, restarts NVDA, and tries to install the new version

In this scenario installation fails since the imported module from the old version is used. A slight variation of this scenario is to try to install both Instant Translate and Mouse Wheel Scrolling the add-on installed last would fail to be installed, since they both bundle module named donate_dialog but with different functions

Description of user facing changes

Add-ons are successfully installed in all scenarios described above.

Description of development approach

All these problems exist because when importing modules in Python if module with the same name exists in sys.modules it is just used without importing it again. To ensure modules are always re-imported when needed, the following has been done:

1. When importing modules we no longer use add-on name to create a name space package in which the module would be imported, the last segment of the add-on path is used instead. This ensures that the package for add-on which is installed would just be its name, and for add-on which is pending install it would end with the pending install suffix. This fixes issue 1.
2. loadModule memorizes all modules it imported for a given add-on. After installation / removal is done they're removed from sys.modules. This fixes issue 2.
3. To remove cached modules imported with the standard import command, we store names of all modules imported before installation / removal, perform it, create list of newly imported modules and remove all newly added modules which belong to the add-on from the cache. This fixes issue 3.

Testing strategy:

Executed scenarios from above. Inspected sys.modules afterwards to make sure no add-on modules remained after add-on removal, and that after installation install tasks are also not present in the modules cache.

Known issues with pull request:

I've decided not to add a change log entry since issues fixed by this PR occur pretty rarely (I have encountered them only because I have been installing / removing the same add-ons over and over without restarting NVDA when working on another PRs) and describing what exactly has been fixed in a concise way seems difficult here.

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.

[lukaszgo1]

@LeonarddeR Would you be able to verify if the changes to loadModule introduced here will not regress your use cases?

[seanbudd]

Thanks @lukaszgo1 - should I wait until testing from @LeonarddeR before merging?

[lukaszgo1]

should I wait until testing from @LeonarddeR before merging?

I've pinged them just because I don't know anyone else who is using loadModule for anything else than loading install tasks.
If you're happy with the approach this can probably be merged now, as these changes are very low risk.

[LeonarddeR]

I don't think this code will introduce regressions for my use cases.
That said, the namespace package structure allows you to do from addons.addonName import something. While that is not documented somewhere I believe, it's a pitty that this would break after this pr.

[lukaszgo1]

That said, the namespace package structure allows you to do from addons.addonName import something. While that is not documented somewhere I believe, it's a pitty that this would break after this pr.

This way of importing is new to me. Unless I missed something it can work only if you've imported at least one module from your add-on using loadModule right? If so this will still work - imported modules are cleaned-up only after given add-on is removed or installed, not after each time loadModule is used.

[LeonarddeR]

This is correct indeed.

[LeonarddeR]

I've decided not to add a change log entry since issues fixed by this PR occur pretty rarely (I have encountered them only because I have been installing / removing the same add-ons over and over without restarting NVDA when working on another PRs) and describing what exactly has been fixed in a concise way seems difficult here.

I guess something like the following should suffice:

• Fixed a rare issue where installing an add-on could go wrong when that add-on was removed before.