Bug 63683

Created attachment 99226 [details] Patch

Created attachment 99234 [details] works, but is inelegant

Comment on attachment 99234 [details] works, but is inelegant View in context: https://bugs.webkit.org/attachment.cgi?id=99234&action=review > Tools/Scripts/webkitpy/common/net/layouttestresults.py:93 > - return [test_failures.FailureCrash()] > + return [test_failures.FailureCrash(process_name="WebProcess")] You should create a new failure type rather than adding state to this one. > Tools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py:107 > - def should_kill_dump_render_tree(self): > - """Returns True if we should kill DumpRenderTree before the next test.""" > + def driver_needs_restart(self): > + """Returns True if we should kill DumpRenderTree/WebKitTestRunner before the next test.""" This should be done in a separate patch. > Tools/Scripts/webkitpy/layout_tests/layout_package/test_failures_unittest.py:-65 > - def test_crash(self): > - FailureCrash() > - > - def test_hash_incorrect(self): > - FailureImageHashIncorrect() > - > - def test_missing(self): > - FailureMissingResult() > - > - def test_missing_image(self): > - FailureMissingImage() > - > - def test_missing_image_hash(self): > - FailureMissingImageHash() > - > - def test_timeout(self): > - FailureTimeout() > - This should be done in a separate patch. > Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:72 > - if failure.is_reftest: > - writer.write_crash_report(expected_driver_output.error) > - else: > - writer.write_crash_report(driver_output.error) > + writer.write_crash_report(driver_output) It's unclear whether this part is correct. > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:492 > + def _check_for_driver_crash(self): You should make these changes separately from all the rest of the plumbing so you can make sure the harness is robust to all these different cases. Plumbing the information out is a separate problem. > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:497 > + if re.search(r"#CRASHED\n", self._server_process.error, re.MULTILINE): Obviously we don't want to use regular expressions. If we need to do a search, we can just do a substring match since the pattern is trivial.

Comment on attachment 99234 [details] works, but is inelegant View in context: https://bugs.webkit.org/attachment.cgi?id=99234&action=review >> Tools/Scripts/webkitpy/common/net/layouttestresults.py:93 >> + return [test_failures.FailureCrash(process_name="WebProcess")] > > You should create a new failure type rather than adding state to this one. OK. We'll end up needing 3 failures types then. The existing one for DRT, one for WebTestRunner and one for WebProcess. >> Tools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py:107 >> + """Returns True if we should kill DumpRenderTree/WebKitTestRunner before the next test.""" > > This should be done in a separate patch. Will do. >> Tools/Scripts/webkitpy/layout_tests/layout_package/test_failures_unittest.py:-65 >> - > > This should be done in a separate patch. Done: https://bugs.webkit.org/show_bug.cgi?id=63742 >> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:72 >> + writer.write_crash_report(driver_output) > > It's unclear whether this part is correct. I guess I don't understand why crashing-ref-tests make any sense. Given the other broken ref_test code I've seen, it seems safer to remove the code rather than try and intuit what it's supposed to do. >> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:497 >> + if re.search(r"#CRASHED\n", self._server_process.error, re.MULTILINE): > > Obviously we don't want to use regular expressions. If we need to do a search, we can just do a substring match since the pattern is trivial. Agreed, this is the inelegant part of the patch. Things get confusing because DRT expects you to read stderr one line at a time, and we don't. We process it all in big gobs.

Comment on attachment 99234 [details] works, but is inelegant View in context: https://bugs.webkit.org/attachment.cgi?id=99234&action=review >>> Tools/Scripts/webkitpy/common/net/layouttestresults.py:93 >>> + return [test_failures.FailureCrash(process_name="WebProcess")] >> >> You should create a new failure type rather than adding state to this one. > > OK. We'll end up needing 3 failures types then. The existing one for DRT, one for WebTestRunner and one for WebProcess. Do we really need three? Is there utility in distinguishing a WebTestRunner crash from a WebProcess crash? > Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:172 > + # FIXME: This is broken. I expect this meant to read "hasattr('is_reftest', failure) and failure.is_reftest". Wouldn't it have been simpler to just change the line of code, rather than adding the fixme? I understand that it's a semi-unrelated change, but there were lots of less-related changes in this patch, and what are the odds that you're going to remember to come back and fix this? >>> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:72 >>> + writer.write_crash_report(driver_output) >> >> It's unclear whether this part is correct. > > I guess I don't understand why crashing-ref-tests make any sense. Given the other broken ref_test code I've seen, it seems safer to remove the code rather than try and intuit what it's supposed to do. I'm not sure what you mean by "make any sense". It's plausible that the driver will crash running the reference just as easily on the test. It seems like the question is, what to do if the reference fails or times out? It seems like there's two options. One is to ensure that we always run the reference *as a test*, so that it would be reported separately. The other would be to roll the reference failure into the test failure. I think the latter path makes more sense (and is what we're doing here), but the crash report probably needs to indicate that this was from the reference and not the test. (cf. below). Given that, I would either leave the code as is, or add the parameter. > Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:168 > + def write_crash_report(self, driver_output): Add an is_reftest param here? (see above)

(In reply to comment #5) > (From update of attachment 99234 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=99234&action=review > > >>> Tools/Scripts/webkitpy/common/net/layouttestresults.py:93 > >>> + return [test_failures.FailureCrash(process_name="WebProcess")] > >> > >> You should create a new failure type rather than adding state to this one. > > > > OK. We'll end up needing 3 failures types then. The existing one for DRT, one for WebTestRunner and one for WebProcess. > > Do we really need three? Is there utility in distinguishing a WebTestRunner crash from a WebProcess crash? I believe so. Changes what process we sample.

Created attachment 111684 [details] Patch

Comment on attachment 111684 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111684&action=review > Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:66 > - if failure.is_reftest: > - writer.write_crash_report(expected_driver_output.error) > - else: > - writer.write_crash_report(driver_output.error) > + writer.write_crash_report(driver_output) This seems unrelated.

Created attachment 111688 [details] Remove unrelated chnage

Comment on attachment 111688 [details] Remove unrelated chnage View in context: https://bugs.webkit.org/attachment.cgi?id=111688&action=review > Tools/Scripts/webkitpy/common/net/layouttestresults.py:93 > - return [test_failures.FailureCrash()] > + return [test_failures.FailureCrash(process_name="WebProcess")] This isn't really right, but the right way to fix this is to use the JSON representation throughout the system. > Tools/Scripts/webkitpy/layout_tests/port/driver.py:44 > + def __init__(self, text, image, image_hash, audio, > + crash=False, test_time=0, timeout=False, error='', > + crashed_process_name=None): This intent is confusing. > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:477 > + elif error_line == "#CRASHED - WebProcess\n": What about the plugin process?

(In reply to comment #10) > (From update of attachment 111688 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=111688&action=review > > > Tools/Scripts/webkitpy/common/net/layouttestresults.py:93 > > - return [test_failures.FailureCrash()] > > + return [test_failures.FailureCrash(process_name="WebProcess")] > > This isn't really right, but the right way to fix this is to use the JSON representation throughout the system. Yeah. :( > > Tools/Scripts/webkitpy/layout_tests/port/driver.py:44 > > + def __init__(self, text, image, image_hash, audio, > > + crash=False, test_time=0, timeout=False, error='', > > + crashed_process_name=None): > > This intent is confusing. Fixed. > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:477 > > + elif error_line == "#CRASHED - WebProcess\n": > > What about the plugin process? looks like WebKitTestRunner doesn't know how to tell us when the plugin process crashed yet.

Sam, Anders: do you know if PluginProcess crashes show up as "WebProcess" crashes from WebKitTestRunner? It would be very easy to add PluginProcess crash reporting to NRWT, if WKTR supported detecting it.

Committed r97931: <http://trac.webkit.org/changeset/97931>