[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

--- Comment #26 from Jonathan Bedard <jbedard at apple.com> ---
Comment on attachment 284354
  --> https://bugs.webkit.org/attachment.cgi?id=284354
Patch

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

>> Tools/WebKitTestRunner/TestInvocation.cpp:203
>> +        return;
> 
> 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)?

Since it is legal to have a script driving the testing process and not be in server mode (even if this may not be a current use case) I figure that it is safer to check for server mode.  We will never waiting for standard in if we aren't in server mode, so while it may not be needed for current use cases, it seems prudent so that this code doesn't cause hangs later on.

>> Tools/WebKitTestRunner/TestInvocation.cpp:206
>> +    if (isatty(fileno(stdin)))
> 
> 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.

Well, it is possible for a user to pipe standard error to a file, correct?  It seems to me that in this circumstance, where the user pipes standard error into a file by standard in is a terminal, we should still prompt the user, as the application will hang, but at least standard error will have a line describing why.

>> Tools/WebKitTestRunner/TestInvocation.cpp:213
>> +        fputs("Failed to sample process\n", stderr);
> 
> 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);
> }

The way I see this, we have a number of use cases:

Not in server mode: Probably not being run by a script, no stdin expected

Server mode, stdin a terminal, stderr and stdout to pipes: A user has instantiated server mode and is piping the results, stdin expected
Server mode, stdin a pipe, stderr and stdout to pipes: Probably a script.  Stdin again expected
Server mode, stdin a pipe, stderr and stdout to terminals: A user is piping in commands (so stdin expected) but reading the output

I can't think of anyone in their right mind who would use the last use case, I supposed we might should output a line to stderr if either stdin or stderr are terminals

-- 
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/20160726/c13648f1/attachment.html>