Bug 168150 – run-webkit-tests for iOS Simulator always complains that stopping WebKitTestRunnerApp.app times out

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

563$ $ wktests --debug --ios-simulator LayoutTests/compositing/masks/
This machine could support 4 simulators, but is only configured for 3.
Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>.
Using port 'ios-simulator'
Test configuration: <, x86_64, debug>
Placing test results in /Volumes/Data/Development/OSX/webkit/OpenSource/WebKitBuild/Debug-iphonesimulator/layout-test-results
Baseline search path: platform/ios-simulator-wk2 -> platform/ios-simulator -> platform/wk2 -> generic
Using Debug build
Pixel tests disabled
Regular timeout: 30000, slow test timeout: 150000
Command line: /Volumes/Data/Development/OSX/webkit/OpenSource/WebKitBuild/Debug-iphonesimulator/WebKitTestRunnerApp.app -

Found 21 tests; running 19, skipping 2.

Running 19 tests

Creating app:/tmp/WebKitTestingSimulators/Simulator0.app
/tmp/WebKitTestingSimulators/Simulator0.app: replacing existing signature
Waiting for all iOS Simulators to finish booting.
Running 1 WebKitTestRunnerApp.app.

[16/19] compositing/masks/solid-color-masked.html passed unexpectedly                               
stopping WebKitTestRunnerApp.app(pid 84992) timed out, killing it   <------------------- here

18 tests ran as expected, 1 didn't:


Expected to fail, but passed: (1)
  compositing/masks/solid-color-masked.html


I also this this when running DRT with -1.

[Radar WebKit Bug Importer]

<rdar://problem/30475467>

[Jonathan Bedard]

I can fix this, and I will shortly upload a patch which does.

That being said, I'm not sure this is a bug.  This happens because simctl terminate does not, despite it's name, terminate the app it is given.  It simply sends it to a background state.  As a result, the process is still running.

We could send the process a SIGTERM after using simctl's api to terminate the app and it's process.

[Jonathan Bedard]

Created attachment 301213 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 301213 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301213&action=review

> Tools/Scripts/webkitpy/port/simulator_process.py:134
> +        os.kill(self._pid, signal.SIGTERM)

Do we need to give it some time to terminate? Overall, this sequence seems confusing.

[Jonathan Bedard]

Comment on attachment 301213 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301213&action=review

>> Tools/Scripts/webkitpy/port/simulator_process.py:134
>> +        os.kill(self._pid, signal.SIGTERM)
> 
> Do we need to give it some time to terminate? Overall, this sequence seems confusing.

I don't believe so.

Basically what's going on here is that self._device.terminate_app will not stop the app process.  SIGTERM should stop the process, if it does not, we will pass the pid to our executive class in server_process which does a more involved kill sequence which includes SIGKILL as well as SIGTERM.  This sequence will prevent the lines that Simon is seeing from being logged unless the processes failed to respond to the SIGTERM as expected.

As I mentioned, I don't really think this is broken at the moment, but if we wanted to suppress the logging line Simon is seeing, this would be the correct way to do so.

[Alexey Proskuryakov]

If we don't need to give it time, why do we need to call self._device.terminate_app right before sending the signal?

> As I mentioned, I don't really think this is broken at the moment, but if we wanted to suppress the logging line Simon is seeing, this would be the correct way to do so.

We do not want spurious error messages in run-webkit-tests output, as that's super misleading. I suspect that it wastes time waiting for the timeout too.

[Jonathan Bedard]

Created attachment 301361 [details]
Patch

[Jonathan Bedard]

(In reply to comment #6)

Ok, this is actually an issue.

I've updated the patch.  The timeout is real, and is a regression caused by <http://trac.webkit.org/changeset/211370>.  It only would have effected things when the app being closed at the end of a test suite.

What I said in comment #1 is also true, hence the need to both terminate the app and terminate the process.  This is done to give the app a chance to gracefully exit before terminating the process.

[Alexey Proskuryakov]

> This is done to give the app a chance to gracefully exit before terminating the process.

To give it a fair chance, how long should we wait before terminating?

[Jonathan Bedard]

(In reply to comment #9)
> > This is done to give the app a chance to gracefully exit before terminating the process.
> 
> To give it a fair chance, how long should we wait before terminating?

Unfortunately, this is going to depend on the app.

DumpRenderTree and WebKitTestRunner should be closed almost instantly.  Does a quarter second pause sound reasonable?

[Alexey Proskuryakov]

I don't know what number is OK, which is part of what makes me question giving the app an opportunity to close nicely. What will break if we always send the signal?

[Jonathan Bedard]

(In reply to comment #11)
> I don't know what number is OK, which is part of what makes me question
> giving the app an opportunity to close nicely. What will break if we always
> send the signal?

With the current implementation of DumpRenderTree and WebKitTestRunner, nothing will break.  Some other app may fail to save assets, gracefully close sockets, that sort of thing.  But in the case of DumpRenderTree and WebKitTestRunner, sending a SIGTERM shouldn't cause an problems.

[Alexey Proskuryakov]

In this case, I suggest just always sending the signal, as we don't give the process any time to orderly terminate anyway.

[Jonathan Bedard]

Created attachment 301402 [details]
Patch

[Daniel Bates]

Comment on attachment 301402 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301402&action=review

> Tools/Scripts/webkitpy/port/simulator_process.py:-133
> -        self._device.terminate_app(self._bundle_id)

Last I recall this was the only function that called Device.terminate_app(). Is there reason to keep this function around?

[Jonathan Bedard]

Comment on attachment 301402 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301402&action=review

>> Tools/Scripts/webkitpy/port/simulator_process.py:-133
>> -        self._device.terminate_app(self._bundle_id)
> 
> Last I recall this was the only function that called Device.terminate_app(). Is there reason to keep this function around?

It is not used anywhere else.

Removing.

[Jonathan Bedard]

Created attachment 301411 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 301411 [details]
Patch

Clearing flags on attachment: 301411

Committed r212297: <http://trac.webkit.org/changeset/212297>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.