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

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 syntacs 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)