When the layout-tests fails due to Timeout, we should take a screenshot. It would help in easily debugging timeout issues, especially the one in which there is a error message or dialog box on UI (which is not captured in logs).
also see <rdar://problem/26479795>
Created attachment 279858 [details] Proposed patch Saving the screenshot locally, we should consider uploading the screenshot to the server. Will probably do that in a separate patch.
Comment on attachment 279858 [details] Proposed patch I think screencapture is a Mac software and won't work on non Mac platforms. In this case we shouldn't try to run a non existent program and spam the user with warnings.
For example, https://bugs.webkit.org/show_bug.cgi?id=153993 made this script work on GTK.
Comment on attachment 279858 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=279858&action=review > Tools/Scripts/webkitpy/benchmark_runner/utils.py:63 > def handle_timeout(self, signum, frame): > + self.capture_screenshot() > raise TimeoutError(self.error_message) Maybe capture_screenshot() can be implemented as an abstract method of the class BrowserDriver() (at browser_driver.py) that just passes, and then implement the platform specific code in each one of the OS drivers? (for Mac it will be osx_browser_driver.py). I can follow up with an implementation for GTK (gtk_browser_driver.py) after this patch lands.
Comment on attachment 279858 [details] Proposed patch Marking r- because of the above comments.
Created attachment 280346 [details] Updated patch BrowserDriver doesn't seems related to layout-tests, benchmark_runner seems like a separate tool and we shouldn't probably modify it for layout-tests. Here is another implementation, please review.
Comment on attachment 280346 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=280346&action=review > Tools/Scripts/webkitpy/common/system/timeout.py:13 > +# Copyright (c) 2016, Apple Inc. All rights reserved. > +# > +# Redistribution and use in source and binary forms, with or without > +# modification, are permitted provided that the following conditions are > +# met: > +# > +# * Redistributions of source code must retain the above copyright > +# notice, this list of conditions and the following disclaimer. > +# * Redistributions in binary form must reproduce the above > +# copyright notice, this list of conditions and the following disclaimer > +# in the documentation and/or other materials provided with the > +# distribution. > +# * Neither the name of Google Inc. nor the names of its Just a nitpick ... I guess you mean here "Google Inc." => "Apple Inc." (or just "copyright holder") https://opensource.org/licenses/BSD-3-Clause
Comment on attachment 280346 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=280346&action=review > Tools/Scripts/webkitpy/common/system/timeout.py:1 > +# Copyright (c) 2016, Apple Inc. All rights reserved. The proper format is: Copyright (C) 2016 Apple Inc. All rights reserved. >> Tools/Scripts/webkitpy/common/system/timeout.py:13 >> +# * Neither the name of Google Inc. nor the names of its > > Just a nitpick ... > I guess you mean here "Google Inc." => "Apple Inc." (or just "copyright holder") > https://opensource.org/licenses/BSD-3-Clause Is this new code, or copied form another file? I do not know whether it's right to keep the license as is (also do we need Google copyright at the top?) > Tools/Scripts/webkitpy/common/system/timeout.py:39 > +class TimeOut(object): With this spelling, it's "time out", a verb. I think that the class name should be "Timeout" > Tools/Scripts/webkitpy/common/system/timeout.py:63 > +# 'http://stackoverflow.com/questions/2281850/timeout-function-if-it-takes-too-long-to-finish' I don't know if the license is compatible.
Created attachment 281325 [details] Updated patch Rewritten timeout.py myself, incorporated review comments.
Comment on attachment 281325 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=281325&action=review > Tools/Scripts/webkitpy/common/system/timeout.py:53 > +class Timeout(object): > + > + def __init__(self, timeout_seconds=60): > + self.timeout_seconds = timeout_seconds > + self.LOGS_DIRECTORY = "/tmp/WebkitLogs/" > + if not os.path.exists(self.LOGS_DIRECTORY): > + os.mkdir(self.LOGS_DIRECTORY) > + > + def timeout_handler(self, signal_number, frame): > + self.collect_logs() > + raise RuntimeError('Timeout happenned after {0} seconds'.format(self.timeout_seconds)) > + > + def __enter__(self): > + signal.signal(signal.SIGALRM, self.timeout_handler) > + signal.alarm(self.timeout_seconds) > + > + def __exit__(self, exception_type, exception_value, traceback): > + signal.alarm(0) > + > + def collect_logs(self): > + pass This class is almost identical to the class timeout in Tools/Scripts/webkitpy/benchmark_runner/utils.py. We should not duplicate node. It seems sufficient to have the constructor of class webkitpy.benchmark_runner.util.timeout take an optional callback function as an argument that is invoked on expiration of the timeout duration and then define a callback function in Tools/Scripts/webkitpy/xcode/simulator.py that prints debug information, captures a screenshot and collects a spin dump. > Tools/Scripts/webkitpy/common/system/timeout.py:64 > + xcrun_output = subprocess.check_output(['xcrun', 'simctl', 'list']) Can we make use of PlatformInfo.xcrun_simctl_list()?