1880215 - "prefs.js" fills up with 'temporary-addon' entries

Status: ASSIGNED

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

Description

[Andrew Larson]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:122.0) Gecko/20100101 Firefox/122.0

Steps to reproduce:

0. Prepare with some unpacked example extension on the side
1. In FireFox, go to about:debugging#/runtime/this-firefox
2. Click on "Load Temporary Add-on..."
3. Select an unpacked extension
4. Note the contents of "prefs.js" in the profile (at "extensions.webextensions.uuids")
5. Restart the browser
6. Note the contents of "prefs.js" in the profile (at "extensions.webextensions.uuids")

Actual results:

The file (from 4 to 6) keeps adding on more 'temporary-addon' uuid entries.

Expected results:

The file (from 4 to 6) should be removing entries, just the same as when you remove a temporary addon manually.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'DevTools::about:debugging' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

We are reproducing this. When removing the addon from about:debugging, the entry is removed from the prefs.js file, but not when restarting Firefox.

Hello Luca, would you know if this should be the responsability of the Addon code that already takes care of removing the temporary addon?

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #2)

We are reproducing this. When removing the addon from about:debugging, the entry is removed from the prefs.js file, but not when restarting Firefox.

Hello Luca, would you know if this should be the responsability of the Addon code that already takes care of removing the temporary addon?

yes, clearing up the mappings from that pref is not a responsability of the about:debugging internals, that map is managed from inside Extension.sys.mjs (defined here) and the ExtensionAddonObserver onUninstalled listener is where the entries from that map are currently being cleared (see here).

I'm moving this issue to WebExtensions :: General, where it belongs belongs in practice. It is not unlikely that when the temporarily addon is not explicitly uninstalled the ExtensionAddonObserver is not being notified and doesn't clear the temporarily installed addon from the map (and so it is also likely missing the rest of the cleanup that the ExtensionAddonObserver onUninstalled listener is meant to be clearing).

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1880215 - Emit onUninstalled event for temporarily addons cleaned up during shutdown. r?robwu!

The cleanup of the temporarily installed extension that may still be around when Firefox is shutting down
is being done from the XPIProvider method cleanupTemporaryAddons.

The cleanupTemporaryAddons method does remove the addon from the location and calls either bootstrap update or
uninstall (update if uninstalling the temporarily installed addon should reveal the same addon installed in
another location or uninstall if there isn't another instance of the addon to reveal from the other locations),
but it will not be calling the onUninstalled AOM addon listeners and so the ExtensionAddonObserver is not going
to be notified of the removal and will not be cleaning the uuid-addonid mapping stored in the prefs (nor cleaning
up other data associated with the temporarily installed addon in the profile).

As additional side notes:

• there is a chance that a crash may prevent the cleanup on shutdown to be run or completed, I haven't digged yet
  into that corner case to determine if there would be some more changes needed (or a different approach)

• The onUninstalled is not the only addon listener that is not going to be called and so there may be other
  things that may not be done for temporarily extensions in response of those events (e.g. Bug 1671681 was filed
  for other ones that are not emitted by temporarily addons), and so this patch (at least in its initial form)
  wouldn't solve other issues that are not hit because of missing calls to the onUninstalled addon listeners.

In the meantime I'm attaching this patch as is (and with only basic test coverage) for a preliminary feedback
from the perspective on a second peer.