Let's say you have a profile at ~/.mozilla/firefox/foo with the e.g. mozmill addon (though any addon will do). If you specify FirefoxProfile(addon=['path/to/mozmill/extension'], profile='~/.mozilla/firefox/foo').cleanup() the addonmanager will delete the mozmill extension from your profile even though it was originally installed by the user

Note also that this has been a bug since before there was mozbase

This is probably easy enough to tackle if you know about Firefox profiles *and* the mozprofile code makes sense to you: https://github.com/mozilla/mozbase/tree/master/mozprofile . (Please look at the code first, as this bug assumes you have some knowledge of how mozprofile functions). The way I see it, there are two options: - if you have an extension installed that you are overwriting, you can copy the already-installed extension somewhere and then copy it back. This is probably best - if you demand an extension be installed which you already have (some version of) a copy of in the chosen profile, a warning message should be printed out to this effect and the new extension should not be installed. This should also have a test to confirm what is done == what is desired.

How does this look? Not sure if the xpi file backup was necessary as well.

Comment on attachment 708587 [details] [diff] [review] forgot to remove the backup dir after restore >diff --git a/mozprofile/mozprofile/addons.py b/mozprofile/mozprofile/addons.py >index e135516..0068ca1 100644 >--- a/mozprofile/mozprofile/addons.py >+++ b/mozprofile/mozprofile/addons.py >@@ -33,6 +33,10 @@ class AddonManager(object): > # addons that we've installed; needed for cleanup > self._addon_dirs = [] > >+ # backup dir for already existing addons >+ self.backup_dir = tempfile.mkdtemp() >+ self.backups = {} >+ I wonder if the backup_dir should be created as a subdirectory of the profile directory. In any case, IMHO we should not create the backup_dir unless it is necessary. > def install_addons(self, addons=None, manifests=None): > """ > Installs all types of addons >@@ -215,8 +219,16 @@ class AddonManager(object): > if not unpack and not addon_details['unpack'] and xpifile: > if not os.path.exists(extensions_path): > os.makedirs(extensions_path) >+ # save existing xpi file to restore later >+ if os.path.exists(addon_path + '.xpi'): >+ self.backups[addon_id] = (addon_id + '.xpi', 'file') >+ shutil.copy(addon_path + '.xpi', self.backup_dir) > shutil.copy(xpifile, addon_path + '.xpi') > else: >+ # save existing dir to restore later >+ if os.path.exists(addon_path): >+ self.backups[addon_id] = (addon_id, 'dir') >+ dir_util.copy_tree(addon_path, self.backup_dir, preserve_symlinks=1) > dir_util.copy_tree(addon, addon_path, preserve_symlinks=1) > self._addon_dirs.append(addon_path) > >@@ -233,3 +245,14 @@ class AddonManager(object): > for addon in self._addon_dirs: > if os.path.isdir(addon): > dir_util.remove_tree(addon) >+ # restore backups >+ extensions_path = os.path.join(self.profile, 'extensions', 'staged') >+ for addon, data in self.backups.iteritems(): >+ backup_path = os.path.join(self.backup_dir, data[0]) >+ backup_type = data[1] >+ addon_path = os.path.join(extensions_path, addon) >+ if backup_type == 'dir': >+ dir_util.copy_tree(backup_path, addon_path, preserve_symlinks=1) >+ else: >+ shutil.copy(backup_path, addon_path + '.xpi') >+ shutil.rmtree(self.backup_dir, ignore_errors=True) Its probably worth adding a __del__ method that front-ends this.

(In reply to Jeff Hammel [:jhammel] from comment #5) > Comment on attachment 708587 [details] [diff] [review] > > I wonder if the backup_dir should be created as a subdirectory of the > profile directory. In any case, IMHO we should not create the backup_dir > unless it is necessary. I think /tmp makes more sense. I added the functionality of not creating the dir unless it is necessary. > > > def install_addons(self, addons=None, manifests=None): > > """ > > Installs all types of addons > >@@ -215,8 +219,16 @@ class AddonManager(object): > > if not unpack and not addon_details['unpack'] and xpifile: > > if not os.path.exists(extensions_path): > > os.makedirs(extensions_path) > >+ # save existing xpi file to restore later > >+ if os.path.exists(addon_path + '.xpi'): > >+ self.backups[addon_id] = (addon_id + '.xpi', 'file') > >+ shutil.copy(addon_path + '.xpi', self.backup_dir) > > shutil.copy(xpifile, addon_path + '.xpi') > > else: > >+ # save existing dir to restore later > >+ if os.path.exists(addon_path): > >+ self.backups[addon_id] = (addon_id, 'dir') > >+ dir_util.copy_tree(addon_path, self.backup_dir, preserve_symlinks=1) > > dir_util.copy_tree(addon, addon_path, preserve_symlinks=1) > > self._addon_dirs.append(addon_path) > > > >@@ -233,3 +245,14 @@ class AddonManager(object): > > for addon in self._addon_dirs: > > if os.path.isdir(addon): > > dir_util.remove_tree(addon) > >+ # restore backups > >+ extensions_path = os.path.join(self.profile, 'extensions', 'staged') > >+ for addon, data in self.backups.iteritems(): > >+ backup_path = os.path.join(self.backup_dir, data[0]) > >+ backup_type = data[1] > >+ addon_path = os.path.join(extensions_path, addon) > >+ if backup_type == 'dir': > >+ dir_util.copy_tree(backup_path, addon_path, preserve_symlinks=1) > >+ else: > >+ shutil.copy(backup_path, addon_path + '.xpi') > >+ shutil.rmtree(self.backup_dir, ignore_errors=True) > > Its probably worth adding a __del__ method that front-ends this. I'm not sure I understand. We have no guarantees __del__ will be called.

(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #6) > (In reply to Jeff Hammel [:jhammel] from comment #5) > > Comment on attachment 708587 [details] [diff] [review] > > > > I wonder if the backup_dir should be created as a subdirectory of the > > profile directory. In any case, IMHO we should not create the backup_dir > > unless it is necessary. > > I think /tmp makes more sense. I added the functionality of not creating the > dir unless it is necessary. Thanks. WFM. In the majority of the cases we shouldn't hit this at all assuming that we aren't doing this a bunch currently (which since no one has complained, i'm guessing not ;) > > > > > def install_addons(self, addons=None, manifests=None): > > > """ > > > Installs all types of addons > > >@@ -215,8 +219,16 @@ class AddonManager(object): > > > if not unpack and not addon_details['unpack'] and xpifile: > > > if not os.path.exists(extensions_path): > > > os.makedirs(extensions_path) > > >+ # save existing xpi file to restore later > > >+ if os.path.exists(addon_path + '.xpi'): > > >+ self.backups[addon_id] = (addon_id + '.xpi', 'file') > > >+ shutil.copy(addon_path + '.xpi', self.backup_dir) > > > shutil.copy(xpifile, addon_path + '.xpi') > > > else: > > >+ # save existing dir to restore later > > >+ if os.path.exists(addon_path): > > >+ self.backups[addon_id] = (addon_id, 'dir') > > >+ dir_util.copy_tree(addon_path, self.backup_dir, preserve_symlinks=1) > > > dir_util.copy_tree(addon, addon_path, preserve_symlinks=1) > > > self._addon_dirs.append(addon_path) > > > > > >@@ -233,3 +245,14 @@ class AddonManager(object): > > > for addon in self._addon_dirs: > > > if os.path.isdir(addon): > > > dir_util.remove_tree(addon) > > >+ # restore backups > > >+ extensions_path = os.path.join(self.profile, 'extensions', 'staged') > > >+ for addon, data in self.backups.iteritems(): > > >+ backup_path = os.path.join(self.backup_dir, data[0]) > > >+ backup_type = data[1] > > >+ addon_path = os.path.join(extensions_path, addon) > > >+ if backup_type == 'dir': > > >+ dir_util.copy_tree(backup_path, addon_path, preserve_symlinks=1) > > >+ else: > > >+ shutil.copy(backup_path, addon_path + '.xpi') > > >+ shutil.rmtree(self.backup_dir, ignore_errors=True) > > > > Its probably worth adding a __del__ method that front-ends this. > > I'm not sure I understand. We have no guarantees __del__ will be called. That is true. My general feeling is that having __del__ prevents some of the cases if the consumer forgets to call cleanup() or otherwise it doesn't get hit. For problems like this, I think this is a (albeit very slight) win, since all we're really doing is cleaning up temporary files/directories that hit disk. If we needed to be absolutely sure that a code path got hit, there are patterns to do this, but IMHO they're not worth it unless its a solid requirement. From another point of view, its an easy one liner to point __del__ to cleanup and I can't think of a reason not to add it (since cleanup should be idempotent): def cleanup(): """cleanup some things""" __del__ = cleanup From my point of view, you may wish to call cleanup other than via __del__, so its good that they're separate anyway. TL;DR - I think a __del__ alias is nice, but not very important. If you don't add it, that's fine too ;)

I misunderstood you about __del__, I didn't think you were talking about just an alias. :) Let me know if this is ok.

Comment on attachment 712885 [details] [diff] [review] fix There may be edge cases that I can't formulate OTTOMH, but this looks good for now...at least much better than what we have currently. Bonus points for writing a follow up ticket on having a test for this

pushed: https://github.com/mozilla/mozbase/commit/6c09199d4bf42e670efa9a3855261fb5996634cb Should write a follow-up for having a test

I actually forgot to close this bug, but this luckily served as a reminder to file the follow up "needs a damn test" bug: https://bugzilla.mozilla.org/show_bug.cgi?id=841086