Marionette does not always remove temporary profiles

RESOLVED FIXED in Firefox 40

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: ato, Assigned: chmanchester)

Tracking

({pi-marionette-runner})

unspecified
mozilla40
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

5 years ago
mozrunner produces temporary folders in /tmp that are not cleaned up.  I haven't investigated if this happens for clean exits, interrupts, terminations, or all of these.

This morning the mozrunner related directories in /tmp had grown to ~790M.
Which type of tests were causing this behavior? What are those files? The profile data?
Reporter

Comment 2

5 years ago
They look like directories containing the profile data as you say.  They appear to be generated from running `./mach marionette-test`.

~ % lc -d /tmp/*mozrunner
tmp0ybIF_.mozrunner tmp8kX8cz.mozrunner tmpBqyKwK.mozrunner tmpNvnqZq.mozrunner tmpWs5pYO.mozrunner tmpbxBEq3.mozrunner tmpi1XoFm.mozrunner tmplGIFFP.mozrunner tmpqJpz9u.mozrunner
tmp11nnL8.mozrunner tmp9MWuWG.mozrunner tmpLVuEhI.mozrunner tmpS8EOr4.mozrunner tmpWve283.mozrunner tmpc248KF.mozrunner tmpieS1wE.mozrunner tmplcvvw8.mozrunner tmptK6WUk.mozrunner
tmp6N9XAy.mozrunner tmp9swc2l.mozrunner tmpMQgvsJ.mozrunner tmpTBa0SY.mozrunner tmpXigj1a.mozrunner tmpd9jcmO.mozrunner tmpj8RCk1.mozrunner tmpmRCyAI.mozrunner tmpusH4ed.mozrunner
tmp6_VBVs.mozrunner tmpA2WUoJ.mozrunner tmpMvXVZq.mozrunner tmpTol_Lm.mozrunner tmpXzLin0.mozrunner tmpda1XSi.mozrunner tmpl28Hg0.mozrunner tmpoA6BJ2.mozrunner tmpwiwgsr.mozrunner
tmp7LOBKV.mozrunner tmpBai08l.mozrunner tmpN50YK_.mozrunner tmpUxnK88.mozrunner tmpaIwKiJ.mozrunner tmpdsXPUB.mozrunner tmplBcNVR.mozrunner tmpooSArl.mozrunner tmpwrbgSf.mozrunner
tmp7SpHBt.mozrunner tmpBhSrAO.mozrunner tmpNtpstG.mozrunner tmpVkp9_G.mozrunner tmpbJizkh.mozrunner tmpgmGBOv.mozrunner tmplBcZdt.mozrunner tmppCR24L.mozrunner
~ % lc /tmp/tmp0ybIF_.mozrunner 
.parentlock             compatibility.ini       extensions.ini          minidumps               prefs.js                sessionstore-backups    webappsstore.sqlite
blocklist.xml           content-prefs.sqlite    extensions.json         permissions.sqlite      safebrowsing            startupCache            webappsstore.sqlite-shm
bookmarkbackups         cookies.sqlite          key3.db                 places.sqlite           search.json             thumbnails              webappsstore.sqlite-wal
cache2                  crashes                 lock                    places.sqlite-shm       secmod.db               user.js
cert8.db                directoryLinks.json     mimeTypes.rdf           places.sqlite-wal       sessionCheckpoints.json webapps
Yeah, that seems to be the case. It would be good to know if that is always happening for you. You may also wanna run mozrunner yourself, and check afterward if the created profile is still present.

It may be that some code in Marionette doesn't let mozrunner remove the profile.
Summary: mozrunner does not clean up temporary folders → mozrunner does not always clean-up temporary profiles
I actually did this myself, and all is fine for the mozrunner command line. But when running Marionette client I can see that the newly created profile is still present in the temporary folder. So this is clearly a Marionette issue.
Component: Mozbase → Marionette
QA Contact: hskupin
Summary: mozrunner does not always clean-up temporary profiles → Marionette does not always remove temporary profiles
Duplicate of this bug: 1118302
This is also a blocker of our upcoming Firefox ui tests. Adding dependency for bug 1080766.
Ok, I found the cause:

It appears that we are setting:

profile_args["restore"] = False

here:

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver/marionette_driver/geckoinstance.py#72

And that cause the profile to not be deleted (I set it to True, and temp dirs are deleted). According to the doc:

:param restore: Flag for removing all custom settings during cleanup

So it seems *normal* to have this behaviour, since we set that to False. The question is why was this set to False ?
Hm, I think that this piece of code (in the context given in comment 7):

if hasattr(self, "profile_path") and self.profile is None:
    if not self.profile_path:
        profile_args["restore"] = False

Could be translated by something like "if the profile given as an argument was None (or at least not evaluated to True), then set restore=False". This is because "profile_path" is only defined if the profile argument was not a Profile instance, and in this case we set profile_path = profile (in our case, None).

Does that make sense ? Maybe we should just get rid of this 'profile_args["restore"] = False' line.

BTW, all the logic here with profile/profile_path/profile_args seems tricky and a bit broken. I'm not sure to understand what we wanted to do with this code.
Thank you Julien for investigating the problem! I kinda appreciate that. I'm also a bit new to Marionette, so I don't know the details about that code and the expected behavior.

David, can you give us some more information here how Marionette handles profiles?
Flags: needinfo?(dburns)
Marionette uses mozrunner and mozprofile to start the browser into a state where we can automate things. 

see https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver/marionette_driver/geckoinstance.py#55. We merge them together on start()
Flags: needinfo?(dburns)
Ok, so given that you can pass in an existing profile via the --profile option, we should only remove the profile if it has really been created by Marionette. Or do we copy the existing profile to a temporarily location? Maybe that is what we have here?

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver/marionette_driver/geckoinstance.py#76

So if we always create a new profile, we should indeed remove it at every time.
Removing the 'profile_args["restore"] = False' line quoted in comment 8 fixes this when I tried it. This appears to be the correct behavior -- this is the common case no profile was passed in and a fresh one was created. If we need a mode for "post mortem" debugging a profile created during the test run I suppose that could be added.
/r/7135 - Bug 1083131 - Always remove a profile created by marionette when the runner shuts down.;r=ato

Pull down this commit:

hg pull -r eef071a3398c65a2150febbc2b0e67e4a47e1271 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8593518 - Flags: review?(ato)
Reporter

Comment 15

4 years ago
Comment on attachment 8593518 [details]
MozReview Request: bz://1083131/chmanchester

https://reviewboard.mozilla.org/r/7133/#review5933

Ship It!
Attachment #8593518 - Flags: review?(ato) → review+
Assignee

Updated

4 years ago
Keywords: checkin-needed
Aha. Windows.
Keywords: checkin-needed
(In reply to Chris Manchester [:chmanchester] from comment #16)
> Aha. Windows.

The only platform where removing the profile while the browser is running is considered a fatal error. This looks ok locally: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93a35acfe73b
Assignee

Updated

4 years ago
Attachment #8593518 - Flags: review+ → review?(ato)
Comment on attachment 8593518 [details]
MozReview Request: bz://1083131/chmanchester

/r/7135 - Bug 1083131 - Always remove a profile created by marionette when the runner shuts down.;r=ato

Pull down this commit:

hg pull -r d63b9ac77254a08b73507c706e44545b5df095c4 https://reviewboard-hg.mozilla.org/gecko/
Ignore the review request if try in comment 17 isn't ok, but this fixes things on a windows build locally.
Reporter

Comment 20

4 years ago
Comment on attachment 8593518 [details]
MozReview Request: bz://1083131/chmanchester

https://reviewboard.mozilla.org/r/7133/#review5959

Ship It!
Attachment #8593518 - Flags: review?(ato) → review+
Assignee

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/778e99b46052
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40

Updated

4 years ago
Duplicate of this bug: 1154060
Attachment #8593518 - Attachment is obsolete: true
Attachment #8618406 - Flags: review+
You need to log in before you can comment on or make changes to this bug.