1180901 - If Firefox fails to remove a file from an addon, it prevents the installation and updating of other unrelated addons

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Trevor Rowbotham [:rowbot]]

The other day I ran into an issue where all addons would fail to update or install.  This issue occurred when both trying to install an addon from the web or using a local file.  In my particular case the issue turned out to be caused by the ADB Helper addon, which is shipped as part of the Firefox devtools.  Unfortunately, I did not think to copy the error message from the Browser Console at the time.  While I am not absolutely certain that these are the correct STR, they seem like the logical ones that would reproduce it.

STR
1) Use an older version of the ADB Helper addon.
2) Use WebIDE to connect to a remote device so that the adb.exe process is launched.
3) Close WebIDE and update the ADB Helper addon to a newer version.  (The adb.exe process doesn't seem to be terminated after closing WebIDE)
4) Try to update or install a different addon.

Actual Results
In my case, after the ADB Helper was updated, it created a trash folder in the extensions directory that contained 3 or 4 files if I remember correctly.
Firefox then fails to remove the trash folder that was created because one of the files in the trash folder is the adb.exe file and it can't be deleted because the process is still running.  The failure to remove the trash folder seems to prevent all other installations because I assume it first attempts to remove any files scheduled to be deleted first, before installing new ones, but an error is thrown when the deletion fails.

Expected Results
Firefox should be able to install and update other addons that are unrelated to the addon that firefox is failing to remove files for.

Comment 1

[Trevor Rowbotham [:rowbot]]

Ok, so I have managed to narrow this down.  The problem is specifically with the directory called trash and the files it contains.  So, what seems to be happening is that if the files contained within the trash directory are in-use, Firefox fails to remove them and just throws errors. Here are some simpler and concrete steps to reproduce:

STR
1) Navigate to your profile folder and open the extensions directory. (On Windows this would be C:\Users\<Username>\AppData\Roaming\Mozilla\Firefox\Profiles\<Profile>\extensions)
2) Create a directory called trash.
3) Inside the trash directory create a new text file.
4) Open the newly created text file.
5) Launch Firefox and go to the Addons Manager.
6) Attempt to install/update any addon.

Actual Results
Addon fails to install.  Browser console shows the following errors:

1436237207623	addons.xpi	ERROR	Failed to remove empty directory C:\Users\Trevor\AppData\Roaming\Mozilla\Firefox\Profiles\euuy2vs8.test42\extensions\trash: [Exception... "Component returned failure code: 0x8052000e (NS_ERROR_FILE_IS_LOCKED) [nsIFile.remove]"  nsresult: "0x8052000e (NS_ERROR_FILE_IS_LOCKED)"  location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm :: recursiveRemove :: line 1540"  data: no] Stack trace: recursiveRemove()@resource://gre/modules/addons/XPIProvider.jsm:1540 < DirInstallLocation_getTrashDir()@resource://gre/modules/addons/XPIProvider.jsm:7532 < DirInstallLocation_installAddon()@resource://gre/modules/addons/XPIProvider.jsm:7554 < AI_startInstall/<()@resource://gre/modules/addons/XPIProvider.jsm:6007 < next()@self-hosted:673 < TaskImpl_run()@resource://gre/modules/Task.jsm:330 < Handler.prototype.process()@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:920 < this.PromiseWalker.walkerLoop()@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:799 < this.PromiseWalker.scheduleWalkerLoop/<()@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:738 < <file:unknown> Log.jsm:749:0

1436237207624	addons.xpi	WARN	Failed to install C:\Users\Trevor\Downloads\uBlock0.firefox.xpi from file:///C:/Users/Trevor/Downloads/uBlock0.firefox.xpi: [Exception... "Component returned failure code: 0x8052000e (NS_ERROR_FILE_IS_LOCKED) [nsIFile.remove]"  nsresult: "0x8052000e (NS_ERROR_FILE_IS_LOCKED)"  location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm :: recursiveRemove :: line 1540"  data: no] Stack trace: recursiveRemove()@resource://gre/modules/addons/XPIProvider.jsm:1540 < DirInstallLocation_getTrashDir()@resource://gre/modules/addons/XPIProvider.jsm:7532 < DirInstallLocation_installAddon()@resource://gre/modules/addons/XPIProvider.jsm:7554 < AI_startInstall/<()@resource://gre/modules/addons/XPIProvider.jsm:6007 < next()@self-hosted:673 < TaskImpl_run()@resource://gre/modules/Task.jsm:330 < Handler.prototype.process()@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:920 < this.PromiseWalker.walkerLoop()@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:799 < this.PromiseWalker.scheduleWalkerLoop/<()@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:738 < <file:unknown>

1436237207626	addons.xpi	WARN	removeActiveInstall: could not find active install for file:///C:/Users/Trevor/Downloads/uBlock0.firefox.xpi

Comment 2

[Trevor Rowbotham [:rowbot]]

I should also mention that this is happening on Windows 7 using the latest nightly.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Looks like recursiveRemove(trashDir) is almost always invoked in a try catch, but in getTrashDir, that is what _installAddons invokes http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#7540

The stack seems to confirm that

Comment 4

[Trevor Rowbotham [:rowbot]]

What's happening here is that within the trash directory, we have a file that is locked because it is in-use.  This only appears to be an issue in Windows, as I could not reproduce it on my Ubuntu system and according to the internet both Mac and Linux do not have the concept of locked files because they are "in-use".  recursiveRemove attempts to remove the files in the trash directory, but fails because one or more files are locked.  It then tries to remove the trash directory, but fails because it contains a locked file.  Then, we try to create a new trash directory, but fail because the trash directory already exists.

Comment 5

[Trevor Rowbotham [:rowbot]]

Attachment: bug1180901_handle_error_removing_locked_file.diff

If you are not the right person to review this, please feel free to point it to someone else.

This just wraps the recursiveRemove function in a try catch block and checks to make sure that no trash directory exists before attempting to create one.  This fixes the problem locally for me.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8633687 [details] [diff] [review]
bug1180901_handle_error_removing_locked_file.diff

Review of attachment 8633687 [details] [diff] [review]:
-----------------------------------------------------------------

Is this urgent? Mossop would be the best person to look at this but he's on PTO until 21st.

I have no problem with the change itself, but I don't know if being unable to remove the folder at that point could create some chain-reaction further down in the code.

I can give some js style hints in the meanwhile.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +7541,5 @@
> +      try {
> +        recursiveRemove(trashDir);
> +      } catch (e) {
> +        logger.warn("Failed to remove trash directory", e);
> +      }

you should always brace the ifs when their bodies are multi-line

@@ +7542,5 @@
> +        recursiveRemove(trashDir);
> +      } catch (e) {
> +        logger.warn("Failed to remove trash directory", e);
> +      }
> +    if (!trashDir.exists())

even if the OS filesystem cache will likely make this cheap, I'd prefer to avoid executing exists() twice.
I'd rather do something like

let trashFolderExists = trashDir.exists();
try {
  ...
  trashFolderExists = false;
} catch (e) {
  ...
}
if (!trashFolderExists)
  create...

Comment 7

[Trevor Rowbotham [:rowbot]]

No, this is not urgent.  I just figured I would take a stab at it since it got flagged backlog+ and I didn't think anyone else would be taking it for a while.  I'll wait for Mossop to come back before I continue here.  Thanks for your feedback!

Comment 8

[Trevor Rowbotham [:rowbot]]

Attachment: bug1180901_handle_error_removing_locked_file v2

This addresses the feedback from :mak.

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 8640687 [details] [diff] [review]
bug1180901_handle_error_removing_locked_file v2

Review of attachment 8640687 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +7549,5 @@
> +    } catch (e) {
> +      logger.warn("Failed to remove trash directory", e);
> +    }
> +    if (!trashDirExists)
> +      trashDir.create(Ci.nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);

You missed the bit from mak's feedback about setting trashDirExists to false, I'd expect this to break things as-is. I once working this leaves us in a state where attempting to upgrade an add-on that is already locked in the trash directory fails with a file I/O error?

Comment 10

[Trevor Rowbotham [:rowbot]]

> You missed the bit from mak's feedback about setting trashDirExists to false, I'd expect this to break things as-is.
I purposely left that bit out because setting |trashDirExists = false| would cause |if (!trashDirExists)| to evaluate to true, which causes us to attempt to create a new trash directory, which would then throw an error because the trash directory already exists.

> I once working this leaves us in a state where attempting to upgrade an add-on that is already locked in the trash directory fails with a file I/O error?
Correct, however, that is really no different from how things work right now since it already fails in this situation.  This patch just allows other add-ons to be installed/updated/uninstalled as long as they don't mess with the locked file.  Technically, with this patch the same add-on that has locked files in the trash directory could still be updated as long as it doesn't attempt to move a file by the same name in to the trash directory during its next update, although installing and uninstalling would still probably fail for that same add-on until the locked files are removed.

Alternatively, instead of trying to patch the issue here, we could see if the dev tools team would be willing to kill the adb.exe process when you close WebIDE, which would fix the specific scenario that I ran into.  Although, that doesn't help if some other add-on works the same way.  I would say that this is probably limited to add-ons that use executables, which are fairly rare, except for in the case of the dev tools add-ons that get shipped with Firefox.

Comment 11

[Dave Townsend [:mossop]]

(In reply to Trevor Rowbotham from comment #1)
> 1436237207623	addons.xpi	ERROR	Failed to remove empty directory
> C:\Users\Trevor\AppData\Roaming\Mozilla\Firefox\Profiles\euuy2vs8.
> test42\extensions\trash: [Exception... "Component returned failure code:
> 0x8052000e (NS_ERROR_FILE_IS_LOCKED)

I'm a little confused that you get this error for a locked file in the trash dir. I'd expect us to fail to delete the locked file but this exception is thrown when failing to remove an empty directory. Did the file get removed when you tested this?

(In reply to Trevor Rowbotham from comment #10)
> > You missed the bit from mak's feedback about setting trashDirExists to false, I'd expect this to break things as-is.
> I purposely left that bit out because setting |trashDirExists = false| would
> cause |if (!trashDirExists)| to evaluate to true, which causes us to attempt
> to create a new trash directory, which would then throw an error because the
> trash directory already exists.

If trashDirExists is true then we remove it. Assuming that works it is gone and when |if (!trashDirExists)| evaluates to false we don't create a new trash dir. You would need to set trashDirExists to false after recursiveRemove succeeds to correctly reflect the new state.

I don't have Windows to check but I think the fix here isn't complete. With a single locked file in the trash I'd expect us to recurse through the trash until we hit the locked file then fail to remove that file and everything after it in the directory tree meaning the trash directory will start to get clogged up with things other than the locked file.

The better fix is probably to delete everything inside the trash dir, wrapping each in a try...catch so only the single directory or locked file remains. Then we don't need to recreate the trash directory in that case.

I'd like to see a test here to verify that the fix is doing what we expect. We can lock files from JavaScript ok, though I'd expect this only to work on Windows as you say: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_locked.js?force=1#260

Comment 12

[Trevor Rowbotham [:rowbot]]

I just did some additional testing to try and answer your questions.

> I'm a little confused that you get this error for a locked file in the trash dir. I'd expect us to fail to delete the
> locked file but this exception is thrown when failing to remove an empty directory. Did the file get removed when you
> tested this?
Visually, at least, the locked dummy text file that I am creating gets removed from the trash directory.  However, it seems that the filesystem is still holding a reference to the file since I have that text file open.  So, it seems as though the filesystem reports that the file was deleted successfully, but it fails to remove the trash directory since the filesystem is still holding a reference to the file.  This would explain why we get this error for removing an "empty" directory.

> With a single locked file in the trash I'd expect us to recurse through the trash until we hit the locked file then fail
> to remove that file and everything after it in the directory tree meaning the trash directory will start to get clogged
> up with things other than the locked file.
As with the explanation above, it seems as though Windows thinks that it has successfully deleted the file.  So, we don't seem to be getting a backlog of undeleted files as it deleted all the extra dummy files I created both before and after the file that should cause problems.

> If trashDirExists is true then we remove it. Assuming that works it is gone and when |if (!trashDirExists)| evaluates to
> false we don't create a new trash dir.
Ah, yeah, my mind was still thinking that I was calling trashDir.exists() a second time and not the cached value.  That's a fail on my part.

> The better fix is probably to delete everything inside the trash dir, wrapping each in a try...catch so only the single
> directory or locked file remains. Then we don't need to recreate the trash directory in that case.
This already appears to be the case: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm?from=XPIProvider.jsm#1523

I know that originally, unlike my test with text files, Firefox was failing to delete the adb.exe file in the trash directory because I went and looked in the folder after I saw the error messages.  So, I don't know why that is.

Comment 13

[Trevor Rowbotham [:rowbot]]

If I put a standalone executable file in the trash folder, run it, and then try to install an add-on, I get the following:

1438309113732	addons.xpi	ERROR	Failed to remove file c:\mozilla\mozilla-central\obj-i686-pc-mingw32\tmp\scratch_user\extensions\trash\GPU-Z.0.8.5.exe: [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/XPIProvider.jsm :: recursiveRemove :: line 1523"  data: no] Stack trace: recursiveRemove()@resource://gre/modules/addons/XPIProvider.jsm:1523 < forEach()@self-hosted:210 < recursiveRemove()@resource://gre/modules/addons/XPIProvider.jsm:1538 < DirInstallLocation_getTrashDir()@resource://gre/modules/addons/XPIProvider.jsm:7538 < DirInstallLocation_installAddon()@resource://gre/modules/addons/XPIProvider.jsm:7560 < AI_startInstall/<()@resource://gre/modules/addons/XPIProvider.jsm:6013 < next()@self-hosted:673 < TaskImpl_run()@resource://gre/modules/Task.jsm:330 < Handler.prototype.process()@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:934 < this.PromiseWalker.walkerLoop()@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813 < <file:unknown> Log.jsm:749:0

1438309113734	addons.xpi	WARN	Failed to install C:\Users\Trevor\Downloads\uBlock0.firefox.xpi from file:///C:/Users/Trevor/Downloads/uBlock0.firefox.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/XPIProvider.jsm :: recursiveRemove :: line 1523"  data: no] Stack trace: recursiveRemove()@resource://gre/modules/addons/XPIProvider.jsm:1523 < forEach()@self-hosted:210 < recursiveRemove()@resource://gre/modules/addons/XPIProvider.jsm:1538 < DirInstallLocation_getTrashDir()@resource://gre/modules/addons/XPIProvider.jsm:7538 < DirInstallLocation_installAddon()@resource://gre/modules/addons/XPIProvider.jsm:7560 < AI_startInstall/<()@resource://gre/modules/addons/XPIProvider.jsm:6013 < next()@self-hosted:673 < TaskImpl_run()@resource://gre/modules/Task.jsm:330 < Handler.prototype.process()@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:934 < this.PromiseWalker.walkerLoop()@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813 < <file:unknown>

1438309113736	addons.xpi	WARN	removeActiveInstall: could not find active install for file:///C:/Users/Trevor/Downloads/uBlock0.firefox.xpi

> ERROR	Failed to remove file c:\mozilla\mozilla-central\obj-i686-pc-mingw32\tmp\scratch_user\extensions\trash
> \GPU-Z.0.8.5.exe: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)
This looks more like what you were expecting to happen in regards to your confusion in comment 11, although I was expecting to get a NS_ERROR_FILE_IS_LOCKED instead of NS_ERROR_FILE_ACCESS_DENIED.

It appears that in the case of the executable, we never continue on to attempt to delete the trash directory, although we do still successfully delete all the other files in the trash directory that are not locked, meaning that we still don't get a backlog of undeleted files.

I'll try writing a test to test this.  If you would find it helpful, I can record whats happening or share my screen with you over Firefox Hello or something.

Comment 14

[Trevor Rowbotham [:rowbot]]

Attachment: test WIP

Dave, I am having some trouble integrating the addon manager into my test.  Would you mind taking a look?  Thanks!

Comment 15

[Trevor Rowbotham [:rowbot]]

Attachment: test_log

I know you said you don't have a Windows machine to test this on, so here is the log from a test run.

Comment 16

[Dave Townsend [:mossop]]

Comment on attachment 8644454 [details] [diff] [review]
test WIP

Review of attachment 8644454 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/test/xpcshell/test_bug1180901.js
@@ +7,5 @@
> +
> +  startupManager();
> +  // Make sure we only register once despite multiple calls
> +  AddonManager.addInstallListener(InstallListener);
> +  AddonManager.addAddonListener(AddonListener);

Registering these listeners is unnecessary and is what is causing this test to fail

Comment 17

[Trevor Rowbotham [:rowbot]]

Thanks for the feedback!

Comment 18

[Trevor Rowbotham [:rowbot]]

Attachment: bug1180901_handle_error_removing_locked_file v3

This just sets |trashDirExists = false| when the call to |recursiveRemove()| succeeds as we had previously discussed.

Comment 19

[Trevor Rowbotham [:rowbot]]

Attachment: bug1180901_test_addon_install_with_locked_file.diff

Adds a test for installing an addon when a locked file is present within the trash directory.

Comment 20

[Dave Townsend [:mossop]]

Comment on attachment 8646607 [details] [diff] [review]
bug1180901_test_addon_install_with_locked_file.diff

Review of attachment 8646607 [details] [diff] [review]:
-----------------------------------------------------------------

One thing to be added here, looks fine otherwise.

::: toolkit/mozapps/extensions/test/xpcshell/test_bug1180901.js
@@ +27,5 @@
> +
> +  yield promiseInstallAllFiles([do_get_addon("test_install1")]);
> +
> +  try {
> +    yield promiseRestartManager();

Add another test that the file still exists here, that will confirm for us that the test is testing what it is supposed to.

Comment 21

[Trevor Rowbotham [:rowbot]]

Attachment: bug1180901_test_locked_file_cannot_be_removed.diff

I removed the try/catch statement since it wasn't really doing anything and I added the test to check to make sure that the test file still exists after attempting to install an extension.

Comment 22

[Dave Townsend [:mossop]]

Comment on attachment 8648761 [details] [diff] [review]
bug1180901_test_locked_file_cannot_be_removed.diff

Review of attachment 8648761 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. Do you have access to push to try? We should check this passes ok there before landing it.

Comment 23

[Trevor Rowbotham [:rowbot]]

Yes, I can push to try.  I'll do that now.

Comment 24

[Trevor Rowbotham [:rowbot]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=910d098cae76

Comment 25

[Trevor Rowbotham [:rowbot]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d8f1ab59829

Comment 26

[Trevor Rowbotham [:rowbot]]

Attachment: bug1180901_test_installing_extension_when_locked_file_exists.diff

I mention to you yesterday on IRC that I didn't understand why the onInstallFailed event wasn't being dispatched when I tried to write a test for the patch.  Here is the test that I was trying to write.  I would expect an exception to be thrown with "Extension installation failed" when the promise gets rejected, however, we never get the onInstallFailed event and we always resolve the promise in the onInstallEnded event.  So, whenever you have some free time or get bored, I would appreciate it if you could take a look at this.

Comment 27

[Trevor Rowbotham [:rowbot]]

Attachment: test_log

Here is a log from running the bug1180901_test_installing_extension_when_locked_file_exists.diff patch.  I had to force a failure in the patch to get useful info since the promise was failing to reject.  Based on the logs, it would appear that the extension fails to install.  Manual testing confirms this as I always get a message in about:addons saying that the install failed as well as a doorhanger message stating that installation failed.

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/aba12b202838
https://hg.mozilla.org/integration/fx-team/rev/723699db4d75

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/aba12b202838
https://hg.mozilla.org/mozilla-central/rev/723699db4d75

Comment 30

[Dave Townsend [:mossop]]

(In reply to Trevor Rowbotham from comment #27)
> Created attachment 8649370 [details]
> test_log
> 
> Here is a log from running the
> bug1180901_test_installing_extension_when_locked_file_exists.diff patch.  I
> had to force a failure in the patch to get useful info since the promise was
> failing to reject.  Based on the logs, it would appear that the extension
> fails to install.  Manual testing confirms this as I always get a message in
> about:addons saying that the install failed as well as a doorhanger message
> stating that installation failed.

When installing an add-on there are two parts to the install. The first is downloading and staging the install (in <install-location>/staged). In the second part the extension is moved into the correct place for loading by the platform, this is the bit where we clear the trash dir to move aside any existing extension and where you'll get a failure from a locked file in the trash.

For a restartless add-on the final onInstallEnded notification is sent after all of that completes and I'm assuming that is the case you are testing manually. The test though is installing a non-restartless extension. In this case the onInstallEnded is sent after the first step, when the extension is staged but not installed. You can see that that part succeeds in the test log:

DEBUG	Staged install of [e-mail] from file:///c:/mozilla/mozilla-central/obj-i686-pc-mingw32/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell/addons/test_install1.xpi ready; waiting for restart.

This is why you don't see an onInstallFailed message, because the install doesn't fail until you restart the add-ons manager and there is no events to detect this other than seeing that the add-on didn't get installed after the restart.

Comment 31

[Trevor Rowbotham [:rowbot]]

Attachment: bug1180901_test_installing_extension_when_locked_file_exists v2

Dave, thanks for taking the time to look at the logs and explaining how the event flow works for the different types of extensions.  Here is an updated test that uses a bootstrapped extension so that we can listen for the onInstallFailed event to tell us when this fails.  I confirmed locally that this test passes with the patch in this bug applied and fails without it.

Comment 32

[Dave Townsend [:mossop]]

Comment on attachment 8657517 [details] [diff] [review]
bug1180901_test_installing_extension_when_locked_file_exists v2

Review of attachment 8657517 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks

Comment 33

[Trevor Rowbotham [:rowbot]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe463060232b

Comment 34

[Trevor Rowbotham [:rowbot]]

Sheriffs: Only bug1180901_test_installing_extension_when_locked_file_exists v2 needs to be checked-in.  The other 2 patches have been checked-in already.

Comment 35

[Carsten Book [:Tomcat]]

Hey Trevor, this failed to apply:

adding 1180901 to series file
renamed 1180901 -> bug1180901_test_installing_extension_when_locked_file_exists.diff
applying bug1180901_test_installing_extension_when_locked_file_exists.diff
patching file toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini
Hunk #1 FAILED at 23
1 out of 1 hunks FAILED -- saving rejects to file toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh bug1180901_test_installing_extension_when_locked_file_exists.diff
Tomcats-MacBook-Pro:fx-team Tomcat$ 

could you take a look, thanks!

Comment 36

[Trevor Rowbotham [:rowbot]]

Attachment: bug1180901_test_installing_extension_when_locked_file_exists_fixed

Thanks for bringing this to my attention, Carsten.  This should apply cleanly now.  Carrying forward r=mossop.

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/190b9bfc9ae2

Comment 38

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/190b9bfc9ae2

Comment 39

[Patrick Dark]

These error messages are still occurring in Thunderbird 38.6.0, though they don't seem to be preventing my extensions from being installed (despite the "Failed to install" portion).

Comment 40

[Trevor Rowbotham [:rowbot]]

(In reply to Patrick Dark from comment #39)
> These error messages are still occurring in Thunderbird 38.6.0, though they
> don't seem to be preventing my extensions from being installed (despite the
> "Failed to install" portion).
I assume you are on Windows.  Do you have a trash folder lingering in the extensions  directory? The path should be something like C:\Users\<username>\AppData\Roaming\Mozilla\Firefox\Profiles\<profile name>\extensions.

Comment 41

[Patrick Dark]

Attachment: Error Messages in Thunderbird 38.6.0 with Firefox 44.0.2 Console

(In reply to Trevor Rowbotham from comment #40)
> (In reply to Patrick Dark from comment #39)
> > These error messages are still occurring in Thunderbird 38.6.0, though they
> > don't seem to be preventing my extensions from being installed (despite the
> > "Failed to install" portion).
> I assume you are on Windows.  Do you have a trash folder lingering in the
> extensions  directory? The path should be something like
> C:\Users\<username>\AppData\Roaming\Mozilla\Firefox\Profiles\<profile
> name>\extensions.

Yes, I'm on Windows 10. I searched the whole Roaming directory and the only folders called Trash are in Thunderbird (not Firefox) directories. None of them are in the Extensions directory or subdirectories though. I can't seem to reproduce the errors now. The errors were occurring when I was testing changes to my extension by repeatedly dragging them into the Add-ons tab, then restarting Thunderbird. I just tried again with the same add-on to no avail.