[mozprofile] Unpredictable behavior of the Profile class because __init__ get called again on Profile and AddonManager class

RESOLVED FIXED

Status

Testing
Mozbase
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
As seen on bug 931894 we call AddonManager.clean_addons() after the profile has been wiped out via Profile.cleanup(). This is the case for a temporarily created profile and with the reset parameter set to true.

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

The problem is visible if a backup of an add-on has been made via the AddonManager during an add-on installation. You can see this by running the following test once the patch on bug 931894 has been landed:

python mozprofile/tests/test_addons.py

Jeff, I'm not exactly sure what to do here, but I would propose that we always clean-up internal instances of other classes before destructing the main mozprofile instance. Please let me know what you think.
(Assignee)

Comment 1

5 years ago
Jeff and Andrew, I would be really interested in your feedback here.
Flags: needinfo?(jhammel)
Flags: needinfo?(ahalberstadt)
(In reply to Henrik Skupin (:whimboo) from comment #0)
> Jeff, I'm not exactly sure what to do here, but I would propose that we
> always clean-up internal instances of other classes before destructing the
> main mozprofile instance. Please let me know what you think.

This makes sense to me. I'm actually a bit surprised we aren't going this in the first place.
Flags: needinfo?(ahalberstadt)
(Assignee)

Updated

5 years ago
Blocks: 931828

Comment 3

5 years ago
(In reply to Henrik Skupin (:whimboo) from comment #0)
> As seen on bug 931894 we call AddonManager.clean_addons() after the profile
> has been wiped out via Profile.cleanup(). This is the case for a temporarily
> created profile and with the reset parameter set to true.
> 
> https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/profile.
> py#L246
> https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/addons.
> py#L283
> 
> The problem is visible if a backup of an add-on has been made via the
> AddonManager during an add-on installation. You can see this by running the
> following test once the patch on bug 931894 has been landed:
> 
> python mozprofile/tests/test_addons.py

I don't see this locally with the patch applied:

(mozbase)│git st
# On branch master
# Changes not staged for commit:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#	modified:   mozprofile/mozprofile/addons.py
#	modified:   mozprofile/mozprofile/profile.py
#	modified:   mozprofile/tests/test_addons.py
#
no changes added to commit (use "git add" and/or "git commit -a")
(mozbase)│python mozprofile/tests/test_addons.py
.s..s......
----------------------------------------------------------------------
Ran 11 tests in 1.054s

OK (skipped=2)

Am I missing something?
 
> Jeff, I'm not exactly sure what to do here, but I would propose that we
> always clean-up internal instances of other classes before destructing the
> main mozprofile instance. Please let me know what you think.

I don't know what you mean; I thought we did do this?

https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/profile.py#L251
Flags: needinfo?(jhammel)
(Assignee)

Comment 4

5 years ago
(In reply to Jeff Hammel [:jhammel] from comment #3)
> > The problem is visible if a backup of an add-on has been made via the
> > AddonManager during an add-on installation. You can see this by running the
> > following test once the patch on bug 931894 has been landed:
> > 
> > python mozprofile/tests/test_addons.py
> 
> I don't see this locally with the patch applied:

The latest version of this patch as uploaded late yesterday has a workaround to remove this left-over profile folder. That's what Andrew suggested. So this extra profile folder will not be visible as of now.

> > Jeff, I'm not exactly sure what to do here, but I would propose that we
> > always clean-up internal instances of other classes before destructing the
> > main mozprofile instance. Please let me know what you think.
> 
> I don't know what you mean; I thought we did do this?
> 
> https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/profile.
> py#L251

No, we do not reach this block. We end-up in the create_new if condition above. So none of the methods you pointed out gets called.
(Assignee)

Comment 5

5 years ago
It's not clear what happens here yet, but I will show you some interesting output:

Here a working example of how I would expect it to look like:

Code:
=====
profile = mozprofile.profile.Profile()

profile.reset()
print 'shutdown'


Result:
=======
Profile.cleanup
AddonManager.__del__
AddonManager.clean_addons
shutdown
Profile.cleanup
AddonManager.__del__
AddonManager.clean_addons


Here an example with the addon manager assigned to another variable:

Code:
=====
profile = mozprofile.profile.Profile()
am = profile.addon_manager

profile.reset()
print 'shutdown'

Result:
=======
Profile.cleanup
shutdown
Profile.cleanup
AddonManager.__del__
AddonManager.clean_addons
AddonManager.__del__
AddonManager.clean_addons


I think we are facing a weak reference issue here for the AddonManager, and that's why the __del__ method is not being called for profile.reset().
(Assignee)

Comment 6

5 years ago
Ok, so that seems to be the problem here. Let me explain with the above failing code, and one more reset() call:

> refcount: 2

This is the refcount right after creating the profile, which is fine given that the sys.getrefcount() method also creates a reference.

> refcount: 3

Here the refcount gets increased because we are creating a new variable for the Addon Manager.

> reset
> Profile.cleanup
> refcount: 2

Calling profile.reset() causes the Profile.__init__ method to be executed. A new instance of the AddonManager gets created, and as result the refcount decreases by one. But given the temporary variable created above, which still holds the reference, the old Addon Manager instance will not be destroyed. So we have two instances alive.

> reset
> Profile.cleanup
> AddonManager.__del__
> AddonManager.clean_addons
> refcount: 2

Another profile.reset() correctly calls the AddonManager.__del__ method because only a single reference to the newly created AddonManager instance exists.

> shutdown
> Profile.cleanup
> AddonManager.__del__
> AddonManager.clean_addons
> AddonManager.__del__
> AddonManager.clean_addons

During shutdown AddonManager.__del__ gets called twice because we have to destruct two different instances. One from the last reset() call and the temporary variable from above.
(Assignee)

Comment 7

5 years ago
Because Andrew asked on IRC, the short explanation is that whenever you create a variable as e.g. shortcut to the add-on manager (am = profile.addon_manager), you will get unpredictable behavior of the Profile class due to multiple references.
Summary: [mozprofile] AddonManager.clean_addons() should not be called *after* Profile.cleanup() → [mozprofile] Unpredictable behavior of the Profile class if multiple references to the AddonManager class exist
(Assignee)

Updated

5 years ago
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
(Assignee)

Comment 8

5 years ago
So the problem here is actually that we call __init__() again inside of reset() in both the Profile and the AddonManager class. This is really bad design and should not be used. Instead another initialize() method should be added to both classes, which can be called to re-initialize all the class members.
Summary: [mozprofile] Unpredictable behavior of the Profile class if multiple references to the AddonManager class exist → [mozprofile] Unpredictable behavior of the Profile class because __init__ get called again on Profile and AddonManager class
(Assignee)

Comment 9

5 years ago
This bug is more complicated to fix right as I first thought. Given my limited availability I might not have a working patch completed before Monday. There are too many subclasses used by Profile I have to take care of.
(Assignee)

Comment 10

5 years ago
Created attachment 832528 [details] [diff] [review]
WIP v0.1

Andrew, as of now I only have an incomplete patch around. There is still work to do for webapps, permissions, and prefs. But I would like to get your feedback first for the current changes of the profile and add-on manager class.

With the current code I do not see the problem with the destructor anymore given that we make use of the same AddonManager instance and do no longer call __init__ from both the Profile and AddonManager class.

If all looks fine to you I will continue when I'm back on Monday. Please let me know what else needs improvements or should not be done that way. Thanks.
Attachment #832528 - Flags: feedback?(ahalberstadt)
Comment on attachment 832528 [details] [diff] [review]
WIP v0.1

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

This looks good.

::: mozprofile/mozprofile/addons.py
@@ +88,5 @@
> +            if not os.listdir(self.backup_dir):
> +                shutil.rmtree(self.backup_dir, ignore_errors=True)
> +
> +        # reset instance variables to defaults
> +        self.initialize()

I would call this "_internal_reset" or something just so it's absolutely clear that it shouldn't ever be called by consumers.

::: mozprofile/mozprofile/profile.py
@@ +73,5 @@
> +        self.addon_manager = AddonManager(self.profile, restore=self.restore)
> +
> +        self.initialize()
> +
> +    def initialize(self):

Same. Also _internal_reset would distinguish it from all the other confusing cleanup/resetting methods we have.
Attachment #832528 - Flags: feedback?(ahalberstadt) → feedback+
(Assignee)

Comment 12

5 years ago
Created attachment 8333825 [details] [diff] [review]
Patch v1

Cleaned-up patch with the proposal included. I left the creation of the Addon Manager in _internal_init(), so any consumer has to ensure to re-assign the shortcut if used. We might want to fix / improve this later. For now it should be fine.
Attachment #832528 - Attachment is obsolete: true
Attachment #8333825 - Flags: review?(ahalberstadt)
Comment on attachment 8333825 [details] [diff] [review]
Patch v1

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

Thanks, looks good!

::: mozprofile/mozprofile/profile.py
@@ +44,5 @@
> +        self._addons = addons
> +        self._addon_manifests = addon_manifests
> +        self._apps = apps
> +        self._locations = locations
> +        self._proxy = proxy

I agree with underscore prefixing these in principle, but I'm worried this is going to cause bustage somewhere in m-c. Personally I wouldn't change them, but I'm ok with landing as is if you really want.
Attachment #8333825 - Flags: review?(ahalberstadt) → review+
(Assignee)

Comment 14

5 years ago
Comment on attachment 8333825 [details] [diff] [review]
Patch v1

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

::: mozprofile/mozprofile/profile.py
@@ +44,5 @@
> +        self._addons = addons
> +        self._addon_manifests = addon_manifests
> +        self._apps = apps
> +        self._locations = locations
> +        self._proxy = proxy

There is no difference for _locations and _proxy given that those were already present. _addons, _addon_manifests, and _apps are new and there should really be no side-effect for landing this on m-c. No other module should really set or modify properties with prefixes.

So I think that we are good in getting this landed.
(Assignee)

Comment 15

5 years ago
Landed as:
https://github.com/mozilla/mozbase/commit/757a28b6626a8f8e93ae24509cf9a1127a438f61
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.