Bug 122319 – WinLauncher needs crash report

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

[Alex Christensen]

When WinLauncher crashes, it would be nice to generate a crash report that can be debugged on another computer with the correct pdb files.  I make a big crash report in debug builds and a little one for uploading from release builds.

Also, I'd like to modify what happens with the crash reports and the default HTML that shows in WinLauncher.  To minimize the tricky things I'll need to do with strings I'd like to manage a header file with modifications that are not general to the WebKit community.

The error messages that pop up while using WinLauncher are also only necessary while debugging.  This patch is a general improvement to WinLauncher.

[Alex Christensen]

Created attachment 213336 [details]
Patch

[Brent Fulgham]

Comment on attachment 213336 [details]
Patch

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

This looks great -- but I have a few questions/suggestions before we can land this.

> Tools/WinLauncher/WinLauncher.cpp:180
> +#ifdef _DEBUG

I don't understand this change. Why don't we want to see why the load failed in Release mode? If you don't you will be puzzled by the browser window not displaying any content when you enter a new URL.

> Tools/WinLauncher/WinLauncher.cpp:416
> +    HANDLE miniDumpFile = ::CreateFile(L"CrashReport.dmp", GENERIC_WRITE, 0, 0, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, 0);

You should create this crash report in some definite user-writable location.  See the documentation of SHGetFolderPath (http://msdn.microsoft.com/en-us/library/windows/desktop/bb762181(v=vs.85).aspx) -- I think you probably want to put these files in a sub-folder of CSIDL_LOCAL_APPDATA.  Then input THAT string into this ::CreateFile function.

> Tools/WinLauncher/WinLauncher.cpp:433
> +        processCrashReport();

So will this be dealt with in a future bug? Or is it left for WinLauncher "users" to flesh out in their own releases.  If it's the latter, I think you should add a comment about this in the "WinLauncherReplace.h" header.

[Alex Christensen]

Created attachment 213389 [details]
Patch

[Alex Christensen]

Created attachment 213391 [details]
Patch

[Brent Fulgham]

Comment on attachment 213391 [details]
Patch

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

r=me, but please fix the missing namespace on the SHGetFolderPathW call when you land.

> Tools/WinLauncher/WinLauncher.cpp:418
> +    if (FAILED(SHGetFolderPathW(0, CSIDL_LOCAL_APPDATA | CSIDL_FLAG_CREATE, 0, 0, appDataDirectory)))

::SHGetFolderPathW

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 213391 [details]:

http/tests/inspector/resource-tree/resource-tree-frame-navigate.html bug 122347 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 213391 [details]
Patch

Clearing flags on attachment: 213391

Committed r156911: <http://trac.webkit.org/changeset/156911>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.