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.
<rdar://problem/30475467>
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.
Created attachment 301213 [details] Patch
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.
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.
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.
Created attachment 301361 [details] Patch
(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.
> 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?
(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?
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?
(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.
In this case, I suggest just always sending the signal, as we don't give the process any time to orderly terminate anyway.
Created attachment 301402 [details] Patch
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?
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.
Created attachment 301411 [details] Patch
Comment on attachment 301411 [details] Patch Clearing flags on attachment: 301411 Committed r212297: <http://trac.webkit.org/changeset/212297>
All reviewed patches have been landed. Closing bug.