[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 #25 from Daniel Bates <dbates at webkit.org> ---
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/ChangeLog:7
> +

As implied by my remark in comment #18, please explain the motivation of this change in this ChangeLog.

> 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

Please use complete sentences that end in a period.

> Tools/Scripts/webkitpy/port/driver.py:447
> +            self._server_process.write('#SAMPLE FINISHED\n')

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.

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

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

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

Ditto.

> Tools/WebKitTestRunner/TestInvocation.cpp:203
> +    if (!TestController::singleton().usingServerMode())
> +        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)?

> 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.

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

We should be checking if standard error is attached to a tty device before we emit this message just as we do above.

> Tools/WebKitTestRunner/TestInvocation.cpp:213
> +#endif
> +    
> +    if (!fgets(buffer, sizeof(buffer), stdin) || strcmp(buffer, "#SAMPLE FINISHED\n"))
> +        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);
}

-- 
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/09f8e047/attachment.html>