Bug 179828 - run-webkit-tests not gathering crash logs on Cygwin Python and Windows Python
Summary: run-webkit-tests not gathering crash logs on Cygwin Python and Windows Python
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-17 10:23 PST by Jonathan Bedard
Modified: 2019-05-20 18:54 PDT (History)
8 users (show)

See Also:


Attachments
Patch (7.22 KB, patch)
2019-05-16 03:30 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (6.89 KB, patch)
2019-05-19 19:39 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2017-11-17 10:23:53 PST
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>.
Comment 1 Fujii Hironori 2019-05-16 01:23:46 PDT
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.
Comment 2 Fujii Hironori 2019-05-16 01:27:34 PDT
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]
Comment 3 Fujii Hironori 2019-05-16 01:50:57 PDT
We can use /proc/$pid/winpid

Convert a Cygwin PID to a Windows PID
https://stackoverflow.com/a/44862746/2691131
Comment 4 Fujii Hironori 2019-05-16 03:30:46 PDT
Created attachment 370035 [details]
Patch
Comment 5 Jonathan Bedard 2019-05-16 08:12:57 PDT
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.
Comment 6 Fujii Hironori 2019-05-16 14:41:01 PDT
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.
Comment 7 Jonathan Bedard 2019-05-16 20:12:48 PDT
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.
Comment 8 Fujii Hironori 2019-05-17 04:54:33 PDT
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.
Comment 9 Jonathan Bedard 2019-05-17 08:09:57 PDT
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.
Comment 10 Fujii Hironori 2019-05-19 19:39:17 PDT
Created attachment 370233 [details]
Patch

* Addressed review feedbacks.
Comment 11 Fujii Hironori 2019-05-20 18:53:21 PDT
Comment on attachment 370233 [details]
Patch

Clearing flags on attachment: 370233

Committed r245544: <https://trac.webkit.org/changeset/245544>
Comment 12 Fujii Hironori 2019-05-20 18:53:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2019-05-20 18:54:17 PDT
<rdar://problem/50970026>