911218 - [mozprofile] AddonManager should only optionally cleanup on __del__

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Jeff Hammel]

https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/addons.py#L266

Currently, use of the AddonsManager by itself does not persist beyond
the instance scope.  This is a problem when it is used independently,
e.g. in bug 746423 :

+ # XXX: del the __del__ + # -AddonManager should only cleanup on __del__ optionally:
+ # https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/addons.py#L266
+ del addons.__del__

Comment 1

[Jeff Hammel]

Attachment: addonmanager-cleanup.diff

Comment 2

[William Lachance (:wlach)]

Comment on attachment 797903 [details] [diff] [review]
addonmanager-cleanup.diff

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

Ok so as discussed on irc:

1. Do we really need this cleanup behaviour at all? Or is it just a potential source of bugs?
2. If we do need it, should we be depending on __del__ to call it? That's generally considered a bad practice in python (). If it's really important to some particular harness to clean stuff up, maybe it should be called explicitly.

:jhammel and I agreed to talk about this at the next mozbase meeting.

These questions aside, this looks ok overall except for the fact that it looks like the addon manager constructed by profile.py isn't actually using its new restore parameter. Shouldn't it? If I'm missing something let me know.

::: mozprofile/mozprofile/profile.py
@@ +40,1 @@
>      """

Shouldn't you be passing this new "restore" parameter to the AddonsManager class we instantiate?

Comment 3

[Jeff Hammel]

(In reply to William Lachance (:wlach) from comment #2)
> Comment on attachment 797903 [details] [diff] [review]
> addonmanager-cleanup.diff
> 
> Review of attachment 797903 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok so as discussed on irc:
> 
> 1. Do we really need this cleanup behaviour at all? Or is it just a
> potential source of bugs?
> 2. If we do need it, should we be depending on __del__ to call it? That's
> generally considered a bad practice in python (). If it's really important
> to some particular harness to clean stuff up, maybe it should be called
> explicitly.
> 
> :jhammel and I agreed to talk about this at the next mozbase meeting.

Noted at https://etherpad.mozilla.org/great-automation-refactor ; in brief
- not cleaning up (or relying on consumers to cleanup) for, notably, persisted profiles introduces a different gotcha
- in general, it'd be nice to have reliable cleanup; AFAIK, __del__ doesn't absolutely guarantee this....but it isn't necessarily the worst answer possible. The contextmanager approach is another way to go: http://docs.python.org/2/library/stdtypes.html#typecontextmanager ; http://docs.python.org/2/reference/datamodel.html#context-managers
In general, I hope we can agree to have idempotent cleanup methods, whatever they're called; in this case, they can be called explicitly if needed or as part of __del__ or __exit__ or other strategy
 
> These questions aside, this looks ok overall except for the fact that it
> looks like the addon manager constructed by profile.py isn't actually using
> its new restore parameter. Shouldn't it? If I'm missing something let me
> know.

Heh, good catch!  Yes, absolutely.  Was adding this when I got distracted by the bad grammar in the docstring. Fixed.

> ::: mozprofile/mozprofile/profile.py
> @@ +40,1 @@
> >      """
> 
> Shouldn't you be passing this new "restore" parameter to the AddonsManager
> class we instantiate?

Um, yes :)

Comment 4

[Jeff Hammel]

Attachment: bug-911218.diff

Same as above, but
- pass the `restore` parameter to the instantiated AddonManager
- ensure that .xpi addons exist; otherwise, calling .cleanup() twice will raise an OSError

Comment 5

[William Lachance (:wlach)]

Comment on attachment 798239 [details] [diff] [review]
bug-911218.diff

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

Looks good, I have some suggestions for making the docs slightly more clear (I think).

::: mozprofile/mozprofile/addons.py
@@ +24,3 @@
>          """
>          :param profile: the path to the profile for which we install addons
> +        :param restore: whether to reset to the previous state on instance garbage collection

Let's be consistent with profile.py

whether to reset to the previous state on instance garbage collection --> if true, remove all installed addons on instance garbage collection

::: mozprofile/mozprofile/profile.py
@@ +36,4 @@
>      :param preferences: Dictionary or class of preferences
>      :param locations: ServerLocations object
>      :param proxy: setup a proxy
> +    :param restore: If true remove all added addons and preferences when cleaning up

We may as well make it more clear when cleanup occurs: 

"If true remove all added addons and preferences on instance garbage collection"

Comment 6

[Jeff Hammel]

pushed: https://github.com/mozilla/mozbase/commit/c97ee18684f30325c7b7fb446704ea4871c1eeac

Will, I didn't change the docstrings per your comment 5 as cleanup is slightly different.  Profile uses restore in cleanup, so if cleanup is called explicitly "stuff" will only be restored if self.restore is true:

https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/profile.py#L246

Profile.__del__ is set to Profile.clean so it follows this logic.  To contrast, AddonManager clean_addons will restore the addons regardless of the state of self.restore:

https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/addons.py#L254

__del__ conditionally calls clean_addons only if self.restore is set:

https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/addons.py#L277

I admit the disparity is...not necessarily ideal, so I will leave this bug open in case a follow up is desired.  We could make Profile.__del__ more like AddonManager.__del__ or vice versa (I think other classes in mozprofile will need similar treatments).  FWIW, I suppose I like the pattern in AddonManager slightly better, but to go back to comment 2, this is IMHO indicative of bigger cleanup issues we haven't dealt with (including the fact that if the profile is temporary...cleanup really doesn't need to do more than .rmtree()).

What are your thoughts, Will?

Comment 7

[Jeff Hammel]

Per today's mozbase meeting, I'll file a bug to work out the details regarding cleanup.  Basic agreement is that mozprofile should be as simple as possible, and that contextmanagers are likely the best way forward.

Comment 8

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #7)
> Per today's mozbase meeting, I'll file a bug to work out the details
> regarding cleanup.  Basic agreement is that mozprofile should be as simple
> as possible, and that contextmanagers are likely the best way forward.

see https://bugzilla.mozilla.org/show_bug.cgi?id=912200

Closing this bug.

Comment 9

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

This has not been fully fixed.  I filed bug 931894 as a follow-up.