Bug 87714 – Mac crash logs can take a really long time to be written out.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Stephanie Lewis]

If lots of tests are crashing or the system is running low on memory  ReportCrash can take 15+ minutes to write a crash log to disk.  The tests can't wait that long so currently report 'no crash log found'.

We can run a second check after the tests have finished for any new logs that have been written out.  By this point the workers have finished which should have freed up enough memory and cpu for ReportCrash to finish.

[Stephanie Lewis]

Created attachment 144489 [details]
first patch

Dirk, any suggestions for improvements?

[Dirk Pranke]

Comment on attachment 144489 [details]
first patch

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

Thanks for working on this! The patch looks generally sound but I think some of the logic needs to be in different places to avoid adding layering dependencies that shouldn't be there. Let me know if my comments below don't make sense.

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:41
> +from webkitpy.layout_tests.models.result_summary import ResultSummary

The Port classes shouldn't have all of these dependencies on test_expectations, TestResultWriter, ResultSummary, etc.

I think it might make more sense to move the logic in look_for_new_crash_logs that actually gets the list of <process_name, pid> pairs to look for into manager.py, and have the Port routine just take a list of tuples to check for. Actually building the list is generic and should be fast, so there's little harm in running it all the time for every port.

Then the look_for_new_crash_logs() call can return a list or a dict of found crash logs, and let the manager instantiate the TestResultWriter to actually write the files (since, again, this should be generic logic).

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:224
> +                crash_log = self._get_crash_log(failure.process_name, failure.pid, None, None, start_time)

I think it might make sense to modify _get_crash_log() so that it doesn't do the slow-spin on the second pass (since there's probably not much point in waiting any longer)?

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:227
> +                if not crash_log.startswith("no"):

It might also be better to modify _get_crash_log() to return None and let the caller come up with a default (here, you don't do anything, in WebKitDriver.run_test() return the "no crash log found" string ...

[Stephanie Lewis]

Created attachment 144924 [details]
reworked patch

[Dirk Pranke]

Comment on attachment 144924 [details]
reworked patch

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

Looks good. thanks!

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:204
> +            _log.warning('no crash log found for %s:%d' % (self._crashed_process_name, self._crashed_pid))

This line should be deleted, right?

[Ojan Vafai]

Comment on attachment 144924 [details]
reworked patch

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

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:207
> +    def look_for_new_crash_logs(self, crashed_processes, start_time):

Should we be adding something like this to the chromium_mac port as well?

[Dirk Pranke]

(In reply to comment #5)
> (From update of attachment 144924 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144924&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/mac.py:207
> > +    def look_for_new_crash_logs(self, crashed_processes, start_time):
> 
> Should we be adding something like this to the chromium_mac port as well?

Well, we get stack traces inline through breakpad, right, so I don't think crashreporter adds much?

[Stephanie Lewis]

Committed http://trac.webkit.org/projects/webkit/changeset/119017