[webkit-reviews] review granted: [Bug 193772] run-webkit-tests should check for leaks in WebKit processes : [Attachment 365868] Patch v4

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Mar 26 16:16:18 PDT 2019


Ryosuke Niwa <rniwa at webkit.org> has granted David Kilzer (:ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 193772: run-webkit-tests should check for leaks in WebKit processes
https://bugs.webkit.org/show_bug.cgi?id=193772

Attachment 365868: Patch v4

https://bugs.webkit.org/attachment.cgi?id=365868&action=review




--- Comment #18 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 365868
  --> https://bugs.webkit.org/attachment.cgi?id=365868
Patch v4

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

> Tools/Scripts/webkitpy/port/driver.py:262
> +	       block = self._read_block(deadline, '', wait_for_stderr_eof=True)
> +	      
self._server_process.set_child_processes(self._parse_child_processes_output(blo
ck.decoded_content))

Driver asking ServerProcess to list child processes then storing child
processes is a very strange architecture.
I think one alternative is to move the leak detection code in
ServerProcess.stop to Driver
then just store the child processes in Driver directly but that's not
necessarily better
if wanted to detect the leaks throughout the testing.

Perhaps this code belongs to ServerProcess, which lazily issues #LIST CHILD
PROCESSES command.
It's a weird layering either way though.

> Tools/Scripts/webkitpy/port/driver.py:281
> +	       m = re.match('^([^:]+): ([0-9]+)$', line)

Please use named groups as in: r'^(?P<name>[^:]+): (?P<pid>[0-9]+)$'
Also not the leading "r" to escape this string as a regular expression.

> Tools/WebKitTestRunner/TestController.cpp:1053
> +    pid_t webContentPID =
WKPageGetProcessIdentifier(TestController::singleton().mainWebView()->page());

Why don't we just add some SPI to WKContext to get the list of PIDs from
WebProcessPool directly?
That would probably catch all leaks because we cache up to 30 processes after
each use.


More information about the webkit-reviews mailing list