Closed Bug 1473837 Opened 2 years ago Closed 11 months ago
Firefox can not remove the extension XPI from OS temporary folder
Bug 1473837 - AddonInstall should flush the jar cache for the temporary xpi file before trying to remove it.
47 bytes, text/x-phabricator-request
|Details | Review|
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0 Build ID: 20180621125625 Steps to reproduce: install the extension, for example: https://addons.mozilla.org/en-US/firefox/addon/twitch_5/ Actual results: open the browser console. it contains: 1530867882557 addons.xpi WARN Failed to remove temporary file D:\Temp\tmp-y3n.xpi for addon https://addons.mozilla.org/firefox/downloads/file/1004981/alternate_player_for_twitchtv_twitch_5-2018.7.3-an+fx.xpi: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.remove]" nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)" location: "JS frame :: resource://gre/modules/addons/XPIInstall.jsm :: removeTemporaryFile :: line 1729" data: no] Stack trace: removeTemporaryFile()@resource://gre/modules/addons/XPIInstall.jsm:1729 startInstall/<()@resource://gre/modules/addons/XPIInstall.jsm:2061 restart the browser. the file D:\Temp\tmp-y3n.xpi is still exists. this also happens on extension update. my temporary folder holds tens of temporary XPIs. Expected results: after installation, temporary files should be deleted.
the folder has permission for delete files for all users on my computer.
Do you have an active backup program or virus scanner that has a lock on this file?
none of these programs is installed
Can we please see if we can reproduce this? It's the only report, and it sounds OS-related, but this is the first step.
actually 2 reports. user of my webextension sent me console screenshot with the identical error.
The issue is reproducible by installing the extension from the example having the console open: (WARN Failed to remove temporary file C:\Users\VLAD~1.JIM\AppData\Local\Temp\tmp-mtl.xpi for addon https://addons.mozilla.org/firefox/downloads/file/1022106) The file is still present after restarting the browser. Reproduced using Firefox 63.0a1 (20180723220051), Firefox 61.0.1 (20180704003137) and Firefox 62.0b9 (20180713213322) running on Win 10 x64.
I just tried to reproduce this on a recent Nightly running on Windows 10 and dig into it a bit more, and this seems to be reproducible consistently. As we suspected it is not an issue specific to a particular extension xpi file, I've been able to reproduce the same issue by installing any other extensions from AMO. The NS_ERROR_FILE_ACCESS_DENIED error seems to be raised because the file is still open, and it seems that it is kept open by Firefox itself (I briefly verified it by using ProcessExplorer, https://docs.microsoft.com/en-us/sysinternals/downloads/process-explorer, and also by trying to remove the file as a user from the file browser, which fails until that Firefox instance is not running anymore). In my opinion, we are currently keeping the file open because we open the archive file to retrieve the addon metadata (which are then used in the install doorhanger): - https://searchfox.org/mozilla-central/search?q=loadManifest%28this.file%29&path=toolkit%2Fmozapps%2Fextensions and so, to "free" this open file, it seems that we should be sure to "flush the jar cache" for this file at least right before we try to remove it here: - https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/toolkit/mozapps/extensions/internal/XPIInstall.jsm#1499 (e.g. using `Services.obs.notifyObservers(this.file, "flush-cache-entry");` or the `flushJarCache` helper function already defined in 'XPIInstall.jsm') Flushing the jar cache is something that we already do in many cases for the exact same reason (Windows raised the Ns_ERROR_FILE_ACCESS_DENIED otherwise), https://searchfox.org/mozilla-central/search?q=flushJarCache(&case=false®exp=false&path=, but apparently we are not currently flushing the jar cache for this file.
We do flush the jar cache from here: https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/toolkit/mozapps/extensions/internal/XPIInstall.jsm#1575 However, I guess we're flushing it too early since we build a jar: url for the icon to use in the install dialog which re-opens the file. Ugh. Perhaps for the purpose of the AddonInstall object we should read the icon into a blob or even a data url?
(In reply to Andrew Swan [:aswan] from comment #8) > We do flush the jar cache from here: > https://searchfox.org/mozilla-central/rev/ > d47c829065767b6f36d29303d650bffb7c4f94a3/toolkit/mozapps/extensions/internal/ > XPIInstall.jsm#1575 > > However, I guess we're flushing it too early since we build a jar: url for > the icon to use in the install dialog which re-opens the file. Ugh. > Perhaps for the purpose of the AddonInstall object we should read the icon > into a blob or even a data url? Yep, it looks like it, we are re-opening the jar file from getIcon  which is called in the checkPrompt . Turning the icon into a blob or data url from inside getIcon sounds like a good idea, because we could make getIcon responsible to flush the jar cache right after it has retrieved and converted the icon. : https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/toolkit/mozapps/extensions/internal/XPIInstall.jsm#1606-1616 : https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/toolkit/mozapps/extensions/internal/XPIInstall.jsm#1631
Status: UNCONFIRMED → NEW
Ever confirmed: true
I was thinking we would read it in loadManifest() and keep the current flush logic. I'm working under the premise that flushing the jar cache is expensive and we should avoid doing it too often, which is oral tradition rather than something I have specific numbers or documentation to back up...
Attachment #9084271 - Attachment description: Bug 1473837 - Convert AddonInstall jar icon urls into a blob url to avoid keeping the downloaded xpi file locked. r?aswan! → Bug 1473837 - AddonInstall should flush the jar cache for the temporary xpi file before trying to remove it. r?aswan!
Attachment #9084271 - Attachment description: Bug 1473837 - AddonInstall should flush the jar cache for the temporary xpi file before trying to remove it. r?aswan! → Bug 1473837 - AddonInstall should flush the jar cache for the temporary xpi file before trying to remove it.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/c4f020146cf8 AddonInstall should flush the jar cache for the temporary xpi file before trying to remove it. r=kmag
You need to log in before you can comment on or make changes to this bug.