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

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has canceled  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 #23 from David Kilzer (:ddkilzer) <ddkilzer 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
>>>>> + 	  
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.
>>> 
>>> We can't have WebKitTestRunner specific logic in the ServerProcess, the
ServerProcess isn't specific to WebKitTestRunner. For example, we use the
ServerProcess to run both API tests and ImageDiff.
>> 
>> The idea of child processes don't need to be specific to WebKitTestRunner.
ServerProcess can have a generic idea of child processes, or a list of PIDs for
which post-test tasks need to be performed.
>> 
>> In fact, one of the things I don't like about the current patch is that LIST
CHILD PROCESSES isn't added to DumpRenderTree. We absolutely should and return
an empty list.
> 
> The idea of child processes isn't specific to WebKitTestRunner, but the
'#LIST CHILD PROCESSES' bit is unique to WebKitTestRunner. That's the code that
needs to remain in the Driver, it would break API tests and ImageDiff if it was
moved to ServerProcess.
> 
> I'm also not sure why you would want to add this to DumpRenderTree, why would
we want to do extra IPC just to answer a question you already know the answer
to? If DumpRenderTree might have child processes in the future, that would be
understandable, but by design, DumpRenderTree will never have child processes.

Sorry, I didn't see that "#CHECK WORLD LEAKS" was added to DumpRenderTree, but
did nothing, so it's simple to add support for "#LIST CHILD PROCESSES".

I will post a new patch shortly.