Bug 63683 - Teach new-run-webkit-tests about #CRASHED and #CRASHED - WebProcess
: Teach new-run-webkit-tests about #CRASHED and #CRASHED - WebProcess
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Eric Seidel
:
Depends on: 63743 70435
Blocks: nrwt-wk2
  Show dependency treegraph
 
Reported: 2011-06-29 20:37 PDT by Eric Seidel
Modified: 2011-10-19 21:21 PDT (History)
5 users (show)

See Also:


Attachments
Patch (21.67 KB, patch)
2011-06-29 20:38 PDT, Eric Seidel
no flags Details | Formatted Diff | Diff
works, but is inelegant (24.86 KB, patch)
2011-06-29 21:42 PDT, Eric Seidel
no flags Details | Formatted Diff | Diff
Patch (12.52 KB, patch)
2011-10-19 15:42 PDT, Eric Seidel
no flags Details | Formatted Diff | Diff
Remove unrelated chnage (10.00 KB, patch)
2011-10-19 16:05 PDT, Eric Seidel
abarth: review+
eric: commit‑queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel 2011-06-29 20:37:52 PDT
Teach new-run-webkit-tests about #CRASHED and #CRASHED - WebProcess
Comment 1 Eric Seidel 2011-06-29 20:38:22 PDT
Created attachment 99226 [details]
Patch
Comment 2 Eric Seidel 2011-06-29 21:42:34 PDT
Created attachment 99234 [details]
works, but is inelegant
Comment 3 Adam Barth 2011-06-29 22:14:45 PDT
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 4 Eric Seidel 2011-06-30 11:49:39 PDT
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 5 Dirk Pranke 2011-06-30 12:03:59 PDT
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)
Comment 6 Eric Seidel 2011-10-19 14:40:44 PDT
(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.
Comment 7 Eric Seidel 2011-10-19 15:42:32 PDT
Created attachment 111684 [details]
Patch
Comment 8 Adam Barth 2011-10-19 15:48:48 PDT
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.
Comment 9 Eric Seidel 2011-10-19 16:05:30 PDT
Created attachment 111688 [details]
Remove unrelated chnage
Comment 10 Adam Barth 2011-10-19 21:13:34 PDT
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?
Comment 11 Eric Seidel 2011-10-19 21:15:20 PDT
(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.
Comment 12 Eric Seidel 2011-10-19 21:20:09 PDT
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.
Comment 13 Eric Seidel 2011-10-19 21:21:33 PDT
Committed r97931: <http://trac.webkit.org/changeset/97931>