Bug 81575 – WTR - log the pid of a crashing WebProcess

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

WTR - log the pid of a crashing WebProcess

[Dirk Pranke]

Created attachment 132692 [details]
Patch

[Dirk Pranke]

n order to make sure we get the right crash reports when running run-webkit-tests in parallel, we need WTR to log the pid of the WebProcess that crashes when it crashes (see bug 71380 for context).

This patch is almost certainly not the right way to fix this, but I thought I'd post it to get feedback on what the right way is ...  presumably I need to add a forwarding header for WKPagePrivateMac.h somewhere? Do we want to make this a generic function, since presumably we need the same functionality on the other ports?

[Alexey Proskuryakov]

Comment on attachment 132692 [details]
Patch

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

> This patch is almost certainly not the right way to fix this

Is your concern only about the style of include? It's wrong indeed, suggestion below. I don't see anything else that would stand out as wrong here.

> Do we want to make this a generic function, since presumably we need the same functionality on the other ports?

It's best to factor out a universal solution when there is practical need for it. Otherwise, we risk creating a wrong design, or just over-engineering things.

> Tools/WebKitTestRunner/TestController.cpp:43
> +#include "../../Source/WebKit2/UIProcess/API/C/mac/WKPagePrivateMac.h"

I think that this should work:

#include <WebKit2/WKPagePrivateMac.h>

> Tools/WebKitTestRunner/TestController.cpp:497
> -    if (!resetStateToConsistentValues()) {
> +     if (!resetStateToConsistentValues()) {

Wrong indentation.

> Tools/WebKitTestRunner/TestController.cpp:499
> +        int pid = WKPageGetProcessIdentifier(m_mainWebView->page());

This function returns pid_t, which is a platform dependent type. I'd use this type for the variable, and cast to long inside printf.

> Tools/WebKitTestRunner/TestController.cpp:814
> +        int pid = WKPageGetProcessIdentifier(m_mainWebView->page());

Same comment about return type.

[Dirk Pranke]

(In reply to comment #3)
> (From update of attachment 132692 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132692&action=review
> 
> > This patch is almost certainly not the right way to fix this
> 
> Is your concern only about the style of include? It's wrong indeed, suggestion below. I don't see anything else that would stand out as wrong here.
> 

Mostly I was thinking that getting the subprocess pid is likely to be needed by every WTR implementation and so maybe it should in a generic header and not a platform-specific header.

I'm happy to land this as-is and change it down the road as other WK2 ports want the same functionality, though.
 
> > Do we want to make this a generic function, since presumably we need the same functionality on the other ports?
> 
> It's best to factor out a universal solution when there is practical need for it. Otherwise, we risk creating a wrong design, or just over-engineering things.
>

Sure.
 
> > Tools/WebKitTestRunner/TestController.cpp:43
> > +#include "../../Source/WebKit2/UIProcess/API/C/mac/WKPagePrivateMac.h"
> 
> I think that this should work:
> 
> #include <WebKit2/WKPagePrivateMac.h>
>

Will give it a try.
 
> > Tools/WebKitTestRunner/TestController.cpp:497
> > -    if (!resetStateToConsistentValues()) {
> > +     if (!resetStateToConsistentValues()) {
> 
> Wrong indentation.
>

Will fix.
 
> > Tools/WebKitTestRunner/TestController.cpp:499
> > +        int pid = WKPageGetProcessIdentifier(m_mainWebView->page());
> 
> This function returns pid_t, which is a platform dependent type. I'd use this type for the variable, and cast to long inside printf.
>

Will fix. Not sure how I missed that :(.
 
Thanks for the review!

[Dirk Pranke]

Created attachment 132926 [details]
patch for landing

[Alexey Proskuryakov]

Comment on attachment 132926 [details]
patch for landing

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

> Tools/WebKitTestRunner/TestController.cpp:39
> +#include <signal.h>

Why do you need signal.h?

[Dirk Pranke]

Comment on attachment 132926 [details]
patch for landing

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

>> Tools/WebKitTestRunner/TestController.cpp:39
>> +#include <signal.h>
> 
> Why do you need signal.h?

Whoops. I don't; I thought I had removed that :(

[Dirk Pranke]

Committed r111619: <http://trac.webkit.org/changeset/111619>