960421 - Mozmill doesn't kill the running application if KeyboardInterrupt error is raised

Status: RESOLVED WORKSFORME

(Testing Graveyard :: Mozmill, defect, critical)

Description

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

I noticed this a lot the last days. Whenever I hit Ctrl-C during a testrun the build is not getting shutdown. This can cause Firefox builds to remain open and blocking the machine from running further testruns.

Comment 1

[Andrei Eftimie]

I can't reproduce this on OSX or on Linux.

This is the case on Windows, but I think the problem there lies with the terminal we are using.

Comment 2

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

This bug was filed because I see this on OS X.

Comment 3

[Sambuddha Basu [:sambuddhabasu1]]

I could reproduce this on OSX.
The normal tests under js/manifest.ini work as expected. But, the restart tests under js/testFrame/testRestart/manifest.ini do not show the desired result. While running the restart tests, when I hit Ctrl+C, the cli exits with a 'TEST-UNEXPECTED-FAIL' error but, the firefox build remains open.
Currently, I'm working to fix this bug.

Comment 4

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Hi Sambuddha! It's great that you want to work on this bug. It's a real blocker for us for the next Mozmill release. So I'm happy to give you any help you need to get this issue fixed. As already said on IRC you had a good start here, that you were already able to reproduce the problem!

I have assigned the bug to you now.

Comment 5

[Sambuddha Basu [:sambuddhabasu1]]

Attachment: This is the fix for the bug.

Comment 6

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Comment on attachment 8366602 [details] [diff] [review]
This is the fix for the bug.

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

Applying the patch fails due to white-space issues. Please see details inline.

::: mozmill/mozmill/__init__.py
@@ +415,4 @@
>  
>                          self.runner.reset()
>                 
> +		except (JSBridgeDisconnectError, KeyboardInterrupt), e:

There are still white-space issues. In line #417 a lot of blanks are left, while in line #418 except has been pushed one position to the left.

@@ +419,4 @@
>  			frame = None
>  			self.handle_disconnect()
> +			if isinstance(e, KeyboardInterrupt):
> +				break

Same here. You might want to disable using tab characters. We use spaces for indentation, and for Python there should be 4 of them.

Comment 7

[Sambuddha Basu [:sambuddhabasu1]]

Attachment: Indentation problem fixed

Comment 8

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Comment on attachment 8366608 [details] [diff] [review]
Indentation problem fixed

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

Even with this patch applied it doesn't work for me when I run the tests in mutt/mutt/tests/js/testRestart/manifest.ini. When I wait until Firefox has been restarted the first time and reopened again, hit ctrl-c the CLI will exit but Firefox is still running.

So i think that the break doesn't help us that much here. Most likely because Firefox gets restarted by itself, and hitting ctrl-c will exit the loop but not kill the newly spawned process.

Comment 9

[Sambuddha Basu [:sambuddhabasu1]]

Attachment: Updated bug fix

Comment 10

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Given that we have to release Mozmill 2.0.4 as soon as possible, I'm going to move this bug for the 2.0.5 release. Reason is that it only affects testruns canceled by the user, and it wont help us in mozmill-ci.

Comment 11

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Given that the 2.0.4 release was busted we have to release 2.0.5 for only bug fixes. I would like to push this bug out to the next feature release.

Comment 12

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Comment on attachment 8366646 [details] [diff] [review]
Updated bug fix

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

I finally was able to test this patch on my OS X box. A testrun for restart tests have shown me that we successfully quit the testrun, but I get a different exception now, which I don't expect to see:

Traceback (most recent call last):
  File "/Volumes/data/code/mozmill/mozmill/mozmill/__init__.py", line 795, in run
    mozmill.run(tests, self.options.restart)
  File "/Volumes/data/code/mozmill/mozmill/mozmill/__init__.py", line 421, in run
    self.handle_disconnect()
  File "/Volumes/data/code/mozmill/mozmill/mozmill/__init__.py", line 479, in handle_disconnect
    self.stop_runner()
  File "/Volumes/data/code/mozmill/mozmill/mozmill/__init__.py", line 525, in stop_runner
    frame = jsbridge.JSObject(self.bridge, js_module_frame)
  File "/Volumes/data/code/mozmill/jsbridge/jsbridge/jsobjects.py", line 73, in __init__
    name = bridge.set(name)['data']
AttributeError: 'NoneType' object has no attribute 'set'

In such a case we should not try to call stop_runner which will send additional information over the bridge. Maybe this bug has to wait until I was able to fix bug 794020, which will add a hard-stop method.

Comment 13

[Joel Maher ( :jmaher ) (UTC-4) (on leave, back November 5th)]

Is work still being done on this bug?  if not, lets unassign it.

Comment 14

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

We are currently blocked on bug 794020. I hope to finally get to it soon, so we can continue here.

Comment 15

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

I re-tested this but I can no longer reproduce it. Hitting Ctrl+C while the restart tests are running successfully closes the application. It even works during a restart of the application, and across all platforms.

There is nothing we can do here, so marking as WFM.