Check that firefox processes are cleaned up across test runs

RESOLVED FIXED in Firefox 58

Status

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gbrown, Assigned: gbrown)

Tracking

Trunk
mozilla59
Points:
---

Firefox Tracking Flags

(firefox58 fixed, firefox59 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

In bug 1397201, it seems that bad things are happening to tests on Windows, at least in part because firefox processes from previous test runs are still running.

Are we doing everything we can to kill firefox at the end of a test run?

Can we kill firefox if it is found running before a test is started?
I already added that to bug 1415245, but will do it here again:

Please note that for bug 1397201 we seem to collect orphaned Firefox processes while mochitests are running. Not sure how mochitests are working, but they start Firefox a couple of times. So ideally for each start of Firefox such an output would be good to have.
See Also: → 1415290
I realized that I run Firefox while running mochitests locally all the time -- that's not a problem (since none of my local Firefox instances are running marionette servers). Anyway, we cannot fail just because Firefox is running.

We can report when 'firefox' is running, even warn about it, and that might be useful in automation, where we don't expect to see extra Firefox instances at all.

Also, runtests.py can fail if it finds a browser running that it knows it started and thinks was cleaned up.
Summary: Cleanup firefox processes across test runs → Check that firefox processes are cleaned up across test runs
Comment on attachment 8927469 [details] [diff] [review]
report when 'firefox' running during mochitests; fail if it is one of ours

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

lots of good checking, just one thing to look out for here.

::: testing/mochitest/runtests.py
@@ +2179,4 @@
>                           'onTimeout': [timeoutHandler]}
>              kp_kwargs['processOutputLine'] = [outputHandler]
>  
> +            self.checkForRunningBrowsers()

will this conflict when running locally with ./mach or debugging on a loaner?
Attachment #8927469 - Flags: review?(jmaher) → review+
Comment on attachment 8927469 [details] [diff] [review]
report when 'firefox' running during mochitests; fail if it is one of ours

Oh darn, with more testing I hit several "Found old browser" exceptions and upon further examination found that the "old browser" was actually a new process using the recycled pid. I didn't think that was likely, but apparently there's a good chance on Windows 7. I'll need a new strategy.

https://archive.mozilla.org/pub/firefox/try-builds/gbrown@mozilla.com-2afc90db1d209ae21cbcbd911db39a492d9eb5b4/try-win32-stylo-disabled/try_win7_ix_stylo_disabled_test-mochitest-clipboard-e10s-bm111-tests1-windows-build371.txt.gz
Attachment #8927469 - Attachment is obsolete: true
Should we better try to find out which tests do not clean-up correctly? As it looks like the media job seems to be a good candidate for that. 

I don't think that we can do more than reporting here, as you both already said. The other solution would only be to let Marionette use a free and not hard-coded port.
This removes the error and just warns when 'firefox' is found running before mochitest starts a browser. I expect warnings when running locally, but I doubt anyone will be troubled by that. In automation, when we hit problems that might be related to extra browsers, we can check for this warning. I think that's the best we can do here.
Attachment #8927912 - Flags: review?(jmaher)
Attachment #8927912 - Flags: review?(jmaher) → review+
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf0dc4097849
Warn if Firefox is running before mochitest starts Firefox; r=jmaher
https://hg.mozilla.org/mozilla-central/rev/cf0dc4097849
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.