Remove add-on modules imported in install tasks to avoid conflicts be…

…tween add-ons (nvaccess#15967)

Fixes regression from PR nvaccess#14481.
Discovered due to intermittent failures occurring when installing add-ons, when working on nvaccess#15852 and nvaccess#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 nvaccess#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:

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

requirements.txt

@@ -13,6 +13,9 @@ Pillow==10.0.1 -C "zlib=disable" -C "jpeg=disable"
 #NVDA_DMP requires diff-match-patch
 fast_diff_match_patch==2.0.1
 
+# typing_extensions are required for specifying default value for `TypeVar`, which is not yet possible with any released version of Python (see PEP 696)
+typing-extensions==4.9.0 
+
 # Packaging NVDA
 git+https://github.com/py2exe/py2exe@4e7b2b2c60face592e67cb1bc935172a20fa371d#egg=py2exe
 

source/addonHandler/__init__.py

@@ -4,6 +4,8 @@
 # This file is covered by the GNU General Public License.
 # See the file COPYING for more details.
 
+from __future__ import annotations  # Avoids quoting of forward references
+
 from abc import abstractmethod, ABC
 import sys
 import os.path
@@ -19,6 +21,7 @@
 from typing import (
 	Callable,
 	Dict,
+	Literal,
 	Optional,
 	Set,
 	TYPE_CHECKING,
@@ -421,25 +424,35 @@ def getAvailableAddons(
 	return (addon for addon in _availableAddons.values() if not filterFunc or filterFunc(addon))
 
 
-def installAddonBundle(bundle: "AddonBundle") -> "Addon":
+def installAddonBundle(bundle: AddonBundle) -> Addon | None:
 	""" Extracts an Addon bundle in to a unique subdirectory of the user addons directory,
 	marking the addon as needing 'install completion' on NVDA restart.
 	"""
-	bundle.extract()
-	addon = Addon(bundle.pendingInstallPath)
-	# #2715: The add-on must be added to _availableAddons here so that
-	# translations can be used in installTasks module.
-	_availableAddons[addon.path]=addon
+	addon: Addon | None = None
 	try:
-		addon.runInstallTask("onInstall")
-	except:
-		log.error("task 'onInstall' on addon '%s' failed"%addon.name,exc_info=True)
-		del _availableAddons[addon.path]
-		addon.completeRemove(runUninstallTask=False)
-		raise AddonError("Installation failed")
-	state[AddonStateCategory.PENDING_INSTALL].add(bundle.manifest['name'])
-	state.save()
-	return addon
+		bundle.extract()
+		addon = Addon(bundle.pendingInstallPath)
+		# #2715: The add-on must be added to _availableAddons here so that
+		# translations can be used in installTasks module.
+		_availableAddons[addon.path] = addon
+		try:
+			addon.runInstallTask("onInstall")
+			# Broad except used, since we can not know what exceptions might be thrown by the install tasks.
+		except Exception:
+			log.error(f"task 'onInstall' on addon '{addon.name}' failed", exc_info=True)
+			del _availableAddons[addon.path]
+			addon.completeRemove(runUninstallTask=False)
+			raise AddonError("Installation failed")
+		state[AddonStateCategory.PENDING_INSTALL].add(bundle.manifest['name'])
+		state.save()
+	finally:
+		# Regardless if installation failed or not, the created add-on object should be returned
+		# to give caller a hance to clean-up modules imported as part of install tasks.
+		# This clean-up cannot be done here, as install tasks are blocking,
+		# and this function returns as soon as they're started,
+		# so removing modules before they're done may cause unpredictable effects.
+		return addon
+
 
 class AddonError(Exception):
 	""" Represents an exception coming from the addon subsystem. """
@@ -500,6 +513,8 @@ def __init__(self, path: str):
 		"""
 		self.path = path
 		self._extendedPackages = set()
+		self._importedAddonModules: list[str] = []
+		self._modulesBeforeInstall: set[str] = set()
 		manifest_path = os.path.join(path, MANIFEST_FILENAME)
 		with open(manifest_path, 'rb') as f:
 			translatedInput = None
@@ -557,6 +572,7 @@ def completeRemove(self, runUninstallTask: bool = True) -> None:
 				log.error("task 'onUninstall' on addon '%s' failed"%self.name,exc_info=True)
 			finally:
 				del _availableAddons[self.path]
+				self._cleanupAddonImports()
 		tempPath=tempfile.mktemp(suffix=DELETEDIR_SUFFIX,dir=os.path.dirname(self.path))
 		try:
 			os.rename(self.path,tempPath)
@@ -661,34 +677,49 @@ def loadModule(self, name: str) -> ModuleType:
 			raise ValueError(f"{name} is an invalid python module name")
 		log.debug(f"Importing module {name} from plugin {self!r}")
 		# Create a qualified full name to avoid modules with the same name on sys.modules.
-		fullName = f"addons.{self.name}.{name}"
+		# Since the same add-on can be installed and its new version can be pending installation
+		# we cannot rely on add-on name being unique.
+		# To avoid conflicts, last part of the add-on's path is used
+		# i.e. the directory name whose suffix is different between add-ons installed and add-ons pending install.
+		# Since dot is used as a separator between add-on name and state suffix,
+		# all dots in the name are replaced with underscores.
+		addonPkgName = f"addons.{os.path.split(self.path)[-1].replace('.', '_')}"
+		fullName = f"{addonPkgName}.{name}"
 		# If the given name contains dots (i.e. it is a submodule import),
 		# ensure the module at the top of the hierarchy is created correctly.
 		# After that, the import mechanism will be able to resolve the submodule automatically.
 		splitName = name.split('.')
-		fullNameTop = f"addons.{self.name}.{splitName[0]}"
+		fullNameTop = f"{addonPkgName}.{splitName[0]}"
 		if fullNameTop in sys.modules:
 			# The module can safely be imported, since the top level module is known.
-			return importlib.import_module(fullName)
+			mod = importlib.import_module(fullName)
+			self._importedAddonModules.append(fullName)
+			return mod
 		# Ensure the new module is resolvable by the import system.
 		# For this, all packages in the tree have to be available in sys.modules.
 		# We add mock modules for the addons package and the addon itself.
 		# If we don't do this, namespace packages can't be imported correctly.
-		for parentName in ("addons", f"addons.{self.name}"):
+		for parentName in ("addons", addonPkgName):
 			if parentName in sys.modules:
 				# Parent package already initialized
 				continue
-			parentSpec = importlib._bootstrap.ModuleSpec(parentName, None, is_package=True)
+			parentSpec = importlib.machinery.ModuleSpec(parentName, None, is_package=True)
 			parentModule = importlib.util.module_from_spec(parentSpec)
 			sys.modules[parentModule.__name__] = parentModule
+			self._importedAddonModules.append(parentModule.__name__)
 		spec = importlib.machinery.PathFinder.find_spec(fullNameTop, [self.path])
 		if not spec:
 			raise ModuleNotFoundError(f"No module named {name!r}", name=name)
 		mod = importlib.util.module_from_spec(spec)
 		sys.modules[fullNameTop] = mod
+		self._importedAddonModules.append(fullNameTop)
 		if spec.loader:
 			spec.loader.exec_module(mod)
-		return mod if fullNameTop == fullName else importlib.import_module(fullName)
+		if fullNameTop == fullName:
+			return mod
+		importedMod = importlib.import_module(fullName)
+		self._importedAddonModules.append(fullName)
+		return importedMod
 
 	def getTranslationsInstance(self, domain='nvda'):
 		""" Gets the gettext translation instance for this add-on.
@@ -700,11 +731,17 @@ def getTranslationsInstance(self, domain='nvda'):
 		localedir = os.path.join(self.path, "locale")
 		return gettext.translation(domain, localedir=localedir, languages=[languageHandler.getLanguage()], fallback=True)
 
-	def runInstallTask(self,taskName,*args,**kwargs):
+	def runInstallTask(
+			self,
+			taskName: Literal["onInstall", "onUninstall"],
+			*args,
+			**kwargs
+	) -> None:
 		"""
 		Executes the function having the given taskName with the given args and kwargs,
 		in the add-on's installTasks module if it exists.
 		"""
+		self._modulesBeforeInstall = set(sys.modules.keys())
 		if not hasattr(self,'_installTasksModule'):
 			try:
 				installTasksModule = self.loadModule('installTasks')
@@ -716,6 +753,17 @@ def runInstallTask(self,taskName,*args,**kwargs):
 			if func:
 				func(*args,**kwargs)
 
+	def _cleanupAddonImports(self) -> None:
+		for modName in self._importedAddonModules:
+			log.debug(f"removing imported add-on module {modName}")
+			del sys.modules[modName]
+		self._importedAddonModules.clear()
+		for modName in set(sys.modules.keys()) - self._modulesBeforeInstall:
+				module = sys.modules[modName]
+				if module.__file__ and module.__file__.startswith(self.path):
+					log.debug(f"Removing module {module} from cache of imported modules")
+					del sys.modules[modName]
+
 	def getDocFilePath(self, fileName: Optional[str] = None) -> Optional[str]:
 		r"""Get the path to a documentation file for this add-on.
 		The file should be located in C{doc\lang\file} inside the add-on,
@@ -749,8 +797,8 @@ def getDocFilePath(self, fileName: Optional[str] = None) -> Optional[str]:
 	def isPendingInstall(self) -> bool:
 		return super().isPendingInstall and self.pendingInstallPath == self.path
 
-	def __repr__(self):
-		return f"{self.__class__.__name__} ({self.name!r}, running={self.isRunning!r})"
+	def __repr__(self) -> str:
+		return f"{self.__class__.__name__} ({self.name!r} at path {self.path!r}, running={self.isRunning!r})"
 
 
 def getCodeAddon(obj=None, frameDist=1):

source/addonStore/install.py

@@ -76,18 +76,18 @@ def installAddon(addonPath: PathLike) -> None:
 	@note See also L{gui.addonGui.installAddon}
 	@raise DisplayableError on failure
 	"""
-	from addonHandler import AddonError, installAddonBundle
+	import addonHandler
 	from gui.message import DisplayableError
 
 	addonPath = cast(str, addonPath)
 	bundle = _getAddonBundleToInstallIfValid(addonPath)
 	prevAddon = _getPreviouslyInstalledAddonById(bundle)
 
 	try:
-		systemUtils.ExecAndPump(installAddonBundle, bundle)
+		addonObj = systemUtils.ExecAndPump[addonHandler.Addon](addonHandler.installAddonBundle, bundle).funcRes
 		if prevAddon:
 			prevAddon.requestRemove()
-	except AddonError:  # Handle other exceptions as they are known
+	except addonHandler.AddonError:  # Handle other exceptions as they are known
 		log.error("Error installing addon bundle from %s" % addonPath, exc_info=True)
 		raise DisplayableError(
 			displayMessage=pgettext(
@@ -97,3 +97,6 @@ def installAddon(addonPath: PathLike) -> None:
 				"Failed to install add-on from %s"
 			) % addonPath
 		)
+	finally:
+		if addonObj is not None:
+			addonObj._cleanupAddonImports()

source/gui/addonGui.py

@@ -219,7 +219,7 @@ def doneAndDestroy(window):
 	try:
 		# Use context manager to ensure that `done` and `Destroy` are called on the progress dialog afterwards
 		with doneAndDestroy(progressDialog):
-			systemUtils.ExecAndPump(addonHandler.installAddonBundle, bundle)
+			addonObj = systemUtils.ExecAndPump[addonHandler.Addon](addonHandler.installAddonBundle, bundle).funcRes
 			if prevAddon:
 				from addonStore.dataManager import addonDataManager
 				assert addonDataManager
@@ -236,6 +236,9 @@ def doneAndDestroy(window):
 			_("Error"),
 			wx.OK | wx.ICON_ERROR
 		)
+	finally:
+		if addonObj is not None:
+			addonObj._cleanupAddonImports()
 	return False
 
 

source/systemUtils.py

@@ -18,8 +18,16 @@
 	windll,
 )
 from typing import (
-	Any,
+	Generic,
+	Optional,
 )
+from typing_extensions import (
+	# Uses `TypeVar` from `typing_extensions`, to be able to specify default type.
+	# This should be changed to use version from `typing`
+	# when updating to version of Python supporting PEP 696.
+	TypeVar,
+)
+
 import winKernel
 import winreg
 import shellapi
@@ -205,15 +213,22 @@ def _isSystemClockSecondsVisible() -> bool:
 		return False
 
 
-class ExecAndPump(threading.Thread):
+_execAndPumpResT = TypeVar("_execAndPumpResT", default=None)
+
+
+class ExecAndPump(threading.Thread, Generic[_execAndPumpResT]):
 	"""Executes the given function with given args and kwargs in a background thread,
 	while blocking and pumping in the current thread.
 	"""
 
-	def __init__(self, func: Callable[..., Any], *args, **kwargs) -> None:
+	def __init__(self, func: Callable[..., _execAndPumpResT], *args, **kwargs) -> None:
 		self.func = func
 		self.args = args
 		self.kwargs = kwargs
+		# Intentionally uses older syntax with `Optional`, instead of `_execAndPumpResT | None`,
+		# as latter is not yet supported for unions potentially containing two instances of `None`
+		# (see CPython issue 107271).
+		self.funcRes: Optional[_execAndPumpResT] = None
 		fname = repr(func)
 		super().__init__(
 			name=f"{self.__class__.__module__}.{self.__class__.__qualname__}({fname})"
@@ -233,7 +248,7 @@ def __init__(self, func: Callable[..., Any], *args, **kwargs) -> None:
 
 	def run(self):
 		try:
-			self.func(*self.args, **self.kwargs)
+			self.funcRes = self.func(*self.args, **self.kwargs)
 		except Exception as e:
 			self.threadExc = e
 			log.debugWarning("task had errors", exc_info=True)