[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

Daniel Bates <dbates at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #284731|review?                     |review+
              Flags|                            |

--- Comment #38 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 284731
  --> https://bugs.webkit.org/attachment.cgi?id=284731
Patch

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

> Tools/WebKitTestRunner/TestInvocation.cpp:47
> +#if PLATFORM(GTK) || PLATFORM(EFL)
> +#include <unistd.h>
> +#endif

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.

> Tools/WebKitTestRunner/TestInvocation.cpp:197
> -    char errorMessageToStderr[1024];
> +    char buffer[1025];

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.

> Tools/WebKitTestRunner/TestInvocation.cpp:198
> +    memset(buffer, 0, sizeof(buffer));

Minor: This is OK as-is. We can use aggregate initialization syntax to initialize the array to 0:

char buffer[1024] = { 0 };

> Tools/WebKitTestRunner/TestInvocation.cpp:203
> +    snprintf(buffer, sizeof(buffer) - 1, "#PROCESS UNRESPONSIVE - %s\n", TestController::webProcessName());

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] <http://pubs.opengroup.org/onlinepubs/9699919799/functions/fprintf.html>

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

This is not needed as we do not build WebKitTestRunner on Windows.

> Tools/WebKitTestRunner/TestInvocation.cpp:221
> +#else
> +    fputs("Grab an image of the stack, then hit enter...\n", stderr);

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.

-- 
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/20160729/a6b47084/attachment-0001.html>