<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><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> 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 #284731 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#c38">Comment # 38</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=284731&action=diff" name="attach_284731" title="Patch">attachment 284731</a> <a href="attachment.cgi?id=284731&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=284731&action=review">https://bugs.webkit.org/attachment.cgi?id=284731&action=review</a>
<span class="quote">> Tools/WebKitTestRunner/TestInvocation.cpp:47
> +#if PLATFORM(GTK) || PLATFORM(EFL)
> +#include <unistd.h>
> +#endif</span >
Please unconditionally include this header as this header contains the isatty() declaration on macOS. Obviously some other header h is including unistd.h when building for Mac and iOS Simulator as indicated by EWS success bubbles. Regardless, we should explicitly include this header in this file to avoid build breakage should unistd.h be removed from h.
<span class="quote">> Tools/WebKitTestRunner/TestInvocation.cpp:197
> - char errorMessageToStderr[1024];
> + char buffer[1025];</span >
I do not understand the need to increase the size of the buffer by one character from 1024 to 1025. It seems sufficient to use 1024.
<span class="quote">> Tools/WebKitTestRunner/TestInvocation.cpp:198
> + memset(buffer, 0, sizeof(buffer));</span >
Minor: This is OK as-is. We can use aggregate initialization syntax to initialize the array to 0:
char buffer[1024] = { 0 };
<span class="quote">> Tools/WebKitTestRunner/TestInvocation.cpp:203
> + snprintf(buffer, sizeof(buffer) - 1, "#PROCESS UNRESPONSIVE - %s\n", TestController::webProcessName());</span >
It is sufficient to pass sizeof(buffer) for the second argument to snprintf(). snprintf() guarantees that the buffer will be null terminated on success with respect to our usage by [1]. So, it is sufficient to write:
snprintf(buffer, sizeof(buffer), ...);
[1] <<a href="http://pubs.opengroup.org/onlinepubs/9699919799/functions/fprintf.html">http://pubs.opengroup.org/onlinepubs/9699919799/functions/fprintf.html</a>>
<span class="quote">> Tools/WebKitTestRunner/TestInvocation.cpp:216
> +#elif PLATFORM(WIN)
> + if (_isatty(fileno(stdin)) || _isatty(fileno(stderr)))
> + fputs("Grab an image of the stack, then hit enter...\n", stderr);</span >
This is not needed as we do not build WebKitTestRunner on Windows.
<span class="quote">> Tools/WebKitTestRunner/TestInvocation.cpp:221
> +#else
> + fputs("Grab an image of the stack, then hit enter...\n", stderr);</span >
If we unconditionally include header unistd.h then I do not see the need for this special case as all platforms we build on will have isatty(). Moreover, I do not feel it provides much value to have platform-specific manual instructions. It seems sufficient to make the instructions generic and then we can remove all of the platform-guards, which clutter this code and make it less readable.</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>