Bug 122319

Summary:

WinLauncher needs crash report

Product:

WebKit

Reporter:

Alex Christensen <alex.christensen>

Component:

Tools / Tests

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, commit-queue

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

Windows 7

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Alex Christensen

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

Attachments
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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Alex Christensen

Comment 1 2013-10-03 22:41:14 PDT

Created attachment 213336 [details] Patch

Brent Fulgham

Comment 2 2013-10-04 11:55:57 PDT

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

Comment 3 2013-10-04 12:39:54 PDT

Created attachment 213389 [details] Patch

Alex Christensen

Comment 4 2013-10-04 12:50:26 PDT

Created attachment 213391 [details] Patch

Brent Fulgham

Comment 5 2013-10-04 12:59:05 PDT

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

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

WebKit Commit Bot

Comment 7 2013-10-04 13:36:30 PDT

Comment on attachment 213391 [details] Patch Clearing flags on attachment: 213391 Committed r156911: <http://trac.webkit.org/changeset/156911>

WebKit Commit Bot

Comment 8 2013-10-04 13:36:32 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.