1173380 - [mozprofile] cloned profiles are not cleaned (__del__ method does not seems to be called)

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Julien Pagès (:parkouss)]

So this is a follow up of bug 1173335. I reduced the test case to reproduce the issue:

import os
import shutil
from mozprofile.profile import Profile

profile = Profile()
profile_dir = profile.profile
# do not automatically remove the profile path
# for this profile instance, so we can clone it.
profile.cleanup = lambda : None

del profile
assert os.path.exists(profile_dir)

# create a clone from this profile path
clone = Profile.clone(profile_dir)
clone_profile_dir = clone.profile

# removing the clone should remove the profile
del clone

try:
    # but it does not
    assert not os.path.exists(clone_profile_dir)
except AssertionError:
    shutil.rmtree(clone_profile_dir)
    raise
finally:
    shutil.rmtree(profile_dir)

Comment 1

[Julien Pagès (:parkouss)]

hmm, this is strange because there is a tests/test_clone_cleanup.py that is intended to test this I would say.

Comment 2

[Julien Pagès (:parkouss)]

Ah, no, this is not exactly the same thing:

http://code.metager.de/source/xref/mozilla/firefox/testing/mozbase/mozprofile/tests/test_clone_cleanup.py#34

There is an explicit call to cleanup(); If I replace the line with "del clone", it does not work.

Comment 3

[Julien Pagès (:parkouss)]

Attachment: 1173380.patch

Well, I found the root cause, and fixed it. Here is a copy paste of the discussion with :whimboo that can help to understand the patch:

10:04	parkouss	this is quite easy, it only use the *create_new* cariable we discussed
10:05	parkouss	somewhere the decorator seems to keep a reference to the instance
10:05	parkouss	well this is my understanding
10:05	parkouss	test pass with the new code, fails without
10:07	whimboo	so we had duplicated code beside __del__ which was responible only for deleting files of a cloned profile?
10:08	whimboo	regarding the review please pass it to ahal or wlach I would say
10:08	parkouss	it seems like that to me
10:08	parkouss	ok
10:09	whimboo	this would be awkward given that the cloned profile is also a profile class instance
10:09	whimboo	and should rely on the same cleanup code as defined on the Profile class
10:09	parkouss	well I think this is because we wanted by default to remove a cloned profile (when restore is True)
10:10	parkouss	but the profile constructor won't let this happen if you pass a profile path
10:10	parkouss	I suppose this was the reason
10:11	parkouss	override the __del__ so cloned are deleted even if the profile path is set (assuming that restore is True)

Comment 4

[Andrew Halberstadt [:ahal]]

Comment on attachment 8620843 [details] [diff] [review]
1173380.patch

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

Clever, thanks for finding this!

Comment 5

[Julien Pagès (:parkouss)]

:ahal, do you think a try push is needed here, or can I land this directly ?

Comment 6

[Andrew Halberstadt [:ahal]]

Doesn't hurt to do a try push, probably just builds for the three major platforms and a test job or two are sufficient.

Comment 7

[Julien Pagès (:parkouss)]

Pushed to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=16a200653f0b

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a3fa532721b

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9a3fa532721b

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

for some reason my local instance of talos is picking up this change and I am not able to run talos successfully.  Oddly enough we have requirements.txt that has 'mozprofile==0.23', but that isn't being respected- maybe there is a dependency somewhere else.

maybe we can figure out how to use profiles properly in talos so we can upgrade to 0.24?

Comment 11

[Julien Pagès (:parkouss)]

Yep, upgrading to mozprofile 0.24 should be easy - I created a bug for that: Bug 1180664.