Bug 203816 – WebKitTestRunner: Optionally print helper process PIDs

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

[Jonathan Bedard]

If we have a bug where the UI process is abandoning it's helper processes, it is difficult to investigate because we can't figure out what the PIDs of those processes are without a debugger.

[Jonathan Bedard]

<rdar://problem/56798411>

[Jonathan Bedard]

Created attachment 382748 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 382748 [details]
Patch

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

> Tools/ChangeLog:3
> +        WebKitTestRunner: Optionally print helper process PIDs

I thought that we still didn’t have a proven use case for this yet? Exposing too much detail about WebKit internal behavior is not necessarily good long term.

[Jonathan Bedard]

(In reply to Alexey Proskuryakov from comment #3)
> Comment on attachment 382748 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382748&action=review
> 
> > Tools/ChangeLog:3
> > +        WebKitTestRunner: Optionally print helper process PIDs
> 
> I thought that we still didn’t have a proven use case for this yet? Exposing
> too much detail about WebKit internal behavior is not necessarily good long
> term.

We're hiding these behind the --verbose flag, this isn't the default behavior.

I'd don't disagree with the sentiment about exposing details about WebKit's internal behavior, but this is the 3rd time in the last 2 years we've had a bug involving orphaned processes in WebKitTestRunner. It's also a bit tough to determine exactly which WebContent process is associated with a specific instance on WebKitTestRunner, especially since opening other apps which render web content often results in a half-dozen different WebKit processes.

[Alexey Proskuryakov]

I may be missing some context here. What were those bugs?

[Jonathan Bedard]

Here is the list of bugs I've been involved with where this sort of thing would have helped:
    ​https://bugs.webkit.org/show_bug.cgi?id=178902
    https://bugs.webkit.org/show_bug.cgi?id=179123
    https://bugs.webkit.org/show_bug.cgi?id=194829

We also have the reason this was requested, which is another variation on a process leak someone found while fuzzing the test runner.

[Alexey Proskuryakov]

The request in <rdar://problem/56798411> claims that this is needed because WebKit processes keep running after WebKitTestRunner is killed. This seems implausible, and we shouldn't be writing code to deal with that until there is some evidence.

The examples you gave are different, it's about when WebKitTestRunner is still running. Is there a task for adopt the functionality that you are adding here to be used in those cases? We don't add features just because they could be useful in indeterminate future. I'm not actually sure how this logging would have helped there, are you thinking of some automated analysis, or just extra info for manual investigation?

[Jonathan Bedard]

(In reply to Alexey Proskuryakov from comment #7)
> The request in <rdar://problem/56798411> claims that this is needed because
> WebKit processes keep running after WebKitTestRunner is killed. This seems
> implausible, and we shouldn't be writing code to deal with that until there
> is some evidence.
> 
> The examples you gave are different, it's about when WebKitTestRunner is
> still running. Is there a task for adopt the functionality that you are
> adding here to be used in those cases? We don't add features just because
> they could be useful in indeterminate future. I'm not actually sure how this
> logging would have helped there, are you thinking of some automated
> analysis, or just extra info for manual investigation?

Extra info for manual investigation hidden behind the --verbose flag (which we have, but aren't using for anything at the moment)

Normally, I would be fine just adding this code in any time we encountered a problem like this for local testing, but the problem with that approach is that it makes bisection expensive. I thought we had fixed all of these issues, but since someone is reporting something like those original process leak problems and using the same technique I used to investigate the previous 3 bugs, I think we might have crossed the point that it's worth landing something like this.

I can't say I feel very strongly about it, your objections here are basically the reasons I didn't post a patch for this originally. But, if we're going to have a --verbose flag in WebKitTestRunner, this seems like the sort of thing that flag would enable.

[Alexey Proskuryakov]

Comment on attachment 382748 [details]
Patch

I meant to mark this as r- long ago. I really don't like exposing these details of WebKit behavior, and making anything depend on it. The data is obviously already incorrect in case of any process swapping occurring.

[Jonathan Bedard]

Seems like there is not interest in implementing something like this, so I'm going to close this bug.