2006512 - An early I/O error during installation of one add-on can nuke the whole staged directory, preventing concurrent installations from succeeding

Status: RESOLVED FIXED

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

Description

[Rob Wu [:robwu]]

Simplified, the relevant code involving the staging directory ("staged") is as follows (source, with many unrelevant lines deleted):

    let stagedAddon = this.location.installer.getStagingDir();
    try {
      await this.unstageInstall(stagedAddon);
      stagedAddon.append(`${this.addon.id}.xpi`);
    } catch (e) {
      if (stagedAddon.exists()) {
        recursiveRemove(stagedAddon);
      }

The problem with this logic is that the logic assumes stagedAddon to be pointing to the xpi file that is being installed.
But if unstageInstall throws for whatever reason (e.g. file being locked), the whole staging directory ends up being nuked instead.

If there are concurrent add-on installations (e.g. during a background update check), this can prevent all other extensions from updating.

This is a subtle regression introduced a long time ago in bug 1231172 by this change.
Previously, stagedAddon was immediately set to an item in the staging directory. That refactor moved the logic in a (then) new unstageInstall method, taking the directory, and no longer mutating the staging directory. Consequently, if any of the intermediate steps failed, the caller would still have stagedAddon at the staging directory.

Comment 1

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

Set release status flags based on info from the regressing bug 1231172

Comment 2

[Rob Wu [:robwu]]

Directory installation was removed in bug 1457072, so a way to fix this could be to remove the directory-deletion logic from https://searchfox.org/firefox-main/rev/f37efeb9fd346125bfc98d132ae0dea48a1e2584/toolkit/mozapps/extensions/internal/XPIInstall.sys.mjs#2111

and restructure the code to avoid passing stagingDir around where addonDir was meant.

and also, this is dead code: https://searchfox.org/firefox-main/rev/f37efeb9fd346125bfc98d132ae0dea48a1e2584/toolkit/mozapps/extensions/internal/XPIInstall.sys.mjs#1126,1131,1244-1247

(I'm looking at this code currently because I'm trying to figure out the best way to fix bug 2006489)

Comment 3

[Rob Wu [:robwu]]

Attachment: Bug 2006512 - Remove obsolete directory cleaning logic

The deleted logic became obsolete in bug 1457072.

Comment 4

[Rob Wu [:robwu]]

The initial version of my patch triggers the following test failure:

TEST-UNEXPECTED-FAIL | xpcshell.toml:toolkit/mozapps/extensions/test/xpcshell/test_undouninstall.js | xpcshell return code: 0
Error: Test cleanup: path /tmp/xpc-profile-ln95givh/extensions/staged exists when it should not at resource://testing-common/AddonTestUtils.sys.mjs:326

Upon further inspection, it turns out that there is still code that creates an empty directory at https://searchfox.org/firefox-main/rev/5917a9f2af3294b27a325371c5c499e7dd9554fd/toolkit/mozapps/extensions/internal/XPIInstall.sys.mjs#5040-5055

That directory-creating code should have been removed in bug 1448221 because part 3 of that patch stack removed the logic that scans the staged directory at https://searchfox.org/firefox-main/diff/ef130246087b29842cc1922b217fc6cf76a66b66/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2744-2745. So there is no point in creating ProfD/extensions/staged/<addon-id> any more.

(In reply to Rob Wu [:robwu] from comment #2)

Directory installation was removed in bug 1457072, so a way to fix this could be to remove the directory-deletion logic from https://searchfox.org/firefox-main/rev/f37efeb9fd346125bfc98d132ae0dea48a1e2584/toolkit/mozapps/extensions/internal/XPIInstall.sys.mjs#2111

The fact that we have continued to create this empty directory for the past 7 years despite its obsolescence means that we cannot remove this directory-deleting logic yet. That's OK, the patch does not rely on it anyway to fix this bug.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/b4bef78a6d09
https://hg.mozilla.org/integration/autoland/rev/757b89bb3bfb
Remove obsolete directory cleaning logic r=zombie

Comment 6

[amarc]

https://hg.mozilla.org/mozilla-central/rev/757b89bb3bfb

Comment 7

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

The patch landed in nightly and beta is affected.
:robwu, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox148 to wontfix.

For more information, please visit BugBot documentation.