[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 #8 from Jonathan Bedard <jbedard at apple.com> ---
Comment on attachment 283805
  --> https://bugs.webkit.org/attachment.cgi?id=283805
Patch

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

>> Tools/ChangeLog:19
>> +        (WTR::TestInvocation:: triggerSpindump): Exchange spindump data with python through stdin/stderr
> 
> Spindump logs can be quite large.  Where will this text output show up in the test results, as stderr output for the test?
> 
> It's okay to send the contents via stderr as an initial experiment (so I'm not asking you to rewrite this patch), but if we find it useful (or it happens more often than we think), we should consider writing the spindump log to a file, and adding the file to the results page so it's a separate entity.  (This also makes it easy to create a Safari extension to process the spindump log when visiting a test results page, rather than parsing it out of a chunk of stderr text.)

My comments here are misleading, partially because the original implementation did dump the spindump log to stdout.  Currently, the spindump log is written to a file.

>> Tools/Scripts/webkitpy/port/driver.py:436
>> +        match = re.search('pid (\d+)', error_line)
> 
> Is it worth matching the parenthesis here?  (pid NNNN)
> 
> No change necessary to land the patch; just asking the question since it may reduce the off chance of a false-positive match.

It shouldn't be needed, as the #TRIGGER SPINDUMP - (\S+) should have already checked for the matching parenthesis

>> Tools/Scripts/webkitpy/port/driver.py:445
>> +                os.makedirs(self._port.results_directory() + '/' + path_location)
> 
> Was this code copied from somewhere?  If so, seems like it should be pulled into a new method.  (Not required to fix to land this patch.)

It was not copied, I'm just not sure how much of it is needed (or exactly how the 'makedris' function works in Python).  The purpose of this code is to guarantee that the location spindump is sending it's output to actually exists.  It seems to me to be connected closely enough to the detection of the trigger output from the C++ program to justify being one function, although it may be sensible to separate the trigger detection from the actual spindump call, particularly if it turns out we want to call spindump from other places as well.

>> Tools/Scripts/webkitpy/port/driver.py:452
>> +                read_line = spindump_process.stdout.readline()
> 
> This assumes password-less sudo access or that sudo was invoked within the password prompt timeout period. Otherwise, we will hang waiting for a password. This is not the correct place for such port-specific code. It should be in the port-specific Port object. Use Port.sample_process() as an example.

That's what the '-A' does.  If password-less sudo is not enabled (and it is enabled on the automation machines) this will simply continue execution without calling spindump (and will output 'sudo: no askpass program specified, try setting SUDO_ASKPASS'), which should be fine since this is designed to catch process hangs on automation machines.

-- 
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/20160719/b9b2ba3c/attachment.html>