Bug 122319 - WinLauncher needs crash report
Summary: WinLauncher needs crash report
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2013-10-03 22:36 PDT by Alex Christensen
Modified: 2013-10-04 13:36 PDT (History)
2 users (show)

See Also:

Patch (9.10 KB, patch)
2013-10-03 22:41 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (9.42 KB, patch)
2013-10-04 12:39 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (9.87 KB, patch)
2013-10-04 12:50 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2013-10-03 22:36:39 PDT
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.
Comment 1 Alex Christensen 2013-10-03 22:41:14 PDT
Created attachment 213336 [details]
Comment 2 Brent Fulgham 2013-10-04 11:55:57 PDT
Comment on attachment 213336 [details]

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.
Comment 3 Alex Christensen 2013-10-04 12:39:54 PDT
Created attachment 213389 [details]
Comment 4 Alex Christensen 2013-10-04 12:50:26 PDT
Created attachment 213391 [details]
Comment 5 Brent Fulgham 2013-10-04 12:59:05 PDT
Comment on attachment 213391 [details]

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

Comment 6 WebKit Commit Bot 2013-10-04 13:35:36 PDT
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: pfeldman@chromium.org, podivilov@chromium.org, and webkit.review.bot@gmail.com)
The commit-queue is continuing to process your patch.
Comment 7 WebKit Commit Bot 2013-10-04 13:36:30 PDT
Comment on attachment 213391 [details]

Clearing flags on attachment: 213391

Committed r156911: <http://trac.webkit.org/changeset/156911>
Comment 8 WebKit Commit Bot 2013-10-04 13:36:32 PDT
All reviewed patches have been landed.  Closing bug.