Bug 55907 - new-run-webkit-tests should upload crash logs
Summary: new-run-webkit-tests should upload crash logs
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Adam Barth
Depends on: 63468
Blocks: 34984
  Show dependency treegraph
Reported: 2011-03-07 14:45 PST by Dirk Pranke
Modified: 2011-06-28 08:19 PDT (History)
5 users (show)

See Also:

Patch (6.41 KB, patch)
2011-06-27 14:59 PDT, Adam Barth
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2011-03-07 14:45:24 PST
old-run-webkit-tests now uploads crash logs; we need to modify new-run-webkit-tests to do the same.

Quoting from an email to aroben@:

On Mar 2, 2011, at 3:44 PM, Dirk Pranke wrote:

> What did you need to change to make this happen? What do I need to add
> to new-run-webkit-tests?

There (thankfully) isn't very much code in old-run-webkit-tests to support this. The functions captureSavedCrashLogs and setUpWindowsCrashLogSaving and their callers/callees, plus the END block, are all that is required.
Comment 1 Eric Seidel (no email) 2011-03-07 15:48:46 PST
If we wrote the code out nicely in webkitpy, we could easily expose commands (as part of webkit-patch or a separate tool) which ORWT could call, so that we could share a single python implementation instead of having two copies. :)
Comment 2 Adam Barth 2011-06-27 14:59:01 PDT
Created attachment 98793 [details]
Comment 3 Dirk Pranke 2011-06-27 15:07:46 PDT
Comment on attachment 98793 [details]

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

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:181
> +        fs.write_text_file(filename, log if log else error)

I just looked at the crashlogs.py module for the first time; does this actually work if you are running multiple DRTs at once? How do you know which pid generated the log?

It looks like there is a timestamp and maybe some other number in the crash log filename; is that naming convention configurable, or is there some other way to get a pid into the filename?
Comment 4 Eric Seidel (no email) 2011-06-27 15:15:25 PDT
The pid is in the file.  We could parse it out if necessary.  I'm not sure we need to associate pid->crashlog yet though.
Comment 5 Dirk Pranke 2011-06-27 15:18:31 PDT
(In reply to comment #4)
> The pid is in the file.  We could parse it out if necessary.  I'm not sure we need to associate pid->crashlog yet though.

Well, since there are multiple workers running in parallel, the risk is that you'll end up associating stacks with the wrong tests. I've certainly seen cases where we've had lots (or all) tests crashing at once, but maybe the break is so obvious that you only really need to see one stack to fix the problem?

If the pid is in the crash file, it doesn't seem like it would be that much harder to pull it out ...
Comment 6 Eric Seidel (no email) 2011-06-27 15:21:56 PDT
Comment on attachment 98793 [details]

LGTM.  I suspect we'll iterate from here.
Comment 7 Adam Barth 2011-06-27 15:23:18 PDT
There's a slight problem in that ReportCrash takes a very long time to run and the master gets bored and kills it.  :(
Comment 8 Adam Barth 2011-06-27 16:05:42 PDT
Committed r89873: <http://trac.webkit.org/changeset/89873>
Comment 9 Adam Barth 2011-06-27 16:27:00 PDT
Comment on attachment 98793 [details]

I'm going to leave this bug closed, but there's going to be some follow up work because ReportCrash is super, duper slow and tricks us into think the test timed out.
Comment 10 Adam Roben (:aroben) 2011-06-28 08:19:53 PDT
Have other bugs been filed to cover the remaining work? (Waiting appropriately for ReportCrash, dealing with multiple concurrent crashes, making this work on Windows…) It seems wrong to close this bug if not.