<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<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#c25">Comment # 25</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:dbates@webkit.org" title="Daniel Bates <dbates@webkit.org>"> <span class="fn">Daniel Bates</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=284354&action=diff" name="attach_284354" title="Patch">attachment 284354</a> <a href="attachment.cgi?id=284354&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=284354&action=review">https://bugs.webkit.org/attachment.cgi?id=284354&action=review</a>
<span class="quote">> Tools/ChangeLog:7
> +</span >
As implied by my remark in <a href="show_bug.cgi?id=159827#c18">comment #18</a>, please explain the motivation of this change in this ChangeLog.
<span class="quote">> Tools/ChangeLog:19
> + * Scripts/webkitpy/port/driver.py:
> + (Driver._check_for_driver_crash_or_unresponsiveness): Notify test when finished through stdin
> + * Scripts/webkitpy/port/ios.py:
> + (IOSSimulatorPort):
> + (IOSSimulatorPort.sample_process): Default to spindump, attempt sample if fail
> + * Scripts/webkitpy/port/mac.py:
> + (MacPort):
> + (MacPort.sample_process): Default to spindump, attempt sample if fail
> + * WebKitTestRunner/TestController.h:
> + (WTR::TestController::usingServerMode): Added accessor
> + * WebKitTestRunner/TestInvocation.cpp:
> + (WTR::TestInvocation::dumpWebProcessUnresponsiveness): Wait for stdin before continuing</span >
Please use complete sentences that end in a period.
<span class="quote">> Tools/Scripts/webkitpy/port/driver.py:447
> + self._server_process.write('#SAMPLE FINISHED\n')</span >
OK. Alternatively, we could have taught WebKitTestRunner to suspend itself (raise a SIGSTOP) when it detects an unresponsive process and then run-webkit-tests can send a SIGCONT when its done.
<span class="quote">> Tools/Scripts/webkitpy/port/ios.py:435
> try:
> hang_report = self.sample_file_path(name, pid)
> - self._executive.run_command([
> - "/usr/bin/sample",
> + exit_status = self._executive.run_command([
> + "/usr/bin/sudo",
> + "-n",
> + "/usr/sbin/spindump",
> pid,
> 10,
> 10,
> "-file",
> hang_report,
> - ])
> + ], None, None, None, None, True, False)
> +
> + if not exit_status == 0:
> + self._executive.run_command([
> + "/usr/bin/sample",
> + pid,
> + 10,
> + 10,
> + "-file",
> + hang_report,
> + ])
> except ScriptError as e:
> - _log.warning('Unable to sample process:' + str(e))
> + _log.warning('Unable to run spindump and sample process:' + str(e))</span >
I would write this as:
hang_report = self.sample_file_path(name, pid)
exit_code = self._executive.run_command(['/usr/bin/sudo', '-n', '/usr/sbin/spindump', pid, 10, 10, '-file', hang_report], return_exit_code=True)
if not exit_code:
return
# User does not have password-less sudo access. Fall back to using sample(1).
try:
self._executive.run_command([
"/usr/bin/sample",
pid,
10,
10,
"-file",
hang_report,
])
except ScriptError as e:
_log.warning('Unable to sample process:' + str(e))
<span class="quote">> Tools/Scripts/webkitpy/port/mac.py:316
> - self._executive.run_command([
> - "/usr/bin/sample",
> + exit_status = self._executive.run_command([
> + "/usr/bin/sudo",
> + "-n",
> + "/usr/sbin/spindump",
> pid,
> 10,
> 10,
> "-file",
> hang_report,
> - ])
> + ], None, None, None, None, True, False)
> +
> + if not exit_status == 0:
> + self._executive.run_command([
> + "/usr/bin/sample",
> + pid,
> + 10,
> + 10,
> + "-file",
> + hang_report,
> + ])
> except ScriptError as e:
> - _log.warning('Unable to sample process:' + str(e))
> + _log.warning('Unable to run spindump and sample process:' + str(e))</span >
Ditto.
<span class="quote">> Tools/WebKitTestRunner/TestInvocation.cpp:203
> + if (!TestController::singleton().usingServerMode())
> + return;</span >
OK. Is it necessary to know whether we are running in server mode to know if we should prompt for input for sample completion before terminating the WebContent process? I suspect that waiting for a sample to be taken of an unresponsive WebContent process is mostly of interest to scripts such as run-webkit-tests (for the purpose of implementing it sample on timeout feature). Would it be sufficient to only prompt to sample the WebContent process if standard error is not attached to a tty (e.g. isatty(fileno(stderr)) returns 0)?
<span class="quote">> Tools/WebKitTestRunner/TestInvocation.cpp:206
> + if (isatty(fileno(stdin)))</span >
This does not seem correct. We should be checking whether standard error is a tty device as opposed to standard input because scripts that tend to capture standard output and standard error of a subprocess (like run-webkit-tests) tend to forward standard input to the subprocess.
<span class="quote">> Tools/WebKitTestRunner/TestInvocation.cpp:209
> + fputs("Grab an image of the stack, then hit enter...\n", stderr);</span >
We should be checking if standard error is attached to a tty device before we emit this message just as we do above.
<span class="quote">> Tools/WebKitTestRunner/TestInvocation.cpp:213
> +#endif
> +
> + if (!fgets(buffer, sizeof(buffer), stdin) || strcmp(buffer, "#SAMPLE FINISHED\n"))
> + fputs("Failed to sample process\n", stderr);</span >
Maybe we could simplify the above logic to be:
...
dump(errorMessage, buffer, true);
if (!isatty(fileno(stderr))) {
// Run-webkit-tests may have initiated a process sample of the unresponsive process. We wait for run-webkit-tests to
// signal completion of its sample activity (it will signal regardless of whether it actually takes a process sample)
// before we continue on and terminate the unresponsive process.
if (!fgets(buffer, sizeof(buffer), stdin) || strcmp(buffer, "#SAMPLE FINISHED\n"))
fprintf(stderr, "Failed to receive expected sample finished response. Got \"%s\". Continuing...", buffer);
}</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>