Bug 112409 – Collect samples for unresponsive web processes

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

[Simon Fraser (smfr)]

Collect samples for unresponsive web processes

[Simon Fraser (smfr)]

Created attachment 193239 [details]
Patch

[Tim Horton]

Comment on attachment 193239 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:172
> +        self._write_text_file(filename, self._filesystem.read_text_file(sample_file))

_filesystem.copyfile?

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:190
> +        return self._filesystem.join(self.results_directory(), "%s-%s-sample.txt" % (name, pid))

%-formatting is deprecated (or at least frowned upon, I'm not sure). I see a lot of it around, but "{0}-{1}-sample.txt".format(name, pid) is the way of the future. Less magic syntax ftw.

[Ryosuke Niwa]

Comment on attachment 193239 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/base.py:1347
> +        pass

We probably need to return [] here.

[Tim Horton]

(In reply to comment #3)
> (From update of attachment 193239 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193239&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/base.py:1347
> > +        pass
> 
> We probably need to return [] here.

Nah, the caller tests for Noneness.

[Dirk Pranke]

Comment on attachment 193239 [details]
Patch

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

>>> Tools/Scripts/webkitpy/layout_tests/port/base.py:1347
>>> +        pass
>> 
>> We probably need to return [] here.
> 
> Nah, the caller tests for Noneness.

Still, it's very unidiomatic python code (in my experience at least) to not explicitly return *something* and rely on the implicit None. It's easy to mistake this for a mistake, so we should change this to either 'return {}' or 'return None'.

>> Tools/Scripts/webkitpy/layout_tests/port/mac.py:190
>> +        return self._filesystem.join(self.results_directory(), "%s-%s-sample.txt" % (name, pid))
> 
> %-formatting is deprecated (or at least frowned upon, I'm not sure). I see a lot of it around, but "{0}-{1}-sample.txt".format(name, pid) is the way of the future. Less magic syntax ftw.

I don't think either of these statements is true in Python 2.x code; certainly we use % args way way more than {} args in webkitpy. I have no objection to the {} syntax, but I wouldn't require it at this time.

[Tim Horton]

Comment on attachment 193239 [details]
Patch

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

>>>> Tools/Scripts/webkitpy/layout_tests/port/base.py:1347
>>>> +        pass
>>> 
>>> We probably need to return [] here.
>> 
>> Nah, the caller tests for Noneness.
> 
> Still, it's very unidiomatic python code (in my experience at least) to not explicitly return *something* and rely on the implicit None. It's easy to mistake this for a mistake, so we should change this to either 'return {}' or 'return None'.

If we change this for *that* reason, then we should change the others nearby too :|

[Dirk Pranke]

Comment on attachment 193239 [details]
Patch

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

>>>>> Tools/Scripts/webkitpy/layout_tests/port/base.py:1347
>>>>> +        pass
>>>> 
>>>> We probably need to return [] here.
>>> 
>>> Nah, the caller tests for Noneness.
>> 
>> Still, it's very unidiomatic python code (in my experience at least) to not explicitly return *something* and rely on the implicit None. It's easy to mistake this for a mistake, so we should change this to either 'return {}' or 'return None'.
> 
> If we change this for *that* reason, then we should change the others nearby too :|

Yes. I wasn't aware that there were many other places doing this.

Also, since there's no type declarations in Python and WebKit is rather un-comment/docstring-friendly, not returning a value gives you no indication that the method is supposed to return anything, which makes it hard to tell what subclasses should implement.

Also also -- since I'm just noticing this -- this change should have at least some basic unit tests.

[Ryosuke Niwa]

I don't think we need a test for this. It's a temporary measure to get samples to diagnose unresponsive WebProcesses we see on bots.

[Dirk Pranke]

(In reply to comment #8)
> I don't think we need a test for this. It's a temporary measure to get samples to diagnose unresponsive WebProcesses we see on bots.

What makes you think that this is a temporary need? I don't see anything in this bug (or the patch) indicating that this is intended to be temporary.

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/145916