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.
Created attachment 144489 [details] first patch Dirk, any suggestions for improvements?
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 ...
Created attachment 144924 [details] reworked patch
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?
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?
(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?
Committed http://trac.webkit.org/projects/webkit/changeset/119017