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
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 63743 70435
: 56729
  Show dependency treegraph
 
Reported: 2011-06-29 20:37 PST by
Modified: 2011-10-19 21:21 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-06-29 20:37:52 PST
Teach new-run-webkit-tests about #CRASHED and #CRASHED - WebProcess
------- Comment #1 From 2011-06-29 20:38:22 PST -------
Created an attachment (id=99226) [details]
Patch
------- Comment #2 From 2011-06-29 21:42:34 PST -------
Created an attachment (id=99234) [details]
works, but is inelegant
------- Comment #3 From 2011-06-29 22:14:45 PST -------
(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()]
> +            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 From 2011-06-30 11:49:39 PST -------
(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.

>> 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 From 2011-06-30 12:03:59 PST -------
(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?

> 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 From 2011-10-19 14:40:44 PST -------
(In reply to comment #5)
> (From update of attachment 99234 [details] [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 From 2011-10-19 15:42:32 PST -------
Created an attachment (id=111684) [details]
Patch
------- Comment #8 From 2011-10-19 15:48:48 PST -------
(From update of attachment 111684 [details])
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 From 2011-10-19 16:05:30 PST -------
Created an attachment (id=111688) [details]
Remove unrelated chnage
------- Comment #10 From 2011-10-19 21:13:34 PST -------
(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.

> 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 From 2011-10-19 21:15:20 PST -------
(In reply to comment #10)
> (From update of attachment 111688 [details] [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 From 2011-10-19 21:20:09 PST -------
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 From 2011-10-19 21:21:33 PST -------
Committed r97931: <http://trac.webkit.org/changeset/97931>