Bug 81575 - WTR - log the pid of a crashing WebProcess
Summary: WTR - log the pid of a crashing WebProcess
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks: 71380
  Show dependency treegraph
 
Reported: 2012-03-19 15:51 PDT by Dirk Pranke
Modified: 2012-03-21 16:29 PDT (History)
8 users (show)

See Also:


Attachments
Patch (4.82 KB, patch)
2012-03-19 15:51 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
patch for landing (4.83 KB, patch)
2012-03-20 16:45 PDT, Dirk Pranke
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2012-03-19 15:51:06 PDT
WTR - log the pid of a crashing WebProcess
Comment 1 Dirk Pranke 2012-03-19 15:51:33 PDT
Created attachment 132692 [details]
Patch
Comment 2 Dirk Pranke 2012-03-19 15:54:40 PDT
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?
Comment 3 Alexey Proskuryakov 2012-03-19 20:41:52 PDT
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.
Comment 4 Dirk Pranke 2012-03-20 13:11:18 PDT
(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!
Comment 5 Dirk Pranke 2012-03-20 16:45:47 PDT
Created attachment 132926 [details]
patch for landing
Comment 6 Alexey Proskuryakov 2012-03-20 16:54:52 PDT
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?
Comment 7 Dirk Pranke 2012-03-20 17:07:05 PDT
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 :(
Comment 8 Dirk Pranke 2012-03-21 16:29:42 PDT
Committed r111619: <http://trac.webkit.org/changeset/111619>