<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:ddkilzer@webkit.org" title="David Kilzer (:ddkilzer) <ddkilzer@webkit.org>"> <span class="fn">David Kilzer (:ddkilzer)</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - run-webkit-tests should trigger a spindump when WebContent process is unresponsive"
href="https://bugs.webkit.org/show_bug.cgi?id=159827">bug 159827</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #283805 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - run-webkit-tests should trigger a spindump when WebContent process is unresponsive"
href="https://bugs.webkit.org/show_bug.cgi?id=159827#c5">Comment # 5</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - run-webkit-tests should trigger a spindump when WebContent process is unresponsive"
href="https://bugs.webkit.org/show_bug.cgi?id=159827">bug 159827</a>
from <span class="vcard"><a class="email" href="mailto:ddkilzer@webkit.org" title="David Kilzer (:ddkilzer) <ddkilzer@webkit.org>"> <span class="fn">David Kilzer (:ddkilzer)</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=283805&action=diff" name="attach_283805" title="Patch">attachment 283805</a> <a href="attachment.cgi?id=283805&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=283805&action=review">https://bugs.webkit.org/attachment.cgi?id=283805&action=review</a>
Overall this looks good, but please post a revised patch addressing the comments below. r- to address feedback.
<span class="quote">> Tools/ChangeLog:19
> + (WTR::TestInvocation:: triggerSpindump): Exchange spindump data with python through stdin/stderr</span >
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.)
<span class="quote">> Tools/Scripts/webkitpy/port/driver.py:38
> import sys
> import time
> import os
> +import subprocess</span >
Nit: We try to keep these sorted in alphabetical order. Please put 'subprocess' between 'shlex' and 'sys', or just re-alphabetize both.
<span class="quote">> Tools/Scripts/webkitpy/port/driver.py:436
> + match = re.search('pid (\d+)', error_line)</span >
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.
<span class="quote">> 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)</span >
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.)
<span class="quote">> 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());</span >
I know dumpWebProcessUnresponsiveness() doesn't use it, but please use snprintf() here and pull the length into a 'const size_t length' variable.
<span class="quote">> Tools/WebKitTestRunner/TestInvocation.cpp:219
> + if (!fgets(spindumpMessage, 1024, stdin)
> + || strcmp(spindumpMessage, "#SPINDUMP FINISHED\n"))</span >
Formatting: IIRC, this should be on one line, or braces should be used for the body of the if statement.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>