[Windows] NRWT doesn't save crash logs on Apple's Windows port

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

NRWT doesn't save crash logs on Windows like ORWT does. This is an important thing to support before we switch our bots over to NRWT.

[Radar WebKit Bug Importer]

<rdar://problem/10747585>

[Brent Fulgham]

See 'setUpWindowsCrashLogSaving' in the ORWT Perl script to see what needs to be done here.

Note: Windows 8 takes long strides to make this even more impossible.  We might need to require that test bots have the registry key permissions changes to permit regular users to change this particular value.

[Brent Fulgham]

Created attachment 203370 [details]
Patch

[Brent Fulgham]

To encourage Windows 7 and Windows 8 to allow the registry keys to be modified, you need to do the following:

1. Since we are running in 32-bit mode, the actual key to be modified is "HKEY_LOCAL_MACHINE\SOFTWARE\Wow6432Node\Microsoft\Windows NT\CurrentVersion\AeDebug"  This is the location we are touching using the "regtool --wow32" command.

2. Adjust the security settings as follows:
(a) Launch regedit as an Administrator.
(b) Navigate to "HKEY_LOCAL_MACHINE\SOFTWARE\Wow6432Node\Microsoft\Windows NT\CurrentVersion\AeDebug".
(c) Right-click on this node, and select 'Permissions'
(d) Make sure that the "Users" group is given Full Control over this key.

This permission adjustment is needed to make sure that Windows will allow the script to flip the desired debugger from the ntsd.exe program back to whatever the user wanted originally.

[Ryosuke Niwa]

Comment on attachment 203370 [details]
Patch

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

Looks okay but needs to use more webkitpy abstractions to be consistent.

> Tools/Scripts/webkitpy/port/win.py:141
> +        possible_paths = [self._filesystem.join(os.environ['PROGRAMFILES'], "Windows Kits", "8.0", "Debuggers", "x64", "ntsd.exe"),
> +                          self._filesystem.join(os.environ['PROGRAMFILES'], "Windows Kits", "8.0", "Debuggers", "x86", "ntsd.exe"),
> +                          self._filesystem.join(os.environ['PROGRAMFILES'], "Debugging Tools for Windows (x86)", "ntsd.exe"),
> +                          self._filesystem.join(os.environ['ProgramW6432'], "Debugging Tools for Windows (x64)", "ntsd.exe"),
> +                          self._filesystem.join(os.environ['SYSTEMROOT'], "system32", "ntsd.exe")]

Nit: wrong indentation. self. should appear exactly 4 spaces to the right of possible.

> Tools/Scripts/webkitpy/port/win.py:157
> +        with open(command_file, "w") as text_file:
> +            text_file.write(".logopen /t \"%s\\CrashLog%s.txt\"\n" % (cygpath(self.results_directory()), self.CRASH_LOG_PREFIX))
> +            text_file.write(".srcpath \"%s\"\n" % cygpath(os.environ['WEBKIT_SOURCE']))
> +            text_file.write("!analyze -vv\n")
> +            text_file.write("~*kpn\n")
> +            text_file.write("q\n")

Please use self._filesystem.write_text_file. cygpath(os.environ['WEBKIT_SOURCE']) should be read off of WebKitFinder.webkit_base.

> Tools/Scripts/webkitpy/port/win.py:162
> +        value = subprocess.Popen(["regtool", "--wow32", "get", registry_key], bufsize=1024, stdout=subprocess.PIPE).communicate()[0]

Use webkitpy.common.system.executive.

> Tools/Scripts/webkitpy/port/win.py:174
> +        rc = subprocess.call(["regtool", "--wow32", "set", "-s", str(registry_key), str(value)])
> +        if rc == 2:
> +            rc = subprocess.call(["regtool", "--wow32", "add", "-s", str(registry_key)])
> +            if rc == 0:
> +                rc = subprocess.call(["regtool", "--wow32", "set", "-s", str(registry_key), str(value)])
> +        if rc:
> +            _log.warn("Error setting key: %s to value %s.  Error=%ld." % (key, value, rc))
> +            return False

Ditto.

> Tools/Scripts/webkitpy/port/win.py:190
> +        ntsd_path = self._ntsd_location()
> +        if None == ntsd_path:

Do: not ntsd_path.

> Tools/Scripts/webkitpy/port/win.py:199
> +        command_file = self.create_debugger_command_file()
> +        if None == command_file:

Ditto.

> Tools/Scripts/webkitpy/port/win.py:201
> +        debugger_options = "\"" + cygpath(ntsd_path) + "\" -p %ld -e %ld -g -lines -cf \"" + cygpath(command_file) + "\""

Why not '"%s" -p %ld -e %ld -g -lines -cf "%s"' % (cygpath(ntsd_path), cygpath(command_file))?

[Brent Fulgham]

Comment on attachment 203370 [details]
Patch

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

>> Tools/Scripts/webkitpy/port/win.py:141
>> +                          self._filesystem.join(os.environ['SYSTEMROOT'], "system32", "ntsd.exe")]
> 
> Nit: wrong indentation. self. should appear exactly 4 spaces to the right of possible.

Done.

>> Tools/Scripts/webkitpy/port/win.py:157
>> +            text_file.write("q\n")
> 
> Please use self._filesystem.write_text_file. cygpath(os.environ['WEBKIT_SOURCE']) should be read off of WebKitFinder.webkit_base.

Great!  I didn't know about WebKitFinder.  That's very useful.

>> Tools/Scripts/webkitpy/port/win.py:162
>> +        value = subprocess.Popen(["regtool", "--wow32", "get", registry_key], bufsize=1024, stdout=subprocess.PIPE).communicate()[0]
> 
> Use webkitpy.common.system.executive.

Done

>> Tools/Scripts/webkitpy/port/win.py:174
>> +            return False
> 
> Ditto.

Done

>> Tools/Scripts/webkitpy/port/win.py:190
>> +        if None == ntsd_path:
> 
> Do: not ntsd_path.

Done

>> Tools/Scripts/webkitpy/port/win.py:199
>> +        if None == command_file:
> 
> Ditto.

Done

>> Tools/Scripts/webkitpy/port/win.py:201
>> +        debugger_options = "\"" + cygpath(ntsd_path) + "\" -p %ld -e %ld -g -lines -cf \"" + cygpath(command_file) + "\""
> 
> Why not '"%s" -p %ld -e %ld -g -lines -cf "%s"' % (cygpath(ntsd_path), cygpath(command_file))?

Yes, that's much cleaner. I had to use the new string format syntax to avoid having to escape all of the other format specifiers (e.g., the '%ld' tokens) that are meant to be passed to the external utility.

[Brent Fulgham]

Created attachment 203403 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 203403 [details]
Patch

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

> Tools/Scripts/webkitpy/port/win.py:33
> +import subprocess

We shouldn't be importing this.

> Tools/Scripts/webkitpy/port/win.py:35
> +import tempfile

We shouldn't include this.

> Tools/Scripts/webkitpy/port/win.py:150
> +        debugger_temp_directory = tempfile.mkdtemp()

Use filesystem.mkdtemp

[Brent Fulgham]

Committed r151004: <http://trac.webkit.org/changeset/151004>