967595 - [mozrunner] Calling process_handler.proc.poll() returns 0 if the application is still running after a restart

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

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

So on bug 966234 we have experienced some very strange process disconnect errors with Mozmill and Firefox. That only happens when the application has been restarted and Mozmill wants to shutdown Firefox afterward. In such a case the self.returncode returns 0 while it should return None. As result we return too early from mozrunner.wait() and run into major trouble, because for the next test we cannot restart Firefox given that the profile is still in use by the former process.

Problem here is that self.returncode internally calls process_handler.proc.poll(), which doesn't care about the outThread, which is still active in those cases, and returns 0. 

So this is actually another regression from bug 962495. Process handling fun!!

What we should do is to have a poll() method on the process_handler which will do the right thing. This request has already been filed as bug 965326 and I will work on that now. Once done we can update mozrunner to make use of that new method.

Comment 1

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

Attachment: Patch v1

Not sure if Andrew is around today so also adding William as reviewer. Once the dependency bug has been fixed, it will be easier for us to check for the returncode via poll().

Comment 2

[Andrew Halberstadt [:ahal]]

Comment on attachment 8370293 [details] [diff] [review]
Patch v1

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

Lgtm, thanks!

Comment 3

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

Now that mozprocess 0.16 has been released (bug 968472) with the new feature included, we can finally land this - after I bumped the mozprocess dep to 0.16.

Comment 4

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

Attachment: Patch v1.1

Jonathan, would you mind to do a quick sanity review?

Comment 5

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8371056 [details] [diff] [review]
Patch v1.1

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

So, we always do a version bump of a package when altering the dependencies; I think we need to do that here as well.

Comment 6

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8371056 [details] [diff] [review]
Patch v1.1

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

talked on irc, and whimboo convinced me to do this and do a proper version bump later

Comment 7

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

(In reply to Jonathan Griffin (:jgriffin) from comment #6)
> talked on irc, and whimboo convinced me to do this and do a proper version
> bump later

The version bump will happen if we can be sure that the strange restart test issues in Mozmill tests are finally solved now. I have filed bug 968508.

Comment 8

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

https://github.com/mozilla/mozbase/commit/3fc539587a5b4155ff8eb56115e7ed5a9702c6c5

Andrei, can you please verify that with latest mozbase master, the problem is fixed in Mozmill? Thanks.

Comment 9

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

Not sure why but on travis we fail with issues I haven't seen locally. I have to investigate those tomorrow morning.

======================================================================

FAIL: test_stop_process (test_stop.MozrunnerStopTestCase)

Stop the process and test properties

----------------------------------------------------------------------

Traceback (most recent call last):

File "/home/travis/build/mozilla/mozbase/mozrunner/tests/test_stop.py", line 19, in test_stop_process

self.assertFalse(self.runner.is_running())

AssertionError: True is not false

======================================================================

FAIL: test_stop_process_custom_signal (test_stop.MozrunnerStopTestCase)

Stop the process via a custom signal and test properties

----------------------------------------------------------------------

Traceback (most recent call last):

File "/home/travis/build/mozilla/mozbase/mozrunner/tests/test_stop.py", line 35, in test_stop_process_custom_signal

self.assertFalse(self.runner.is_running())

AssertionError: True is not false

======================================================================

FAIL: test_process_post_stop_via_thread (test_threads.MozrunnerThreadsTestCase)

Stop the runner and try it again with a thread a bit later

----------------------------------------------------------------------

Traceback (most recent call last):

File "/home/travis/build/mozilla/mozbase/mozrunner/tests/test_threads.py", line 69, in test_process_post_stop_via_thread

self.assertNotIn(returncode, [None, 0])

AssertionError: None unexpectedly found in [None, 0]

Comment 10

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

https://github.com/mozilla/mozbase/commit/f07dcdffcb0b69900f0b2f4cf3db68f75a951b6f (backout)

Comment 11

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

So we basically depend at least on bug 967782 and bug 968718 here. I'm not sure what else could come.

Andrew, I hope that you would be fine with the workaround which I will upload in a bit, so that we can get a working mozrunner and our mozmill 2.0.5 release out, which we really need. And once the dependencies of this bug have been fixed, we could fully revert the workarounds.

Comment 12

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

Attachment: Patch v2

Not tested across platforms yet. Will ask for review once that has been done.

Comment 13

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

Attachment: Patch v2.1 (with workarounds)

I missed to revert a change which I made for testing purposes. So a single test was failing. The patch as of now passes all the tests.

Comment 14

[Andrei Eftimie]

Comment on attachment 8371338 [details] [diff] [review]
Patch v2

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

This is still failing for me with the issue from bug 966234

Comment 15

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

Andrei, for me this patch works fine. Please make sure that your machine has been correctly setup, and give me details of what you are running and how. Your comment doesn't tell me anything.

Comment 16

[Andrei Eftimie]

The testcase from bug 966234 does pass, and I am able to run our tests with a set profile without problems.


But running a testrun still gives the error.
I've disected which dependencies have been updated and the regressor appears to be a combination of ManifestDestiny, mozfile and mozInstall. This looks like a different issue.

Comment 17

[Andrei Eftimie]

Comment on attachment 8371338 [details] [diff] [review]
Patch v2

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

All good!

Patched mozmill-automation to require the latest versions of its dependencies 
>'mozdownload==1.11.1',
>'mozfile == 1.1',
>'mozinstall == 1.9',
and I can safely run both functional and remote testruns.

Comment 18

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

Comment on attachment 8371343 [details] [diff] [review]
Patch v2.1 (with workarounds)

Looks like William and Andrew are out the whole week. Jonathan, would you mind to review? Also adding Joel in case he could do it.

So it might be that we do not need this patch with workarounds if we can get the depending bugs fixed. Might be that we could do that until tomorrow. But for safety I would like to get a review on that one too. Just in case it will not work.

Comment 19

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

Comment on attachment 8371343 [details] [diff] [review]
Patch v2.1 (with workarounds)

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

overall, this is a workaround which appears ok, but i would rather we fix mozprocess (0.17) and go that route.

Comment 20

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8371343 [details] [diff] [review]
Patch v2.1 (with workarounds)

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

Agree with Joel; if we land this we should file another bug to revert this once mozprocess is fixed.

Comment 21

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

The workaround should not be necessary. I was able to find patches for both the issues I have brought up here. So once the remaining one has been reviewed and possible comments addressed, we can release 0.17. Then this patch will have to be updated to depend on 0.17 instead of 0.16.

Comment 22

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

Comment on attachment 8371343 [details] [diff] [review]
Patch v2.1 (with workarounds)

All dependencies have been fixed. So the workarounds are not necessary anymore. I will come up with an update for the initial patch with mozprofile=0.17.

Comment 23

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

Attachment: Patch v3.1

Final patch which only contains the version bump and the fix for the returncode property.

Andrei, would you mind to do a final pass over if all is fine, and nothing else came up? Thanks.

Comment 24

[Andrei Eftimie]

Windows looks fine.

I've also used another OSX box (10.8) and on my regular 10.9 box.
This fails for running our restart tests from mozmill-tests

Here are more complete steps. Adding them in this more complete form. Maybe I am missing something...

Steps:
1) installed pip && vitualenv // didn't have these on the 10.8 machine
2) $ virtualenv env
3) $ . env/bin/activate
4) $ git clone http://github.com/mozilla/mozmill
5) $ git clone http://github.com/mozilla/mozbase
6) $ hg clone http://hg.mozilla.org/qa/mozmill-tests
7) $ cd mozmill
8) $ ./setup_development.py
9) $ cd ../mozbase
10)download patch from https://bugzilla.mozilla.org/attachment.cgi?id=8372112 in ../patches
11)$ git checkout -b 967595 
12)$ git am ../patches/0001-Bug-967595-mozrunner-Calling-process_handler.proc.po.patch
13)$ ./setup_development.py
14)$ cd ../
15)$ mozmill -m mozmill-tests/firefox/tests/functional/restartTests/manifest.ini -b /Applications/FirefoxNightly.app/ -p profile

This fails.

Comment 25

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

I ran dozen of tests and I haven't seen this failure at all. So please check your created venv in detail and especially the versions which are used. Keep in mind that running setup_development.py for mozbase after mozmill's one causes mozbase packages to be downloaded from pypi, and not been replaced afterward. This can cause exactly those failures.

Also more comments about this should go onto the Mozmill bug. Here a simple final confirmation should be made.

Comment 26

[Andrei Eftimie]

Comment on attachment 8372112 [details] [diff] [review]
Patch v3.1

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

Runs great!

Thanks for the help of IRC.

> Keep in mind that running setup_development.py for mozbase after mozmill's one causes mozbase packages to be downloaded from pypi, and not been replaced afterward. This can cause exactly those failures.
I think this is exactly what has happened.

To confirm now, this is running great on OSX.

Comment 27

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

https://github.com/mozilla/mozbase/commit/2636578efc5b96c813350a93bc4d0cf0c05f0337

An automated test is hard to do in mozrunner. We would need an extension which will restart the application after a couple of seconds. For now I would say we take our Mozmill tests as tests.