Last Comment Bug 358082 - improve crash reporter client UI
: improve crash reporter client UI
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Breakpad Integration (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Dave Camp (:dcamp)
:
Mentors:
: 366971 (view as bug list)
Depends on: 354980 378581
Blocks:
  Show dependency treegraph
 
Reported: 2006-10-25 14:40 PDT by Ted Mielczarek [:ted.mielczarek]
Modified: 2008-03-09 08:02 PDT (History)
23 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
beltzner's mockup (84.24 KB, image/png)
2007-05-11 13:20 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details
MainMenu.nib update (10.91 KB, application/octet-stream)
2007-05-24 17:24 PDT, Dave Camp (:dcamp)
no flags Details
first stab (58.50 KB, patch)
2007-05-24 17:32 PDT, Dave Camp (:dcamp)
ted: review+
Details | Diff | Splinter Review
default UI screenshot, osx (48.22 KB, image/png)
2007-05-25 14:19 PDT, Dave Camp (:dcamp)
no flags Details
with report details, osx (66.58 KB, image/png)
2007-05-25 14:20 PDT, Dave Camp (:dcamp)
no flags Details
default ui, windows (16.03 KB, image/png)
2007-05-25 14:20 PDT, Dave Camp (:dcamp)
no flags Details
with details, win (19.73 KB, image/png)
2007-05-25 14:21 PDT, Dave Camp (:dcamp)
no flags Details
MainMenu.nib update (11.32 KB, application/octet-stream)
2007-05-26 13:01 PDT, Dave Camp (:dcamp)
no flags Details
second stab (58.32 KB, patch)
2007-05-26 13:04 PDT, Dave Camp (:dcamp)
benjamin: review+
Details | Diff | Splinter Review
small fixup for linux (830 bytes, patch)
2007-06-01 07:46 PDT, Ted Mielczarek [:ted.mielczarek]
benjamin: review+
Details | Diff | Splinter Review

Description Ted Mielczarek [:ted.mielczarek] 2006-10-25 14:40:10 PDT
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.
Comment 1 Ted Mielczarek [:ted.mielczarek] 2006-10-26 12:04:22 PDT
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.

Comment 2 Wolfgang Rosenauer [:wolfiR] 2007-01-29 07:07:03 PST
Besides enable/disable we might want to have a "always ask".
Comment 3 Ted Mielczarek [:ted.mielczarek] 2007-05-11 13:20:19 PDT
Created attachment 264523 [details]
beltzner's mockup

This is beltzner's concept.  I'll post some of his writeup here as well shortly.
Comment 4 Ted Mielczarek [:ted.mielczarek] 2007-05-11 13:31:31 PDT
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.
Comment 5 Ted Mielczarek [:ted.mielczarek] 2007-05-11 13:45:39 PDT
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.
Comment 6 Ted Mielczarek [:ted.mielczarek] 2007-05-16 10:13:16 PDT
*** Bug 366971 has been marked as a duplicate of this bug. ***
Comment 7 Wayne Mery (:wsmwk, NI for questions) 2007-05-16 10:31:31 PDT
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..."?
Comment 8 Smokey Ardisson (offline for a while; not following bugs - do not email) 2007-05-19 21:41:15 PDT
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 ;)
Comment 9 Adam Guthrie 2007-05-21 11:27:48 PDT
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.
Comment 10 Dave Camp (:dcamp) 2007-05-24 17:24:29 PDT
Created attachment 266014 [details]
MainMenu.nib update
Comment 11 Dave Camp (:dcamp) 2007-05-24 17:32:08 PDT
Created attachment 266016 [details] [diff] [review]
first stab

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 12 Dave Camp (:dcamp) 2007-05-25 14:19:34 PDT
Created attachment 266106 [details]
default UI screenshot, osx
Comment 13 Dave Camp (:dcamp) 2007-05-25 14:20:09 PDT
Created attachment 266107 [details]
with report details, osx
Comment 14 Dave Camp (:dcamp) 2007-05-25 14:20:38 PDT
Created attachment 266108 [details]
default ui, windows
Comment 15 Dave Camp (:dcamp) 2007-05-25 14:21:05 PDT
Created attachment 266109 [details]
with details, win
Comment 16 Ted Mielczarek [:ted.mielczarek] 2007-05-25 15:28:51 PDT
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 17 Smokey Ardisson (offline for a while; not following bugs - do not email) 2007-05-25 18:39:30 PDT
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.
Comment 18 Dave Camp (:dcamp) 2007-05-26 13:01:56 PDT
Created attachment 266208 [details]
MainMenu.nib update

Updated MainMenu.nib.

Fixes the checkbox spacing (kinda lame that interface builder gets that wrong), and fixes the tab order.
Comment 19 Dave Camp (:dcamp) 2007-05-26 13:04:30 PDT
Created attachment 266210 [details] [diff] [review]
second stab

Fixes for ted's comments.
Fixes the tab order on windows.
Puts the launch arguments in environment variables rather than the annotations.
Comment 20 Dave Camp (:dcamp) 2007-05-26 13:09:15 PDT
filed bug 382128 about the firefox-specific description.
Comment 21 Benjamin Smedberg [:bsmedberg] 2007-05-26 16:31:09 PDT
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.
Comment 22 timeless 2007-05-26 16:34:20 PDT
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.
Comment 23 Ted Mielczarek [:ted.mielczarek] 2007-05-26 19:46:05 PDT
Couldn't that just be ToNewCString(envVar), which would make NSPR happy?
Comment 24 timeless 2007-05-27 21:21:27 PDT
yes (be sure to deal w/ ToNewString failing though ;-)
Comment 25 Gábor Stefanik 2007-05-28 07:06:18 PDT
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!".
Comment 26 Benjamin Smedberg [:bsmedberg] 2007-05-28 07:20:30 PDT
It is not a joke. Why do you object to it?
Comment 27 Gábor Stefanik 2007-05-28 08:04:35 PDT
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.
Comment 28 Benjamin Smedberg [:bsmedberg] 2007-05-29 04:23:53 PDT
Fixed on trunk. I'm having some minor issues with relaunch args but I'll file that separately.
Comment 29 Jim Jeffery not reading bug-mail 1/2/11 2007-05-29 06:42:45 PDT
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]
Comment 30 Jim Jeffery not reading bug-mail 1/2/11 2007-05-29 07:45:58 PDT
(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 31 Ted Mielczarek [:ted.mielczarek] 2007-06-01 07:46:25 PDT
Created attachment 266914 [details] [diff] [review]
small fixup for linux
Comment 32 Ted Mielczarek [:ted.mielczarek] 2007-06-01 07:47:10 PDT
Comment on attachment 266914 [details] [diff] [review]
small fixup for linux 

Too lazy to file a new bug.
Comment 33 Ted Mielczarek [:ted.mielczarek] 2007-06-02 06:34:32 PDT
Comment on attachment 266914 [details] [diff] [review]
small fixup for linux 

Checked in.
Comment 34 Mike Beltzner [:beltzner, not reading bugmail] 2007-06-07 11:46:18 PDT
(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?
Comment 35 Smokey Ardisson (offline for a while; not following bugs - do not email) 2007-06-07 15:25:04 PDT
(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.
Comment 36 Marcia Knous [:marcia - use ni] 2007-06-20 13:29:40 PDT
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?
> 

Comment 37 Adam Guthrie 2007-06-20 13:50:00 PDT
Marcia, both of those seems like valid points, but I think you should file a follow-up bug, as this bug is already FIXED.
Comment 38 Adam Guthrie 2007-06-20 14:36:50 PDT
Marcia filed bug 385236 to address the wording issue.

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