Bug 158105 – Capture screenshot on timeout in webkitpy

• Status: ASSIGNED
• Component: Tools / Tests

[Aakash Jain]

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).

[Aakash Jain]

also see <rdar://problem/26479795>

[Aakash Jain]

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.

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

For example, https://bugs.webkit.org/show_bug.cgi?id=153993 made this script work on GTK.

[Carlos Alberto Lopez Perez]

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.

[Alexey Proskuryakov]

Comment on attachment 279858 [details]
Proposed patch

Marking r- because of the above comments.

[Aakash Jain]

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.

[Carlos Alberto Lopez Perez]

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

[Alexey Proskuryakov]

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.

[Aakash Jain]

Created attachment 281325 [details]
Updated patch

Rewritten timeout.py myself, incorporated review comments.

[Daniel Bates]

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()?