<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#c18">Comment # 18</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=284131&action=diff" name="attach_284131" title="Patch">attachment 284131</a> <a href="attachment.cgi?id=284131&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=284131&action=review">https://bugs.webkit.org/attachment.cgi?id=284131&action=review</a>
<span class="quote">> Tools/ChangeLog:7
> +</span >
Please explain the motivation for this change. From what I can tell the spindump(8) report is the concatenation of sample(1)ing all running processing plus some additional data. I take it that our interest in a spindump report is to capture the state of other WebKit processes (e.g. the Network process) when a WebContent process hangs to help diagnose a hard-to-reproduce test failure.
<span class="quote">> Tools/Scripts/webkitpy/port/driver.py:192
> + if driver_input.test_name.startswith('http://') or driver_input.test_name.startswith('https://') or driver_input.test_name.startswith('file:///'):
> + self._test_name = self.uri_to_test(driver_input.test_name)
> + else:
> + self._test_name = driver_input.test_name</span >
See my remark in _check_for_spindump_trigger() and in run_spindump() (defined in ios.py) that suggest making use of the existing machinery for sample reports. If we can make use of this machinery then we can remove this code.
<span class="quote">> Tools/Scripts/webkitpy/port/driver.py:440
> + if not error_line.startswith("#TRIGGER SPINDUMP - "):
> + return False
> +
> + match = re.match('#TRIGGER SPINDUMP - (\S+)', error_line)
> + child_process_name = match.group(1) if match else 'WebProcess'
> + match = re.search('pid (\d+)', error_line)
> + child_process_pid = int(match.group(1)) if match else None
> + if child_process_pid:
> + self._port.run_spindump(self._test_name, child_process_pid)
> + self._server_process.write('#SPINDUMP FINISHED\n')
> + return True</span >
Is it necessary to have separate logic for spin dumps? I mean, this patch unconditionally replaces the machinery for sampling a process using sample(1) with machinery to call spindump(8). I do not see the need to only do one or the other, especially given that spindump(8) can only be run by root. Could we make use of the existing "PROCESS UNRESPONSIVE" message and incorporate the spin dump logic into Port.sample_process()? Could we incorporate the spin dump logic into Port.sample_process() such that we first call spindump(8) and if that command returns a non-zero exit status (say, the current user does not have password-less sudo access) then we call sample(1) on the WebContent process as we do today?
<span class="quote">> Tools/Scripts/webkitpy/port/ios.py:432
> + parsed_filename = os.path.splitext(test_name)[0]
> + path_location = os.path.split(parsed_filename)[0]
> +
> + if not os.path.exists(self.results_directory()):
> + os.makedirs(self.results_directory())
> + if not os.path.exists(self.results_directory() + '/' + path_location):
> + os.makedirs(self.results_directory() + '/' + path_location)</span >
Can we make use of the existing machinery (*) that copies sample(1) reports to the appropriate place in the results directory? If it makes sense to incorporate the spin dump logic into Port.sample_process() then we likely can pass Port.sample_file_path(..., ...) as the path for the -file argument to spindump(8) and make use of the existing machinery the collect samples and moves then to be near the test results for a failed test.
(*) After running a test Manager._look_for_new_crash_logs() is called to collect crash reports and sample reports. Manager._look_for_new_crash_logs() calls Port.look_for_new_samples() for the list of sample reports that were written and uses TestResultWriter.copy_sample_file() to copy each sample report to the appropriate place in the results directory.
<span class="quote">> Tools/WebKitTestRunner/TestInvocation.cpp:212
> + char spindumpMessage[1024];
> +#if PLATFORM(COCOA)
> + pid_t pid = WKPageGetProcessIdentifier(TestController::singleton().mainWebView()->page());
> + snprintf(spindumpMessage, 1024, "#TRIGGER SPINDUMP - %s (pid %ld)\n", TestController::webProcessName(), static_cast<long>(pid));
> +#else
> + snprintf(spindumpMessage, 1024, "#TRIGGER SPINDUMP - %s\n", TestController::webProcessName());
> +#endif</span >
It it necessary to emit a new message here "TRIGGER SPINDUMP"? Could we make use of the existing "#PROCESS UNRESPONSIVE" message and teach run-webkit-test to call spindump(8) when it sees this message instead of sample(1) on applicable platforms?
<span class="quote">> Tools/WebKitTestRunner/TestInvocation.cpp:215
> + if (!TestController::singleton().usingServerMode())
> + return;</span >
Can you elaborate on this?
<span class="quote">> Tools/WebKitTestRunner/TestInvocation.cpp:216
> + fputs("If running manually, hit enter...\n", stderr);</span >
Is this a workflow that most people will make use of? Running WebKitTestRunner in server mode and wanting to invoke spindump(8) by hand?
<span class="quote">> Tools/WebKitTestRunner/TestInvocation.cpp:218
> + if (!fgets(spindumpMessage, 1024, stdin) || strcmp(spindumpMessage, "#SPINDUMP FINISHED\n"))</span >
Is it necessary to look for the message "#SPINDUMP FINISHED\n"? Would it be sufficient to use fgetc(3) to read a single newline character or EOF character and teach run-webkit-tests to emit a single newline character?
<span class="quote">> Tools/WebKitTestRunner/TestInvocation.cpp:219
> + fputs("Spindump return failure\n", stderr);</span >
I take it that this output is helpful.</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>