759594 - mozprofile will delete addons if you specify the same addon to install

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Jeff Hammel]

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

Comment 1

[Jeff Hammel]

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

Comment 2

[Jeff Hammel]

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.

Comment 3

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: back up existing addons that get overridden

How does this look?

Not sure if the xpi file backup was necessary as well.

Comment 4

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: forgot to remove the backup dir after restore

Comment 5

[Jeff Hammel]

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.

Comment 6

[Mihnea Dobrescu-Balaur (:mihneadb)]

(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.

Comment 7

[Jeff Hammel]

(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 ;)

Comment 8

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: fix

I misunderstood you about __del__, I didn't think you were talking about just an alias. :)

Let me know if this is ok.

Comment 9

[Jeff Hammel]

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

Comment 10

[Jeff Hammel]

pushed: https://github.com/mozilla/mozbase/commit/6c09199d4bf42e670efa9a3855261fb5996634cb

Should write a follow-up for having a test

Comment 11

[Jeff Hammel]

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