834635 - [Mac] mozmill-restart fails to force kill the application in the middle of a testrun if application is busted

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

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

Because of bug 834432 we had busted l10n builds. Those caused a yellow-screen-of-death window to appear instead of the browser window. In those cases we timeout but Mozmill is not able to kill those applications on OS X. Linux and Windows platforms are working fine.

Steps to reproduce:

1. Download a broken l10n nightly build which raises a yellow-screen-of-death
window from here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013/01/2013-01-24-05-41-58-mozilla-central-l10n/

2. Clone our mozmill tests repository 

3. Execute mozmill like: mozmill -b
/Volumes/Nightly/FirefoxNightly.app/Contents/MacOS/firefox -t
tests/functional/testToolbar/

I'm going to investigate that now. It's clearly a blocker for us!

Comment 1

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

Each time we want to restart Firefox in a restart test I can see the following abort message in the console:

###!!! ABORT: file ../../../../ipc/chromium/src/base/thread_local_posix.cc, line 33

http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/thread_local_posix.cc#33

30 // static
31 void ThreadLocalPlatform::SetValueInSlot(SlotType& slot, void* value) {
32   int error = pthread_setspecific(slot, value);
33   CHECK(error == 0);
34 }

Not sure how much that affects us but I would leave this beside for now.

Comment 2

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

So this only applies to mozmill-restart. It works fine for the mozmill command.

Comment 3

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

For restart tests it only happens in the middle of a testrun. It's enough to even have two skipped tests only. The instance of the application used for test1.js will still remain.

Comment 4

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

So the problem is located here:
https://github.com/mozilla/mozmill/blob/hotfix-1.5/mozrunner/mozrunner/killableprocess.py#L312

As given by the timeout we should run the loop by default 30s, but by multiplying the count with two, we exit the loop already after 15s. This seems to be plainly wrong to me. 

Further it will fail later here:

https://github.com/mozilla/mozmill/blob/hotfix-1.5/mozmill/mozmill/__init__.py#L495

Given that we are back too early we do not properly stop the runner, and the application is not getting closed.

I will have to check how Mozmill 2 handles that code and if I can simply c&p it.

Comment 5

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

Attachment: Patch v1

Least invasive change to Mozrunner on the hotfix-1.5 branch. Jeff, if you think we should do something better please propose but I'm worried that we could break something.

In current mozprocess we do not differentiate between linux and mac and always call the `os.waitpid()` method. But as mentioned above I'm not inclined to change that without causing other regressions.

Comment 6

[Jeff Hammel]

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

+                        while (count <= timeout):

No need for ()s here.  Please remove.  Otherwise, lgtm. I vaguely recall looking at this code and thinking it was, at best, strange

Comment 7

[Jeff Hammel]

(In reply to Henrik Skupin (:whimboo) from comment #5)
<snip/>
> In current mozprocess we do not differentiate between linux and mac and
> always call the `os.waitpid()` method. But as mentioned above I'm not
> inclined to change that without causing other regressions.

Is this a bug? Worth filing?

Comment 8

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

Attachment: patch v1.1

Patch updated. Please also set the review flag if it's fine. :)

(In reply to Jeff Hammel [:jhammel] from comment #7)
> > In current mozprocess we do not differentiate between linux and mac and
> > always call the `os.waitpid()` method. But as mentioned above I'm not
> > inclined to change that without causing other regressions.
> 
> Is this a bug? Worth filing?

It seems to work there and we do not hit this problem. But I will make sure to test this once 1.5.20 has been released.

Comment 9

[Jeff Hammel]

> It seems to work there and we do not hit this problem. But I will make sure to test this once 1.5.20 has been released.

Awesome! Thanks.

Comment 10

[Jeff Hammel]

Comment on attachment 706439 [details] [diff] [review]
patch v1.1

Looks good!

Comment 11

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

Fix landed for hotfix-1.5 branch:
https://github.com/mozilla/mozmill/commit/5c270c73567a279444d5cc1d3e3d52cb76cba9aa

I will leave this bug open for further investigation of Mozmill 2.0.

Comment 12

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

I cannot see that Mozmill 2.0 is affected by this bug. But there is bug 794020 which can clearly be reproduced with those builds. Clearly another thing.

Closing bug given that it is fixed for Mozmill 1.5.