Bug 76935 - [Windows] NRWT doesn't save crash logs on Apple's Windows port
Summary: [Windows] NRWT doesn't save crash logs on Apple's Windows port
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar, NRWT, PlatformOnly
Depends on:
Blocks: 38756
  Show dependency treegraph
 
Reported: 2012-01-24 13:00 PST by Adam Roben (:aroben)
Modified: 2013-05-30 20:31 PDT (History)
7 users (show)

See Also:


Attachments
Patch (7.09 KB, patch)
2013-05-30 12:00 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (7.33 KB, patch)
2013-05-30 17:49 PDT, Brent Fulgham
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2012-01-24 13:00:04 PST
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.
Comment 1 Radar WebKit Bug Importer 2012-01-24 13:00:20 PST
<rdar://problem/10747585>
Comment 2 Brent Fulgham 2013-05-24 17:03:49 PDT
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.
Comment 3 Brent Fulgham 2013-05-30 12:00:55 PDT
Created attachment 203370 [details]
Patch
Comment 4 Brent Fulgham 2013-05-30 12:09:29 PDT
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.
Comment 5 Ryosuke Niwa 2013-05-30 12:21:26 PDT
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))?
Comment 6 Brent Fulgham 2013-05-30 17:47:43 PDT
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.
Comment 7 Brent Fulgham 2013-05-30 17:49:13 PDT
Created attachment 203403 [details]
Patch
Comment 8 Ryosuke Niwa 2013-05-30 18:45:15 PDT
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
Comment 9 Brent Fulgham 2013-05-30 20:31:31 PDT
Committed r151004: <http://trac.webkit.org/changeset/151004>