Bug 66806 - [chromium] Stacktrace not in test output when a test crashes
Summary: [chromium] Stacktrace not in test output when a test crashes
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
Depends on:
Reported: 2011-08-23 13:41 PDT by James Robinson
Modified: 2011-08-24 18:02 PDT (History)
6 users (show)

See Also:

Patch (1.76 KB, patch)
2011-08-24 15:59 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (1.78 KB, patch)
2011-08-24 16:20 PDT, James Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 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.
Comment 1 James Robinson 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?
Comment 2 Eric Seidel (no email) 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?
Comment 3 James Robinson 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?
Comment 4 James Robinson 2011-08-23 15:06:00 PDT
https://bugs.webkit.org/show_bug.cgi?id=58708 seems related.
Comment 5 Dirk Pranke 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.
Comment 6 James Robinson 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.
Comment 7 James Robinson 2011-08-24 15:59:06 PDT
Created attachment 105091 [details]
Comment 8 Dirk Pranke 2011-08-24 16:00:47 PDT
Comment on attachment 105091 [details]

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.
Comment 9 Tony Chang 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.

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.
Comment 10 James Robinson 2011-08-24 16:20:44 PDT
Created attachment 105092 [details]
Comment 11 Dirk Pranke 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?
Comment 12 WebKit Review Bot 2011-08-24 18:02:15 PDT
Comment on attachment 105092 [details]

Clearing flags on attachment: 105092

Committed r93751: <http://trac.webkit.org/changeset/93751>
Comment 13 WebKit Review Bot 2011-08-24 18:02:20 PDT
All reviewed patches have been landed.  Closing bug.