[Webkit-unassigned] [Bug 159827] run-webkit-tests should trigger a spindump when WebContent process is unresponsive

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=159827

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #283805|review?                     |review-
              Flags|                            |

--- Comment #5 from David Kilzer (:ddkilzer) <ddkilzer at webkit.org> ---
Comment on attachment 283805
  --> https://bugs.webkit.org/attachment.cgi?id=283805
Patch

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

Overall this looks good, but please post a revised patch addressing the comments below.  r- to address feedback.

> Tools/ChangeLog:19
> +        (WTR::TestInvocation:: triggerSpindump): Exchange spindump data with python through stdin/stderr

Spindump logs can be quite large.  Where will this text output show up in the test results, as stderr output for the test?

It's okay to send the contents via stderr as an initial experiment (so I'm not asking you to rewrite this patch), but if we find it useful (or it happens more often than we think), we should consider writing the spindump log to a file, and adding the file to the results page so it's a separate entity.  (This also makes it easy to create a Safari extension to process the spindump log when visiting a test results page, rather than parsing it out of a chunk of stderr text.)

> Tools/Scripts/webkitpy/port/driver.py:38
>  import sys
>  import time
>  import os
> +import subprocess

Nit: We try to keep these sorted in alphabetical order.  Please put 'subprocess' between 'shlex' and 'sys', or just re-alphabetize both.

> Tools/Scripts/webkitpy/port/driver.py:436
> +        match = re.search('pid (\d+)', error_line)

Is it worth matching the parenthesis here?  (pid NNNN)

No change necessary to land the patch; just asking the question since it may reduce the off chance of a false-positive match.

> Tools/Scripts/webkitpy/port/driver.py:445
> +            parsed_filename = os.path.splitext(self._test_name)[0]
> +            path_location = os.path.split(parsed_filename)[0]
> +
> +            if not os.path.exists(self._port.results_directory()):
> +                os.makedirs(self._port.results_directory())
> +            if not os.path.exists(self._port.results_directory() + '/' + path_location):
> +                os.makedirs(self._port.results_directory() + '/' + path_location)

Was this code copied from somewhere?  If so, seems like it should be pulled into a new method.  (Not required to fix to land this patch.)

> Tools/WebKitTestRunner/TestInvocation.cpp:211
> +    sprintf(spindumpMessage, "#TRIGGER SPINDUMP - %s (pid %ld)\n", TestController::webProcessName(), static_cast<long>(pid));
> +#else
> +    sprintf(spindumpMessage, "#TRIGGER SPINDUMP - %s\n", TestController::webProcessName());

I know dumpWebProcessUnresponsiveness() doesn't use it, but please use snprintf() here and pull the length into a 'const size_t length' variable.

> Tools/WebKitTestRunner/TestInvocation.cpp:219
> +    if (!fgets(spindumpMessage, 1024, stdin)
> +        || strcmp(spindumpMessage, "#SPINDUMP FINISHED\n"))

Formatting: IIRC, this should be on one line, or braces should be used for the body of the if statement.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160718/5b2e5e21/attachment-0001.html>