The airbag crash reporter client has a very simple GUI currently. We probably want to collect the reporter's email and URL visited like talkback currently does. In addition, the "enable crash reporting" dialog currently has bogus text in the explanation box. Probably need some input from beltzner on this.

Oh yeah, we also either need to get airbag to give us progress notification, or get rid of the progress bar and just use a twirly thing like the throbber. Probably the latter, since minidumps are only about 60k anyway.

Besides enable/disable we might want to have a "always ask".

This is beltzner's concept. I'll post some of his writeup here as well shortly.

From beltzner's email: I like the way you're thinking here. As we discussed initially on IRC, when a user suffers a crash, the first reaction will be a combination of surprise and anger. The primary goal of the user at that point will be to get back to what they were doing at the time of the crash. So the primary goal of the Breakpad UI should be to help the user get back to what they were doing as quickly as possible, while at the same time extracting useful information. Fewer questions are obviously better, as are fewer pages of UI to go through. I'm thinking something like this: [see attachment] "View Report" could also button that launches a viewer window, although I like the idea of an expander widget that otherwise hides a textbox with the full information. If we want to make the URL opt-outable, that's just an extra checkbox to add. You'll note I've got that thing about the email address. One thing I'd *love* Breakpad to do is create an end-to-end user experience. It would be really cool if we could somehow make it so that when bugs created from crash reports are fixed, the people who reported that bug get emailed so they know that they actually helped fix something. We could do this in the reporter UI, or I guess we could relaunch Firefox and open a new tab that points to the crash reporter server and do it there. Obviously this is additional functionality, so feel free to prune it, but I think it makes for a solid user experience.

bsmedberg reminds us that there may in fact be personal information contained in the minidump itself, since it captures stack data. He also says there's some official data collection policy that Talkback links to, but I don't know where that is offhand.

Is it envisioned that there will be no option for the reporter to supply comments? mockup is nice and simple. Might it make sense to change "you can send us.." to "please send us..."?

Two comments: 1) If this really is "Mozilla Crash Reporter", i.e., client code to be shared across all mozilla.org apps which use breakpad, then the Firefox-specific language won't work (I'm thinking specifically about "restoring tabs" and, say, Thunderbird or whatnot). 2) Comments really are *essential* for anything other than "viewing this page crashes it automatically 100% of the time" (aka "crash on pageload") type crashes. One of the things that frustrates me the most in tackling Talkback reports is when you've got a big crasher and a vague stack and no reports with comments. A good comment is the difference between a frustrating collection of crashes and a filed bug that gets fixed. Also, tying back in to point 1, you aren't going to have "crash on pageload" type crashes in non-browser apps (at least not pageloads that have meaningful URLs to capture). 3) I lied, here's a third: is the URL of the current page being captured automatically somehow? I see no field for it. If it is, what about crashes that happen, e.g., when switching tabs, or before a page has been added to session history; what URL gets captured, if any? If I'm confused about point 1, please ignore it ;)

Smokey, #3 is bug 375083. There is some design discussion in that bug, but it's good that you mention it here, since it probably needs to be more formally spec'd.

here's a first stab at the UI on windows and osx: * On windows I didn't get around to the actual disclosure triangle, it's a toggle button instead. This is fixable if we want, it'll take a bit of work. * I took out the note about private data, since it isn't really true. * There isn't a place to add comments yet

Comment on attachment 266016 [details] [diff] [review] first stab >diff -r f584f3ac7e36 toolkit/airbag/client/crashreporter.cpp > bool CrashReporterSendCompleted(bool success, > const string& serverResponse) > { > if (success) { >+#if 0 > if (!gDumpFile.empty()) > UIDeleteFile(gDumpFile); > if (!gExtraFile.empty()) > UIDeleteFile(gExtraFile); >+#endif This is cause I removed the Delete= setting, isn't it? If it would make your testing easier, go ahead and add a MOZ_CRASHREPORTER_NO_DELETE_DUMP env var to control this. >+ if (!ReadStringsFromFile(gExtraFile, queryParameters, false)) { Humorously enough, you should be passing true here, since the data there really does need to be unescaped in the same exact way, I just never implemented that (and we don't have any extra data that uses newlines or backslashes currently). >+ vector<string> restartArgs; >+ for (int i = 0; true; i++) { I don't like this construct at all. Can you either invent an artificial maximum, or use a while loop or something, please? >diff -r f584f3ac7e36 toolkit/airbag/client/crashreporter.ini >--- a/toolkit/airbag/client/crashreporter.ini Thu May 24 17:02:19 2007 -0400 >+++ b/toolkit/airbag/client/crashreporter.ini Thu May 24 10:21:33 2007 -0700 >+CrashReporterDescription=We're sorry, but %s hit an unexpected problem and crashed. We'll try to restore your tabs and windows when it restarts.\n\nTo help us diagnose and repair this problem, you can send us a crash report. We need to deal with this better in the non-Firefox case. If you don't want to solve it here, please file a follow-up bug. >diff -r f584f3ac7e36 toolkit/airbag/client/crashreporter_win.cpp >-static bool GetRegValue(HKEY hRegKey, LPCTSTR valueName, DWORD* value) >-{ >- DWORD type, dataSize; >- dataSize = sizeof(DWORD); >- if (RegQueryValueEx(hRegKey, valueName, NULL, &type, (LPBYTE)value, &dataSize) == ERROR_SUCCESS >- && type == REG_DWORD) >- return true; >- >- return false; >-} Looks like you have some actual tab characters in there. Can you detabify? That happens to me all the time, I think mento is tired of yelling at me about it. >+static void RestartApplication() >+{ >+ wstring cmdLine; >+ >+ for (unsigned int i = 0; i < gRestartArgs.size(); i++) { >+ cmdLine += L"\"" + UTF8ToWide(gRestartArgs[i]) + L"\""; >+ } Shouldn't you be putting spaces between commandline args? >+static void UpdateEmail(HWND hwndDlg) >+ gQueryParameters[L"ReporterEmail"] = email; This field is just "Email" in the processor code, so it needs to match here. >+static BOOL CALLBACK CrashReporterDialogProc(HWND hwndDlg, UINT message, >+ if (CheckBoolKey(CRASH_REPORTER_KEY, SUBMIT_REPORT_VALUE, &enabled) && Can we change the reg key based on the vendor? \\Software\Vendor\Crash Reporter maybe? I guess we can sub in Mozilla as a default if vendor is blank. >+ >+ for (int i = 0; i < argc; i++) { >+ nsCAutoString arg("RestartArg"); >+ arg.AppendInt(i); >+ CrashReporter::AnnotateCrashReport(arg, nsDependentCString(argv[i])); >+ } Check with bsmedberg to make sure this is kosher! r=me with those fixes.

Comment on attachment 266014 [details] MainMenu.nib update Dave, are you setting up the key loop for this window in code (because IB notes there's no initial first repsonder set and complains all the UI elements are missing nextKey stuff)? If not, the a11y folks will go nuts. ;) Also, per the AHIG, there should be 8px between the checkboxes instead of 6px. I suppose the "controls area" is intended to be indented, but those extra 12px do make it look a little wonky IMO.

Updated MainMenu.nib. Fixes the checkbox spacing (kinda lame that interface builder gets that wrong), and fixes the tab order.

Fixes for ted's comments. Fixes the tab order on windows. Puts the launch arguments in environment variables rather than the annotations.

filed bug 382128 about the firefox-specific description.

Comment on attachment 266210 [details] [diff] [review] second stab >--- a/toolkit/airbag/nsAirbagExceptionHandler.cpp Fri May 25 19:33:42 2007 -0700 >+++ b/toolkit/airbag/nsAirbagExceptionHandler.cpp Fri May 25 19:33:42 2007 -0700 >@@ -414,4 +414,26 @@ nsresult AnnotateCrashReport(const nsACS > return NS_OK; > } > >+nsresult >+SetRestartArgs(int argc, char **argv) >+{ >+ int i; >+ nsCAutoString envVar; >+ for (i = 0; i < argc; i++) { >+ envVar = "MOZ_CRASHREPORTER_RESTART_ARG_"; >+ envVar.AppendInt(i); >+ envVar += "="; >+ envVar += argv[i]; >+ PR_SetEnv(envVar.get()); >+ } >+ >+ // make sure the arg list is terminated >+ envVar = "MOZ_CRASHREPORTER_RESTART_ARG_"; >+ envVar.AppendInt(i); >+ envVar += "="; Technically this doesn't conform to the PR_SetEnv API, because those strings are supposed to be statically available for the life of the program. However, I don't think it's a problem for any modern OS that we want to support for breakpad.

i suspect that at some point solaris might fall into this category. i'd have to check. i'm not very fond of people violating nspr contracts.

Couldn't that just be ToNewCString(envVar), which would make NSPR happy?

yes (be sure to deal w/ ToNewString failing though ;-)

I hope that "Crash! Bang! Boom!" line is a joke. We don't need more "delicious delicacies". (Similarly, about:config's "This gun is loaded" will need to go before release.) My idea is "Fatal error detected!".

It is not a joke. Why do you object to it?

Because we want Firefox 3 to go mainstream. We must remain formal. (Or why isn't the Windows Registry officially called "Dump of all the garbage that programs need to work"? Because of this!) We need a quality product.

Fixed on trunk. I'm having some minor issues with relaunch args but I'll file that separately.

This is not causing Windows to not start up, only calls the Crash-Boom-Bang reporter. Cannot close the window with X, Ctrl+F4, and clicking on Restore only starts the cycle over again. See: http://forums.mozillazine.org/viewtopic.php?p=2902531#2902531 for more reports. Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a5pre) Gecko/20070528 Minefield/3.0a5pre Firefox/3.0 ID:2007052818 [cairo]

(In reply to comment #29) > This is not causing Windows to not start up, only calls the Crash-Boom-Bang > reporter. Cannot close the window with X, Ctrl+F4, and clicking on Restore > only starts the cycle over again. > > See: > http://forums.mozillazine.org/viewtopic.php?p=2902531#2902531 for more reports. > > Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a5pre) Gecko/20070528 > Minefield/3.0a5pre Firefox/3.0 ID:2007052818 [cairo] > (In reply to comment #29) > This is not causing Windows to not start up, only calls the Crash-Boom-Bang > reporter. Cannot close the window with X, Ctrl+F4, and clicking on Restore > only starts the cycle over again. > > See: > http://forums.mozillazine.org/viewtopic.php?p=2902531#2902531 for more reports. > > Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a5pre) Gecko/20070528 > Minefield/3.0a5pre Firefox/3.0 ID:2007052818 [cairo] > Oops! Typo - first line should read: This is causing Windows to not start....

Comment on attachment 266914 [details] [diff] [review] small fixup for linux Too lazy to file a new bug.

Comment on attachment 266914 [details] [diff] [review] small fixup for linux Checked in.

(In reply to comment #25) > I hope that "Crash! Bang! Boom!" line is a joke. We don't need more "delicious > delicacies". (Similarly, about:config's "This gun is loaded" will need to go > before release.) My idea is "Fatal error detected!". It's not a joke. I'm willing to take string change suggestions there, but I ardently believe that the tone should be light and self-effacing. Using "Fatal error detected" doesn't help anyone. The user doesn't care that it was a "fatal" error versus any other kind, and certainly doesn't care that it was "detected". They want to know: - where'd my browser go? - oh crap, am I pooched and did I lose all my work? The dialog specifically answers those questions: - it crashed, our mistake! - nope, you can restart and pick up where you left off Formality of language doesn't confer quality. Not encountering this dialog at all infers quality; if the user gets here, they should be treated with respect and not made to feel like it was something they did. (In reply to comment #11) > * I took out the note about private data, since it isn't really true. Can we get a link to the privacy policy? I guess it won't load all that easily since the browser's crashed, though ;) > * There isn't a place to add comments yet Is there a follow up bug for this? Do you need design work from me? (In reply to comment #1) > get rid of the progress bar and just use a twirly thing like the throbber. > Probably the latter, since minidumps are only about 60k anyway. Followup bug?

(In reply to comment #34) > > * There isn't a place to add comments yet > > Is there a follow up bug for this? Do you need design work from me? Bug 382538.

I can point to a few hendrix users who were not too pleased with the "Crash Bang Boom" verbiage. I think in general people are frustrated when they crash, and although I am not certain what the right error messaging is, having something too light hearted might not be the right approach. On another note, I also think emphasizing the fact that these crash reports make the Firefox product better would be a good thing. Diagnose and repair the problem is good, but did we have any other thoughts here? (In reply to comment #34) > (In reply to comment #25) > > I hope that "Crash! Bang! Boom!" line is a joke. We don't need more "delicious > > delicacies". (Similarly, about:config's "This gun is loaded" will need to go > > before release.) My idea is "Fatal error detected!". > > It's not a joke. I'm willing to take string change suggestions there, but I > ardently believe that the tone should be light and self-effacing. > > Using "Fatal error detected" doesn't help anyone. The user doesn't care that it > was a "fatal" error versus any other kind, and certainly doesn't care that it > was "detected". They want to know: > > - where'd my browser go? > - oh crap, am I pooched and did I lose all my work? > > The dialog specifically answers those questions: > > - it crashed, our mistake! > - nope, you can restart and pick up where you left off > > Formality of language doesn't confer quality. Not encountering this dialog at > all infers quality; if the user gets here, they should be treated with respect > and not made to feel like it was something they did. > > (In reply to comment #11) > > * I took out the note about private data, since it isn't really true. > > Can we get a link to the privacy policy? I guess it won't load all that easily > since the browser's crashed, though ;) > > > * There isn't a place to add comments yet > > Is there a follow up bug for this? Do you need design work from me? > > (In reply to comment #1) > > get rid of the progress bar and just use a twirly thing like the throbber. > > Probably the latter, since minidumps are only about 60k anyway. > > Followup bug? >

Marcia, both of those seems like valid points, but I think you should file a follow-up bug, as this bug is already FIXED.

Marcia filed bug 385236 to address the wording issue.