Bug 66806

Summary: [chromium] Stacktrace not in test output when a test crashes
Product: WebKit Reporter: James Robinson <jamesr>
Component: Tools / TestsAssignee: James Robinson <jamesr>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, dpranke, eric, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

James Robinson
Reported 2011-08-23 13:41:21 PDT
When a test crashes in the chromium port, the full stacktrace is produced by the binary but isn't actually visible it the output. It looks like the output is ending up in DriverOutput.text, but when a test crashes we only output DriverOutput.error. http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py#L69. I think this is due to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/chromium.py#L448 which pipes stderr to the same place as stdout. If locally I patch http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py#L69 to write "driver_output.error + driver_output.text", then I do see the full stack (in addition to whatever test output was produced on stdout). Based on the comments we should be using ServerProcess, which does have logic to open up stdout and stderr FDs and select() between them. Because of this bug we often don't get usable stack traces from even the debug layout test bots, which sucks.
Attachments
Patch (1.76 KB, patch)
2011-08-24 15:59 PDT, James Robinson 		no flags
DetailsFormatted DiffDiff
Patch (1.78 KB, patch)
2011-08-24 16:20 PDT, James Robinson 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
James Robinson
Comment 1 2011-08-23 13:45:33 PDT
At a glance maybe I could change the Popen call to leave stderr in its own PIPE, modify ChromiumDriver._write_command_and_read_line() to return a tuple (line, error, did_crash) and do a select() on stdout/stderr inside that function instead of reading directly from stdout always. Sound reasonable?
Eric Seidel (no email)
Comment 2 2011-08-23 14:26:05 PDT
Our stdout vs. stderr handling needs work in both the WebKit and Chromium Driver implementations. I do have plans to fix WebKit's/ServerProcess, but I don't have any plans to fix ChromiumDriver since Chromium's DRT is so different from the rest (and I don't want to break it). It seems we should be writing out to a stderr file liek the rest of the webkit ports do, no?
James Robinson
Comment 3 2011-08-23 14:29:35 PDT
Why would we write the stderr output into a file instead of reading it directly from the test harness?
James Robinson
Comment 4 2011-08-23 15:06:00 PDT
https://bugs.webkit.org/show_bug.cgi?id=58708 seems related.
Dirk Pranke
Comment 5 2011-08-24 15:52:41 PDT
The problem with your solution in comment #1 is that select doesn't work on windows. You end up needing to do something like what I outlined in bug bug 58708, or looping between the two fd's after setting them both to be nonblocking (assuming that even works, I haven't tested it on all platforms, but it probably does). It would be nice if we could unify around ServerProcess, but as Eric says, the logic is at least somewhat different so it's a non-zero amount of work. We could consider also making Chromium DRT match the other ports' output; Tony & co got this mostly working but we never finished working out the bugs. In addition to letting us delete code, it would also probably provide a decent speed boost since we wouldn't have to write the PNGs to disk.
James Robinson
Comment 6 2011-08-24 15:56:08 PDT
Can you point me at where that work went to unify the DRT output? I think that would be a big win, since currently when we get a crash on the bots we have to reproduce locally just to get a stack. If we could just get a stack from the bots it would help tremendously.
James Robinson
Comment 7 2011-08-24 15:59:06 PDT
Created attachment 105091 [details] Patch
Dirk Pranke
Comment 8 2011-08-24 16:00:47 PDT
Comment on attachment 105091 [details] Patch change the run_test() method in ChromiumPort to return output + error if it's a crash instead. That way the hack is isolated to Chromium ports.
Tony Chang
Comment 9 2011-08-24 16:19:43 PDT
(In reply to comment #6) > Can you point me at where that work went to unify the DRT output? Chromium's DRT can do either output. If you pass in --test-shell, we get the Chromium style output, otherwise, we're supposed to output DRT style. Most of this logic seems encapsulated in TestEventPrinter. The non-test-shell output may be out of date. http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/chromium/TestEventPrinter.cpp When we were upstreaming chromium's DRT, we tried to switch to the common DRT format at the same time, but there were bugs that we failed to track down (random test timeouts) so we cut our losses and just used the old test_shell format. I'm not sure exactly what you'd need to do to try to use the common DRT format. You might be able to just change ChromiumMacPort to inherit from WebKitPort instead of ChromiumPort, but there are probably some additional changes needed.
James Robinson
Comment 10 2011-08-24 16:20:44 PDT
Created attachment 105092 [details] Patch
Dirk Pranke
Comment 11 2011-08-24 16:22:14 PDT
Note that if we were to unify the output, we'd also need to make sure that ServerProcess works on windows (win32, not just cygwin). I have no idea what the status of the NRWT-on-win port is. Eric?
WebKit Review Bot
Comment 12 2011-08-24 18:02:15 PDT
Comment on attachment 105092 [details] Patch Clearing flags on attachment: 105092 Committed r93751: <http://trac.webkit.org/changeset/93751>
WebKit Review Bot
Comment 13 2011-08-24 18:02:20 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.