931894 - [mozprofile] IOError in AddonManager.__del__ when trying to backup an already existent unpacked add-on

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

With the Addon Manager you can install the same addon twice, e.g. when they have different file names or locations. In those cases the addon in the profile will be removed successfully for the first installed version. But then __del__ fails for the second one, because it really only exists once in the profile:

Exception IOError: IOError(2, 'No such file or directory') in <bound method AddonManager.__del__ of <mozprofile.addons.AddonManager object at 0xd60550>> ignored


Here my list of test add-ons:

AddonManager._addons:
[u'/tmp/tmpjIbaWK.mozrunner/extensions/staged/[e-mail]',
 u'/tmp/tmpjIbaWK.mozrunner/extensions/staged/[e-mail]',
 u'/tmp/tmpjIbaWK.mozrunner/extensions/staged/[e-mail]',
 u'/tmp/tmpjIbaWK.mozrunner/extensions/staged/{8620c15f-30dc-4dba-a131-7c5d20cf4a29}',
 u'/tmp/tmpjIbaWK.mozrunner/extensions/staged/[e-mail]',
 u'/tmp/tmpjIbaWK.mozrunner/extensions/staged/[e-mail]']

AddonManager.installed_addons:
['/tmp/tmpBLAy5j.memchaser-0.5.2.1-fx.xpi', 
 '/tmp/tmpWG46zC.another-empty-0-1.xpi', 
 '/tmp/tmpLBrR85.empty.xpi',
 '/tmp/tmp4pkA_E.nightly_tester_tools-3.6-tb+sm+fx.xpi',
 '/mozilla/code/mozbase/xpi/another-empty-0-1',
 '/mozilla/code/mozbase/xpi/[e-mail]']


I think we should not allow additional installations of the same addon if another one with the same ID has already been installed.

Comment 1

[Jeff Hammel]

I

Comment 2

[Jeff Hammel]

beh, premature submit.

I don't think we should disallow installation of the same addon ID; IIRC, a different version (version number or otherwise )will have the same ID and it may be desirable to install the new version.

I do think we shouldn't clean up twice.  Once is plenty

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Oh, that's correct and makes sense. I missed this specific use-case.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So the problem here is that we fail in backing up the unpacked add-on. Instead of creating a subfolder for it at the backup location we copy all files directly into the parent folder.

Sadly I can't provide a patch yet due to the strange issue that the temporary profile folder is left and doesn't get removed. This has somewhat to do in our moving of files. I have no idea yet what it's causing it. I will continue tomorrow.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1

As mentioned on IRC already, there is one problem here. We do not remove the created profile at the end of the tests. I have absolutely no idea why. Any call to rmtree etc. succeeds and I don't get any failure.

Andrew, if you have some minutes and could have a look at it would be great. Just run the following commands to hopefully see this issue:

1. rm -rf /tmp/tmp*
2. Run ´python mozprofile/tests/test_addons.py´
3. ls -l /tmp*

As you can see one .mozrunner folder is left.

Comment 6

[Andrew Halberstadt [:ahal]]

Comment on attachment 824953 [details] [diff] [review]
Patch v1

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

It looks like it is getting cleaned up, but then at some point afterwards the install.rdf file is getting re-created in it's old location which creates the whole directory structure again.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hm, that would explain it. Looks like some weird path is hit here which tries to restore the add-on once more, even when it shouldn't. I will dive into that.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

If we do not restore the backup of the duplicated add-on in the clean_addons() method, the backup folder with the add-on will remain. It looks like that some other process still has a handle on it.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Heh, got it. Interestingly we call profile.cleanup() before AddonManager.clean_addons() is executed. That means that a backup which we have created gets restored after the profile has been removed! So we end-up with the same profile again.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Given that this problem might be larger I filed bug 934484 for it. I will go ahead and request review on this patch given that it fixes the exception and only has the caveat to leave a tmp folder behind in case a duplicated add-on is getting installed.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1.1

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 826771 [details] [diff] [review]
Patch v1.1

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

Moving to Andrew given that Jeff is overloaded.

Comment 13

[Andrew Halberstadt [:ahal]]

Comment on attachment 826771 [details] [diff] [review]
Patch v1.1

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

Makes sense to me.

::: mozprofile/mozprofile/profile.py
@@ +237,4 @@
>                          break
>                      except:
>                          count += 1
> +        except ImportError, e:

Not necessary

::: mozprofile/tests/test_addons.py
@@ +101,5 @@
> +
> +        # Bug 934484
> +        # Currently the profile folder gets recreated at the end and will be left
> +        # behind. So we should add another test for its existence once the bug
> +        # has been fixed.

If this test leaves behind the tmp profile dir we should delete it manually after the test is finished.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 826771 [details] [diff] [review]
Patch v1.1

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

::: mozprofile/mozprofile/profile.py
@@ +237,4 @@
>                          break
>                      except:
>                          count += 1
> +        except ImportError, e:

Ups. That's a left-over from testing where I have printed the exception details.

::: mozprofile/tests/test_addons.py
@@ +101,5 @@
> +
> +        # Bug 934484
> +        # Currently the profile folder gets recreated at the end and will be left
> +        # behind. So we should add another test for its existence once the bug
> +        # has been fixed.

Ok, will do.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: patch v2

So the reason why we don't correctly delete the profile folder might be related to weak references to the add-on manager in the test module. A final solution I will work out on bug 934484.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed on master:
https://github.com/mozilla/mozbase/commit/2de29b0e449ff8ee18cd4d67aa9ecac8a5272bc4