Bug 179828

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.

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]

We can use /proc/$pid/winpid Convert a Cygwin PID to a Windows PID https://stackoverflow.com/a/44862746/2691131

Created attachment 370035 [details] Patch

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 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 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 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 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.

Created attachment 370233 [details] Patch * Addressed review feedbacks.

Comment on attachment 370233 [details] Patch Clearing flags on attachment: 370233 Committed r245544: <https://trac.webkit.org/changeset/245544>

All reviewed patches have been landed. Closing bug.

<rdar://problem/50970026>