Closed Bug 960421 Opened 6 years ago Closed 5 years ago

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

Categories

(Testing Graveyard :: Mozmill, defect, critical)

All
macOS
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: whimboo, Assigned: whimboo)

References

()

Details

(Whiteboard: [mozmill-2.1+])

Attachments

(1 file, 2 obsolete files)

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.
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.
Blocks: 960495
Whiteboard: [mozmill-2.0.4+] → [mozmill-2.0.4+][mentor=whimboo][lang=py]
This bug was filed because I see this on OS X.
OS: All → Mac OS X
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.
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.
Assignee: nobody → sammygamer
Status: NEW → ASSIGNED
Attached patch This is the fix for the bug. (obsolete) — Splinter Review
Attachment #8366602 - Flags: review?(hskupin)
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.
Attachment #8366602 - Flags: review?(hskupin) → review-
Attached patch Indentation problem fixed (obsolete) — Splinter Review
Attachment #8366602 - Attachment is obsolete: true
Attachment #8366608 - Flags: review?(hskupin)
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.
Attachment #8366608 - Flags: review?(hskupin) → review-
Attached patch Updated bug fixSplinter Review
Attachment #8366608 - Attachment is obsolete: true
Attachment #8366646 - Flags: review?(hskupin)
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.
Blocks: 965216
No longer blocks: 960495
Whiteboard: [mozmill-2.0.4+][mentor=whimboo][lang=py] → [mozmill-2.0.5+][mentor=whimboo][lang=py]
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.
Whiteboard: [mozmill-2.0.5+][mentor=whimboo][lang=py] → [mozmill-2.1?][mentor=whimboo][lang=py]
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.
Attachment #8366646 - Flags: review?(hskupin) → review-
Depends on: 794020
No longer blocks: 965216
Blocks: 970594
Whiteboard: [mozmill-2.1?][mentor=whimboo][lang=py] → [mozmill-2.1?][mentor=whimboo][lang=py][good first bug]
Mentor: hskupin
Whiteboard: [mozmill-2.1?][mentor=whimboo][lang=py][good first bug] → [mozmill-2.1?][lang=py][good first bug]
Is work still being done on this bug?  if not, lets unassign it.
Flags: needinfo?(hskupin)
We are currently blocked on bug 794020. I hope to finally get to it soon, so we can continue here.
Flags: needinfo?(hskupin)
Assignee: sambuddhabasu1 → hskupin
Whiteboard: [mozmill-2.1?][lang=py][good first bug] → [mozmill-2.1+]
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.
Mentor: hskupin
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → WORKSFORME
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.