Bug 179828 – run-webkit-tests not gathering crash logs on Cygwin Python and Windows Python

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

[Jonathan Bedard]

This seems like is has been a long-standing bug, as discussed in <https://bugs.webkit.org/show_bug.cgi?id=179514>.

Example of this <https://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/3191>.

[Fujii Hironori]

find_system_pid has been removed in Bug 179514.
However it is still needed because Windows PID should be stored in 'pid_to_system_pid' dictionary before crashing DumpRenderTree in Cygwin Python.

[Fujii Hironori]

There is one more problem.
CrashLog format has been slightly changed. I don't know since when.

CrashLogs.GLOBAL_PID_REGEX should be accommodate the new format

Current regexp:

> GLOBAL_PID_REGEX = re.compile(r'\s+Global\s+PID:\s+\[(?P<pid>\d+)\]')

Actual text:

> Global    D1    PID: [9976]

[Fujii Hironori]

We can use /proc/$pid/winpid

Convert a Cygwin PID to a Windows PID
https://stackoverflow.com/a/44862746/2691131

[Fujii Hironori]

Created attachment 370035 [details]
Patch

[Jonathan Bedard]

Comment on attachment 370035 [details]
Patch

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

> Tools/Scripts/webkitpy/port/win.py:-432
> -            crash_log = crash_logs.find_newest_log(name, system_pid, include_errors=True, newer_than=newer_than)

Is the system PID actually useful for something other than crashlogs?

It looks like the original intention here was to have the _get_crash_log function do the cygwin mapping magic, and this function should have all the needed information to generate the system PID. If the system PID is only useful for crashlogs, it seems like it should live entirely in this function.

[Fujii Hironori]

Comment on attachment 370035 [details]
Patch

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

>> Tools/Scripts/webkitpy/port/win.py:-432
>> -            crash_log = crash_logs.find_newest_log(name, system_pid, include_errors=True, newer_than=newer_than)
> 
> Is the system PID actually useful for something other than crashlogs?
> 
> It looks like the original intention here was to have the _get_crash_log function do the cygwin mapping magic, and this function should have all the needed information to generate the system PID. If the system PID is only useful for crashlogs, it seems like it should live entirely in this function.

CrashLogs is constructed after crashing. It’s too late to get pid.

[Jonathan Bedard]

Comment on attachment 370035 [details]
Patch

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

> Tools/Scripts/webkitpy/common/system/crashlogs.py:40
> +    GLOBAL_PID_REGEX = re.compile(r'\s+Global\b.+\bPID:\s+\[(?P<pid>\d+)\]')

This new format actually makes me quire uncomfortable....what exactly is new between 'Global' and 'PID'? This new regex would match any string between the two, so long as 'Global' and 'PID:' are separate words. Do we have examples of the new string format?

> Tools/Scripts/webkitpy/port/server_process.py:126
> +    def _find_system_pid(self, pid):

Making this a function doesn't seem needed, since it could be a one-line.

self._system_pid = int(self._port._filesystem.read_text_file('/proc/%d/winpid' % pid)) if self._port.host.platform.is_cygwin() else pid

>>> Tools/Scripts/webkitpy/port/win.py:-432
>>> -            crash_log = crash_logs.find_newest_log(name, system_pid, include_errors=True, newer_than=newer_than)
>> 
>> Is the system PID actually useful for something other than crashlogs?
>> 
>> It looks like the original intention here was to have the _get_crash_log function do the cygwin mapping magic, and this function should have all the needed information to generate the system PID. If the system PID is only useful for crashlogs, it seems like it should live entirely in this function.
> 
> CrashLogs is constructed after crashing. It’s too late to get pid.

That makes sense.

[Fujii Hironori]

Comment on attachment 370035 [details]
Patch

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

>> Tools/Scripts/webkitpy/common/system/crashlogs.py:40
>> +    GLOBAL_PID_REGEX = re.compile(r'\s+Global\b.+\bPID:\s+\[(?P<pid>\d+)\]')
> 
> This new format actually makes me quire uncomfortable....what exactly is new between 'Global' and 'PID'? This new regex would match any string between the two, so long as 'Global' and 'PID:' are separate words. Do we have examples of the new string format?

See comment 2 and bug 197941 comment 2.

>> Tools/Scripts/webkitpy/port/server_process.py:126
>> +    def _find_system_pid(self, pid):
> 
> Making this a function doesn't seem needed, since it could be a one-line.
> 
> self._system_pid = int(self._port._filesystem.read_text_file('/proc/%d/winpid' % pid)) if self._port.host.platform.is_cygwin() else pid

Will fix.

[Jonathan Bedard]

Comment on attachment 370035 [details]
Patch

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

>>> Tools/Scripts/webkitpy/common/system/crashlogs.py:40
>>> +    GLOBAL_PID_REGEX = re.compile(r'\s+Global\b.+\bPID:\s+\[(?P<pid>\d+)\]')
>> 
>> This new format actually makes me quire uncomfortable....what exactly is new between 'Global' and 'PID'? This new regex would match any string between the two, so long as 'Global' and 'PID:' are separate words. Do we have examples of the new string format?
> 
> See comment 2 and bug 197941 comment 2.

Could we put a comment above the regex with something like:

# Matches a string like '    Global    D1    PID: [14516]'

Actually seeing the string we're matching makes me feel better about the regex changes.

[Fujii Hironori]

Created attachment 370233 [details]
Patch

* Addressed review feedbacks.

[Fujii Hironori]

Comment on attachment 370233 [details]
Patch

Clearing flags on attachment: 370233

Committed r245544: <https://trac.webkit.org/changeset/245544>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/50970026>