Bug 225818 – [WebKitTestRunner] Forward WebContent termination reason

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

[Jonathan Bedard]

We should print the reason a WebContent process was terminated to aid in debugging.

[Radar WebKit Bug Importer]

<rdar://problem/78027164>

[Jonathan Bedard]

Created attachment 428645 [details]
Patch

[Chris Dumez]

Comment on attachment 428645 [details]
Patch

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

> Tools/WebKitTestRunner/TestController.cpp:2163
> +        fprintf(stderr, "Terminating %s (pid %ld) ", webProcessName(), static_cast<long>(pid));

Isn't this going to look confusing?
"""
Terminating WebProcess (pid: 123) because the process crashed
#CRASHED - WebProcess (pid: 123)
"""

Why are we saying we are terminating a process when the process actually crashed?

Maybe it should be something like:
fprintf(stderr, "%s terminated (pid %ld) ", webProcessName(), static_cast<long>(pid));

> Tools/WebKitTestRunner/TestController.cpp:2182
> +            return;

Note that you are not calling fflush() in this early return case even though you printed something.

> Tools/WebKitTestRunner/TestController.cpp:2185
>          fflush(stderr);

Note the call to fflush() here.

[Jonathan Bedard]

Comment on attachment 428645 [details]
Patch

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

>> Tools/WebKitTestRunner/TestController.cpp:2163
>> +        fprintf(stderr, "Terminating %s (pid %ld) ", webProcessName(), static_cast<long>(pid));
> 
> Isn't this going to look confusing?
> """
> Terminating WebProcess (pid: 123) because the process crashed
> #CRASHED - WebProcess (pid: 123)
> """
> 
> Why are we saying we are terminating a process when the process actually crashed?
> 
> Maybe it should be something like:
> fprintf(stderr, "%s terminated (pid %ld) ", webProcessName(), static_cast<long>(pid));

The #CRASHED string will not show up in run-webkit-test's output, that's a control statement that triggers some logic in webkitpy about collecting crashes. I was trying to match the name of the API

I'm good with your phrasing, though, indicating the termination has already occurred.

>> Tools/WebKitTestRunner/TestController.cpp:2182
>> +            return;
> 
> Note that you are not calling fflush() in this early return case even though you printed something.

My rationale here is that the processDidCrash callback was not triggered in the case originally, and the whole reason we call flush is because we may call exit at the end of this function. That being said, I suppose flush can't do any harm in the early return case.

[Jonathan Bedard]

Created attachment 428654 [details]
Patch

[Chris Dumez]

Comment on attachment 428654 [details]
Patch

r=me

[Jonathan Bedard]

Created attachment 428687 [details]
Patch

[EWS]

Committed r277533 (237761@main): <https://commits.webkit.org/237761@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428687 [details].