1761093 - Add unit tests for dictionary updating and unloading

Status: NEW

(Toolkit :: Add-ons Manager, task, P3)

Description

[Rob Wu [:robwu]]

The only unit test for dictionary registration in the addon manager is at https://searchfox.org/mozilla-central/rev/630b197be00e3df3f2e3fe995bf31ca50d146eaa/toolkit/mozapps/extensions/test/xpcshell/test_dictionary_webextension.js.

It tests the dictionary integration (spellCheck) by loading and unloading dictionary add-ons at runtime in the tests.

There is logic that restores the built-in dictionary when a non-builtin dictionary is uninstalled, at https://searchfox.org/mozilla-central/rev/630b197be00e3df3f2e3fe995bf31ca50d146eaa/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2415-2432.

... but I don't see any unit tests for that scenario.
The logic is not trivial:

1. Built-in dictionaries are registered very early in XPIProvider startup.
2. Dictionary WebExtensions are registered via the spellCheck API in the startup method of the Dictionary in Extension.jsm.
3. Dictionaries are unregistered by shutdown of Dictionary in Extension.jsm, with a call to XPIProvider.unregisterDictionaries.

• Dictionary's shutdown is called by DictionaryBootstrapScope's shutdown.
• NOTE: Dictionary's shutdown is marked as an async function, but doesn't use await and does not return any Promise either. Since DictionaryBootstrapScope does not use await on the .shutdown() call, any errors thrown by Dictionary's shutdown does not interrupt the add-on shutdown. So if any error were to occur, at most an unhandled rejection is raised.

Note that step 2 registers the dictionary through spellCheck, whereas step 3 unregisters dictionaries via XPIProvider. The current implementation calls XPIProvider.unregisterDictionaries directly, but we should ideally do this via AddonManagerInternal._getProviderByName("XPIProvider") to avoid the direct reference to XPIProvider. In bug 1760146 I refactored the use of XPIProvider in Extension.jsm, but am still accessing unregisterDictionaries via XPIProvider because I am not sure whether it is safe to access it via AddonManagerInternal._getProviderByName, and there are no unit tests for the relevant scenario's either.

E.g. if the dictionary were to somehow start and update during sync startup (checkForChanges may spin an event loop while it's installing extensions, before XPIProvider is marked as started). In that case, it's potentially possible for the dictionary to start before XPIProvider has started, and AddonManagerInternal._getProviderByName("XPIProvider") would then return undefined instead of `XPIProvider.

So, we need to add unit tests for the scenario above, and consider replacing the use of XPIProvider.unregisterDictionaries with AddonManagerInternal._getProviderByName("XPIProvider").unregisterDictionaries.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Thanks for filing this, I looked it as well yesterday. There is one test (not sufficient either) at https://searchfox.org/mozilla-central/source/browser/components/preferences/tests/browser_browser_languages_subdialog.js

Ideally this dictionary handling would be spun out to the spellcheck module. It directly loads "builtin" dictionaries that are not addons. The only reason I see it being in XPIProvider is that the list of builtin dictionaries was placed in the builtin addons file. The only use of the shutdown part is to fall back to a builtin if it was overridden by an addon. That could be handled differently, thus removing this concept from the addon manager altogether.