Crash reporter cleanup and fix auto-send config


Submitter:	Stone Linden	Reviewers
Branch:	http://bitbucket.org/stone_linden/viewer-development-crashlog	Groups:
Bugs:	storm-1482	People:	alain_linden, jenn, nat_linden
Change Number:	None	Repository:	viewer-development

Description:

The Viewer ships with the crash reporter config set to auto-send crash reports, but at runtime overwrites this config with always-ask. Restore default auto-send functionality, as well as respecting other options as configured.

And be a good code citizen; do janitorial work as necessary to make this feature neat and clean.

Testing Done:

Builds.

Expand All

indra/newview/llappviewer.cpp (Diff revision 1)

Why is this a warning?  Is something wrong?  Would info level be better?

indra/newview/llappviewer.cpp (Diff revision 1)

Again, warning or info?

indra/llcommon/indra_constants.h (Diff revision 1)

I'm not necessarily opposed to this change, but I'd like to ask your reason.

I'm sure that whoever originally coded those string constants was defending against subtle, hard-to-spot typos. Such a typo in a string literal means things silently fail. By contrast, a typo in a reference to the name of a constant produces a big, obvious compile error.

Am I correct that you're thinking it's easier to grep for the actual string than to grep twice, once for the string value in the symbol definition, then again for references to its symbolic name in the code?

Stone Linden July 7, 2011, 11:36 a.m. (July 7, 2011, 11:36 a.m.)

I couldn't find any other config files specified in this way. There's app_settings/settings_files.xml which sets up which files should be used for which config data, but it's all very klugey in code for everything that's not in the Globals config group.

Yes, I do think it's easier to grep for the actual string. All other settings are done this way; the constant is the odd-man-out.

indra/llcrashlogger/llcrashlogger.cpp (Diff revision 1)

This new switch and the one below it make me cringe. Would you consider an inline function in the header, right where the enum is defined, something like 'isValidCrashBehavior(S32)'? Then this switch could be replaced with 'if (isValidCrashBehavior(value)) return value;'

indra/llcrashlogger/llcrashlogger.cpp (Diff revision 1)

This switch could become 'if (! isValidCrashBehavior(crash_behavior)) return false;'

Stone Linden July 7, 2011, 11:34 a.m. (July 7, 2011, 11:34 a.m.)

I like that better, not sure what header file I'd put it into though. indra_constants.h feels a little wrong for this.

Nat Goodspeed July 7, 2011, 11:54 a.m. (July 7, 2011, 11:54 a.m.)

Well, the reason I suggested defining isValidCrashBehavior() right adjacent to the enum definition is that then you could plausibly get away with using < tests. Or you could write the (one and only) switch statement restating all valid values there. If you want isValidCrashBehavior() to be in a different header, then I'd suggest moving the enum to be in the same place in the same header.

indra/llcrashlogger/llcrashlogger.cpp (Diff revision 1)

Heh, maybe:

mCrashSettings.declareS32("CrashSubmitBehavior", CRASH_BEHAVIOR_ASK, STRINGIZE("Controls behavior when viewer crashes (" << CRASH_BEHAVIOR_ASK << " = ask before sending crash report, " << CRASH_BEHAVIOR_ALWAYS_SEND << " = always send crash report, " << CRASH_BEHAVIOR_NEVER_SEND << " = never send crash report)"));

with appropriate line breaks, of course.

STRINGIZE is in "stringize.h".

Stone Linden July 7, 2011, 11:32 a.m. (July 7, 2011, 11:32 a.m.)

Why stringize macro when this is part of the language spec? (Well, it was in C anyhow...)

Nat Goodspeed July 7, 2011, 11:57 a.m. (July 7, 2011, 11:57 a.m.)

Hmm, I think you're thinking of something like:

#CRASH_BEHAVIOR_ASK " = ask before sending crash report, "  // etc. etc.

which I think would produce the string:

"CRASH_BEHAVIOR_ASK = ask before sending crash report, " // etc. etc.

STRINGIZE() is just shorthand for instantiating a std::ostringstream, throwing a buncha stuff at it and then retrieving its str().

indra/newview/llappviewer.cpp (Diff revision 1)

Doesn't this file already #include "indra_constants.h"? Couldn't we just use CRASH_BEHAVIOR_NEVER_SEND ?

Stone Linden July 7, 2011, 11:31 a.m. (July 7, 2011, 11:31 a.m.)
```
Yes, good catch. I've removed several of these already.
```

indra/newview/llappviewer.cpp (Diff revision 1)

Urk. I don't suppose the existing settings machinery has a feature to segregate out different variables to different files, but introducing --setcrash for this one variable in this file feels like overkill. Kinda wish you could set it with --set and have the viewer figure out which file it belongs in.

Stone Linden July 7, 2011, 11:35 a.m. (July 7, 2011, 11:35 a.m.)

I wish it did; I could see about adding namespace support, so the option would be --set CrashSettings.CrashSubmitBehavior. That would apply then to any of the configs specified in app_settings/settings_files.xml:

    <file name="Global"
    <file name="PerAccount"
    <file name="CrashSettings"
    <file name="Warnings"

Default would be Global.

Diff
- added Diff r2

Testing Done

Changed from:

Builds, can set preferences, see them persist across restarts.
On prefs change, the settings_crash_behavior.xml file is written out with the new value.
When the viewer is started with --setcrash CrashSubmitBehavior 0/1/2 the settings file is written out with the new value.

No work yet on the installer.

Changed to:

Builds.

Description

Changed from:

Add a preference panel item to set when crash reports are sent. The options are:
o Ask before each crash report
o Always send crash reports
o Don't send crash reports

Add an option to the installer to set Always or Ask.

And be a good code citizen; do janitorial work as necessary to make this feature neat and clean.

Changed to:

The Viewer ships with the crash reporter config set to auto-send crash reports, but at runtime overwrites this config with always-ask. Restore default auto-send functionality, as well as respecting other options as configured.

And be a good code citizen; do janitorial work as necessary to make this feature neat and clean.

Summary changed from Preferences and install options for sending crash reports to Crash reporter cleanup and fix auto-send config

Description:

New approach to this issue.

indra/linux_crash_logger/linux_crash_logger.cpp (Diff revision 2)

I thoroughly approve of your testing the return from 'app.init()'. Stylistically, I'd prefer something like:

if (! app.init())
{
    llwarns << "!@#$%^&*(!" << llendl;
    return 1;
}

app.mainLoop();
// ...

because then you see both alternatives with a minimum of intervening code. But as I said, that's just a style point.

indra/newview/llappviewer.cpp (Diff revision 2)

Don't you mean substr(pos+1) here?

Diff
- added Diff r3

Description:

I think this is ready to ship. I've testing on Mac OS X and it works nicely, and even logs now, so you can see what it's up to! The log file is at ~/Library/Application\ Support/SecondLife/logs/crashreport.log on Mac, and some variant of that on the other platforms.

Nat - the one suggestion you made I didn't end up using was the isValidCrashBehavior check. Since I cut out the Viewer usage, and only have the list of possible values in two places right next to each other in llcrashlogger, I kept the switch statements.

You have a pending review.

Other reviews

Your comment

Review Board 1.6.11

This change has been marked as submitted.