Add crash_reporter functionality

Add crash_reporter functionality

BUG=2616, 2617

[sosa, 2010-04-26 20:45:51]

http://codereview.chromium.org/1702008/diff/2001/3003
File src/platform/crash/crash_reporter.cc (right):

http://codereview.chromium.org/1702008/diff/2001/3003#newcode24
src/platform/crash/crash_reporter.cc:24:
"/var/run/crash_reporter/pending_clean_shutdown";
This location is on tmpfs so won't persist across reboots.  Is this intended?

http://codereview.chromium.org/1702008/diff/2001/3003#newcode88
src/platform/crash/crash_reporter.cc:88: if (!PrepareUncleanShutdownCheck()) {
Should this be part of the flagged if or up one level?  It seems like you should
prepare each time.

http://codereview.chromium.org/1702008/diff/2001/3008
File src/platform/crash/system_logging_mock.h (right):

http://codereview.chromium.org/1702008/diff/2001/3008#newcode8
src/platform/crash/system_logging_mock.h:8: #include "crash/system_logging.h"
#include <string>

http://codereview.chromium.org/1702008/diff/2001/3009
File src/platform/crash/user_collector.cc (right):

http://codereview.chromium.org/1702008/diff/2001/3009#newcode44
src/platform/crash/user_collector.cc:44: file_path_.c_str());
I don't understand this.

http://codereview.chromium.org/1702008/diff/2001/3010
File src/platform/crash/user_collector.h (right):

http://codereview.chromium.org/1702008/diff/2001/3010#newcode19
src/platform/crash/user_collector.h:19: bool
(*is_metrics_collection_allowed_)(),
No _ in argument

http://codereview.chromium.org/1702008/diff/2001/3010#newcode21
src/platform/crash/user_collector.h:21: virtual ~UserCollector();
Add documentation to all public methods

http://codereview.chromium.org/1702008/diff/2001/3014
File src/scripts/run_tests.sh (right):

http://codereview.chromium.org/1702008/diff/2001/3014#newcode47
src/scripts/run_tests.sh:47: cd -
You should define this before you declare your trap to it

[petkov, 2010-04-26 23:27:10]

Mostly nits...

http://codereview.chromium.org/1702008/diff/2001/3001
File src/platform/crash/Makefile (right):

http://codereview.chromium.org/1702008/diff/2001/3001#newcode6
src/platform/crash/Makefile:6: CXXFLAGS ?= -Wall -Werror -g
Does ebuild set CXXFLAGS? Do you still get -Wall -Werror?

http://codereview.chromium.org/1702008/diff/2001/3001#newcode16
src/platform/crash/Makefile:16: INCLUDE_DIRS = -I..
It might be intentional, but you wouldn't need -I.. if you remove the "crash/"
from the includes...

http://codereview.chromium.org/1702008/diff/2001/3003
File src/platform/crash/crash_reporter.cc (right):

http://codereview.chromium.org/1702008/diff/2001/3003#newcode6
src/platform/crash/crash_reporter.cc:6: #include <string>
I'm not entirely sure on style but <string> might need to be in a separate
group.

http://codereview.chromium.org/1702008/diff/2001/3003#newcode8
src/platform/crash/crash_reporter.cc:8: #include "base/file_util.h"
You pick these from /usr/include, right? Use <> instead of "" then. And group
appropriately.

http://codereview.chromium.org/1702008/diff/2001/3003#newcode13
src/platform/crash/crash_reporter.cc:13: #include "gflags/gflags.h"
Use <> instead of "" and group appropriately.

http://codereview.chromium.org/1702008/diff/2001/3003#newcode22
src/platform/crash/crash_reporter.cc:22: const char kCrashCounterHistogram[] =
"CrashCounter";
static?

You should add a "Prefix." to the histogram name. Maybe "Platform." or
"Logging."? Unless we come up with a better scheme... FYI, currently BootTime is
Platform.BootTime.

http://codereview.chromium.org/1702008/diff/2001/3003#newcode23
src/platform/crash/crash_reporter.cc:23: const char kUncleanShutdownFile[] =
static?

http://codereview.chromium.org/1702008/diff/2001/3003#newcode46
src/platform/crash/crash_reporter.cc:46:
MetricsLibrary::SendToChrome(std::string(kCrashCounterHistogram),
Sync to latest -- this should be SendEnumToChrome with name, sample and max
arguments.

http://codereview.chromium.org/1702008/diff/2001/3003#newcode56
src/platform/crash/crash_reporter.cc:56: char empty[] = "";
static const?

http://codereview.chromium.org/1702008/diff/2001/3003#newcode68
src/platform/crash/crash_reporter.cc:68:
MetricsLibrary::SendToChrome(std::string(kCrashCounterHistogram),
SendEnumToChrome

http://codereview.chromium.org/1702008/diff/2001/3003#newcode68
src/platform/crash/crash_reporter.cc:68:
MetricsLibrary::SendToChrome(std::string(kCrashCounterHistogram),
Consider checking IsMetricsCollectionAllowed here -- then you don't need the
is_metrics_collection_allowed callback to UserCollector.

http://codereview.chromium.org/1702008/diff/2001/3003#newcode93
src/platform/crash/crash_reporter.cc:93: } else if (FLAGS_clean_shutdown) {
Given the "return 0", you don't really need the "else" here. I'd suggest either
remove the return, or the else.

http://codereview.chromium.org/1702008/diff/2001/3003#newcode97
src/platform/crash/crash_reporter.cc:97: CHECK(FLAGS_signal != -1) << "Signal
must be set";
Add a comment that this mode handles a specific user process crash.

http://codereview.chromium.org/1702008/diff/2001/3004
File src/platform/crash/make_tests.sh (right):

http://codereview.chromium.org/1702008/diff/2001/3004#newcode3
src/platform/crash/make_tests.sh:3: # Copyright (c) 2009 The Chromium OS
Authors. All rights reserved.
2010

http://codereview.chromium.org/1702008/diff/2001/3007
File src/platform/crash/system_logging_mock.cc (right):

http://codereview.chromium.org/1702008/diff/2001/3007#newcode7
src/platform/crash/system_logging_mock.cc:7: #include "base/string_util.h"
Use <> instead of "" for base.

http://codereview.chromium.org/1702008/diff/2001/3009
File src/platform/crash/user_collector.cc (right):

http://codereview.chromium.org/1702008/diff/2001/3009#newcode8
src/platform/crash/user_collector.cc:8: #include "base/file_util.h"
Use <> instead of "" for base/

http://codereview.chromium.org/1702008/diff/2001/3009#newcode13
src/platform/crash/user_collector.cc:13: const char kCorePatternFile[] =
"/proc/sys/kernel/core_pattern";
static?

Can you add a couple of words about what this file does?

http://codereview.chromium.org/1702008/diff/2001/3010
File src/platform/crash/user_collector.h (right):

http://codereview.chromium.org/1702008/diff/2001/3010#newcode17
src/platform/crash/user_collector.h:17: void Initialize(void (*count_crash)(),
Consider typedef'ing the callbacks.

http://codereview.chromium.org/1702008/diff/2001/3011
File src/platform/crash/user_collector_test.cc (right):

http://codereview.chromium.org/1702008/diff/2001/3011#newcode8
src/platform/crash/user_collector_test.cc:8: #include "base/file_util.h"
Use <> instead of "" for base/

http://codereview.chromium.org/1702008/diff/2001/3011#newcode15
src/platform/crash/user_collector_test.cc:15: const char kFilePath[] =
"/my/path";
static?

[kmixter1, 2010-04-29 02:19:38]

PTAL

http://codereview.chromium.org/1702008/diff/2001/3001
File src/platform/crash/Makefile (right):

http://codereview.chromium.org/1702008/diff/2001/3001#newcode6
src/platform/crash/Makefile:6: CXXFLAGS ?= -Wall -Werror -g
It does and I don't.  Copied this from another project and it's not necessary. 
I'd rather turn on -Wall -Werror always except that DEFINE_string seems to cause
a warning/error.

On 2010/04/26 23:27:10, petkov wrote:
> Does ebuild set CXXFLAGS? Do you still get -Wall -Werror?
>

http://codereview.chromium.org/1702008/diff/2001/3001#newcode16
src/platform/crash/Makefile:16: INCLUDE_DIRS = -I..
It's intentional. :)

On 2010/04/26 23:27:10, petkov wrote:
> It might be intentional, but you wouldn't need -I.. if you remove the "crash/"
> from the includes...
>

http://codereview.chromium.org/1702008/diff/2001/3003
File src/platform/crash/crash_reporter.cc (right):

http://codereview.chromium.org/1702008/diff/2001/3003#newcode6
src/platform/crash/crash_reporter.cc:6: #include <string>
On 2010/04/26 23:27:10, petkov wrote:
> I'm not entirely sure on style but <string> might need to be in a separate
> group.

The style is supposed to be
#include <stdincludes>

#include "project/thirdparty/etc/includes"

Unfortunately we mix those concepts in this project.  As a compromise, since
metrics_library is a project specific library, I changed it to double quotes and
sorted it in the later list.

http://codereview.chromium.org/1702008/diff/2001/3003#newcode8
src/platform/crash/crash_reporter.cc:8: #include "base/file_util.h"
Same deal - I'm trying to keep the style as close to Google style as possible -
when I can use double quotes I do.  See other examples in src/platform as well.

On 2010/04/26 23:27:10, petkov wrote:
> You pick these from /usr/include, right? Use <> instead of "" then. And group
> appropriately.
>

http://codereview.chromium.org/1702008/diff/2001/3003#newcode13
src/platform/crash/crash_reporter.cc:13: #include "gflags/gflags.h"
On 2010/04/26 23:27:10, petkov wrote:
> Use <> instead of "" and group appropriately.
> 

Same as above.

http://codereview.chromium.org/1702008/diff/2001/3003#newcode23
src/platform/crash/crash_reporter.cc:23: const char kUncleanShutdownFile[] =
On 2010/04/26 23:27:10, petkov wrote:
> static?
> 

Done.

http://codereview.chromium.org/1702008/diff/2001/3003#newcode24
src/platform/crash/crash_reporter.cc:24:
"/var/run/crash_reporter/pending_clean_shutdown";
Switched to /var/lib.

On 2010/04/26 20:45:51, sosa wrote:
> This location is on tmpfs so won't persist across reboots.  Is this intended?

http://codereview.chromium.org/1702008/diff/2001/3003#newcode46
src/platform/crash/crash_reporter.cc:46:
MetricsLibrary::SendToChrome(std::string(kCrashCounterHistogram),
On 2010/04/26 23:27:10, petkov wrote:
> Sync to latest -- this should be SendEnumToChrome with name, sample and max
> arguments.
> 

Done.

http://codereview.chromium.org/1702008/diff/2001/3003#newcode56
src/platform/crash/crash_reporter.cc:56: char empty[] = "";
On 2010/04/26 23:27:10, petkov wrote:
> static const?
> 

Done.

http://codereview.chromium.org/1702008/diff/2001/3003#newcode68
src/platform/crash/crash_reporter.cc:68:
MetricsLibrary::SendToChrome(std::string(kCrashCounterHistogram),
On 2010/04/26 23:27:10, petkov wrote:
> SendEnumToChrome
> 

Done.

http://codereview.chromium.org/1702008/diff/2001/3003#newcode68
src/platform/crash/crash_reporter.cc:68:
MetricsLibrary::SendToChrome(std::string(kCrashCounterHistogram),
It will need it in the future though when determining whether or not to send
back crash diagnostics.  Adding a CHECK here.

On 2010/04/26 23:27:10, petkov wrote:
> Consider checking IsMetricsCollectionAllowed here -- then you don't need the
> is_metrics_collection_allowed callback to UserCollector.
>

http://codereview.chromium.org/1702008/diff/2001/3003#newcode88
src/platform/crash/crash_reporter.cc:88: if (!PrepareUncleanShutdownCheck()) {
On 2010/04/26 20:45:51, sosa wrote:
> Should this be part of the flagged if or up one level?  It seems like you
should
> prepare each time.

It's really meant to separate out (for testing) the ability to track user
crashes and kernel/unclean-unmount crashes.

http://codereview.chromium.org/1702008/diff/2001/3003#newcode93
src/platform/crash/crash_reporter.cc:93: } else if (FLAGS_clean_shutdown) {
On 2010/04/26 23:27:10, petkov wrote:
> Given the "return 0", you don't really need the "else" here. I'd suggest
either
> remove the return, or the else.
> 

Done.

http://codereview.chromium.org/1702008/diff/2001/3003#newcode97
src/platform/crash/crash_reporter.cc:97: CHECK(FLAGS_signal != -1) << "Signal
must be set";
On 2010/04/26 23:27:10, petkov wrote:
> Add a comment that this mode handles a specific user process crash.
> 

Done.

http://codereview.chromium.org/1702008/diff/2001/3004
File src/platform/crash/make_tests.sh (right):

http://codereview.chromium.org/1702008/diff/2001/3004#newcode3
src/platform/crash/make_tests.sh:3: # Copyright (c) 2009 The Chromium OS
Authors. All rights reserved.
On 2010/04/26 23:27:10, petkov wrote:
> 2010

Done.

http://codereview.chromium.org/1702008/diff/2001/3007
File src/platform/crash/system_logging_mock.cc (right):

http://codereview.chromium.org/1702008/diff/2001/3007#newcode7
src/platform/crash/system_logging_mock.cc:7: #include "base/string_util.h"
See elsewhere. 
On 2010/04/26 23:27:10, petkov wrote:
> Use <> instead of "" for base.
>

http://codereview.chromium.org/1702008/diff/2001/3008
File src/platform/crash/system_logging_mock.h (right):

http://codereview.chromium.org/1702008/diff/2001/3008#newcode8
src/platform/crash/system_logging_mock.h:8: #include "crash/system_logging.h"
On 2010/04/26 20:45:51, sosa wrote:
> #include <string>

Done.

http://codereview.chromium.org/1702008/diff/2001/3009
File src/platform/crash/user_collector.cc (right):

http://codereview.chromium.org/1702008/diff/2001/3009#newcode8
src/platform/crash/user_collector.cc:8: #include "base/file_util.h"
see earlier.
On 2010/04/26 23:27:10, petkov wrote:
> Use <> instead of "" for base/

http://codereview.chromium.org/1702008/diff/2001/3009#newcode13
src/platform/crash/user_collector.cc:13: const char kCorePatternFile[] =
"/proc/sys/kernel/core_pattern";
On 2010/04/26 23:27:10, petkov wrote:
> static?
> 
> Can you add a couple of words about what this file does?
> 

Done.

http://codereview.chromium.org/1702008/diff/2001/3009#newcode44
src/platform/crash/user_collector.cc:44: file_path_.c_str());
Does my comment above kCorePatternFile explain it better (particularly reading
the core man page)?

On 2010/04/26 20:45:51, sosa wrote:
> I don't understand this.

http://codereview.chromium.org/1702008/diff/2001/3010
File src/platform/crash/user_collector.h (right):

http://codereview.chromium.org/1702008/diff/2001/3010#newcode19
src/platform/crash/user_collector.h:19: bool
(*is_metrics_collection_allowed_)(),
I don't understand.

On 2010/04/26 20:45:51, sosa wrote:
> No _ in argument

http://codereview.chromium.org/1702008/diff/2001/3010#newcode21
src/platform/crash/user_collector.h:21: virtual ~UserCollector();
On 2010/04/26 20:45:51, sosa wrote:
> Add documentation to all public methods

Done.

http://codereview.chromium.org/1702008/diff/2001/3011
File src/platform/crash/user_collector_test.cc (right):

http://codereview.chromium.org/1702008/diff/2001/3011#newcode8
src/platform/crash/user_collector_test.cc:8: #include "base/file_util.h"
see above.
On 2010/04/26 23:27:10, petkov wrote:
> Use <> instead of "" for base/

http://codereview.chromium.org/1702008/diff/2001/3011#newcode15
src/platform/crash/user_collector_test.cc:15: const char kFilePath[] =
"/my/path";
On 2010/04/26 23:27:10, petkov wrote:
> static?

Done.

http://codereview.chromium.org/1702008/diff/2001/3014
File src/scripts/run_tests.sh (right):

http://codereview.chromium.org/1702008/diff/2001/3014#newcode47
src/scripts/run_tests.sh:47: cd -
On 2010/04/26 20:45:51, sosa wrote:
> You should define this before you declare your trap to it

Done.

[sosa, 2010-04-29 16:51:23]

Did you upload all the changes?  I noticed there is no new patch to
user_collect.c/h where changes are implied by your comments.

http://codereview.chromium.org/1702008/diff/2001/3010
File src/platform/crash/user_collector.h (right):

http://codereview.chromium.org/1702008/diff/2001/3010#newcode19
src/platform/crash/user_collector.h:19: bool
(*is_metrics_collection_allowed_)(),
The argument shouldn't end in a _.  

On 2010/04/29 02:19:38, kmixter1 wrote:
> I don't understand.
> 
> On 2010/04/26 20:45:51, sosa wrote:
> > No _ in argument
> 
>

http://codereview.chromium.org/1702008/diff/2001/3010#newcode21
src/platform/crash/user_collector.h:21: virtual ~UserCollector();
Did you upload it?

On 2010/04/29 02:19:38, kmixter1 wrote:
> On 2010/04/26 20:45:51, sosa wrote:
> > Add documentation to all public methods
> 
> Done.

[kmixter1, 2010-04-29 20:21:45]

You're right - it's uploaded now.

On Thu, Apr 29, 2010 at 9:51 AM,  <sosa@chromium.org> wrote:
> Did you upload all the changes?  I noticed there is no new patch to
> user_collect.c/h where changes are implied by your comments.
>
>
> http://codereview.chromium.org/1702008/diff/2001/3010
> File src/platform/crash/user_collector.h (right):
>
> http://codereview.chromium.org/1702008/diff/2001/3010#newcode19
> src/platform/crash/user_collector.h:19: bool
> (*is_metrics_collection_allowed_)(),
> The argument shouldn't end in a _.
>
> On 2010/04/29 02:19:38, kmixter1 wrote:
>>
>> I don't understand.
>
>> On 2010/04/26 20:45:51, sosa wrote:
>> > No _ in argument
>
>
>
> http://codereview.chromium.org/1702008/diff/2001/3010#newcode21
> src/platform/crash/user_collector.h:21: virtual ~UserCollector();
> Did you upload it?
>
> On 2010/04/29 02:19:38, kmixter1 wrote:
>>
>> On 2010/04/26 20:45:51, sosa wrote:
>> > Add documentation to all public methods
>
>> Done.
>
> http://codereview.chromium.org/1702008/show
>

[petkov, 2010-04-29 21:43:33]

mostly questions/comments, no real issues, so lgtm in respect to my review.

http://codereview.chromium.org/1702008/diff/2001/3003
File src/platform/crash/crash_reporter.cc (right):

http://codereview.chromium.org/1702008/diff/2001/3003#newcode6
src/platform/crash/crash_reporter.cc:6: #include <string>
On 2010/04/29 02:19:38, kmixter1 wrote:
> On 2010/04/26 23:27:10, petkov wrote:
> > I'm not entirely sure on style but <string> might need to be in a separate
> > group.
> 
> The style is supposed to be
> #include <stdincludes>
> 
> #include "project/thirdparty/etc/includes"
> 
> Unfortunately we mix those concepts in this project.  As a compromise, since
> metrics_library is a project specific library, I changed it to double quotes
and
> sorted it in the later list.

It seems to me that once a header file is installed in /usr/include/, it
basically becomes part of the standard include files rather than project
specific. I.e., you're not picking up the header file from the your source tree
but rather from the standard linux header file location...

Either way, it's a bit messy for us and this is probably not the CL to make
things right. So, nevermind.

http://codereview.chromium.org/1702008/diff/19001/20001
File src/platform/crash/Makefile (right):

http://codereview.chromium.org/1702008/diff/19001/20001#newcode18
src/platform/crash/Makefile:18: $(CXX) $(CXXFLAGS) $(LIB_DIRS) $^ $(LIBS) -o $@
I assume you get optimizations turned on in the default CXXFLAGS...

http://codereview.chromium.org/1702008/diff/19001/20002
File src/platform/crash/crash_reporter.cc (right):

http://codereview.chromium.org/1702008/diff/19001/20002#newcode22
src/platform/crash/crash_reporter.cc:22: static const char
kCrashCounterHistogram[] = "logging.CrashCounter";
FYI, the current histogram group names tend to be CamelCase (e.g., Logging).

http://codereview.chromium.org/1702008/diff/19001/20002#newcode26
src/platform/crash/crash_reporter.cc:26: // Enumeration of kinds of crashes to
be used in the CrashCounter histogram.
Thinking forward, if you log time between crashes, you could avoid an
enumeration histogram -- you could have one histogram for time between user
crashes and one histogram for time between kernel crashes. The sample count
would give you the number of crashes for each kind.

But having an explicit enumeration histogram would also do it...

http://codereview.chromium.org/1702008/diff/19001/20002#newcode36
src/platform/crash/crash_reporter.cc:36: // TODO(kmixter): Eventually check
system tainted state.
Do you actually need to check this? Wouldn't this be part of Chrome -- i.e., you
could still collect metrics but Chrome will not be sending them out if there's
no opt-in.

http://codereview.chromium.org/1702008/diff/19001/20012
File src/platform/init/crash-reporter.conf (right):

http://codereview.chromium.org/1702008/diff/19001/20012#newcode9
src/platform/init/crash-reporter.conf:9: start on stopped dump-boot-stats
You'd be initting every time the login screen is presented. I assume that's OK.

[sosa, 2010-04-29 22:49:44]

lgtm + nit

http://codereview.chromium.org/1702008/diff/19001/20009
File src/platform/crash/user_collector.h (right):

http://codereview.chromium.org/1702008/diff/19001/20009#newcode27
src/platform/crash/user_collector.h:27: 
Kill this extra line

[petkov, 2010-04-30 15:06:09]

http://codereview.chromium.org/1702008/diff/19001/20002
File src/platform/crash/crash_reporter.cc (right):

http://codereview.chromium.org/1702008/diff/19001/20002#newcode35
src/platform/crash/crash_reporter.cc:35: static bool
IsMetricsCollectionAllowed() {
Actually, I do have some concerns about this. Is this the check for
opt-in/opt-out? If so, it seems wasteful to do it in every package that
generates metrics. So, back to the original comment -- it would be best if the
check is done only in Chrome or, a bit worse, only in Chrome and the metrics
library.

If this is not the check for opt-in/opt-out, please ignore my concerns.

[kmixter1, 2010-04-30 23:18:52]

PTAL petkov

http://codereview.chromium.org/1702008/diff/19001/20001
File src/platform/crash/Makefile (right):

http://codereview.chromium.org/1702008/diff/19001/20001#newcode18
src/platform/crash/Makefile:18: $(CXX) $(CXXFLAGS) $(LIB_DIRS) $^ $(LIBS) -o $@
On 2010/04/29 21:43:33, petkov wrote:
> I assume you get optimizations turned on in the default CXXFLAGS...
> 

Yes.

http://codereview.chromium.org/1702008/diff/19001/20002
File src/platform/crash/crash_reporter.cc (right):

http://codereview.chromium.org/1702008/diff/19001/20002#newcode22
src/platform/crash/crash_reporter.cc:22: static const char
kCrashCounterHistogram[] = "logging.CrashCounter";
On 2010/04/29 21:43:33, petkov wrote:
> FYI, the current histogram group names tend to be CamelCase (e.g., Logging).
> 

Done.

http://codereview.chromium.org/1702008/diff/19001/20002#newcode26
src/platform/crash/crash_reporter.cc:26: // Enumeration of kinds of crashes to
be used in the CrashCounter histogram.
On 2010/04/29 21:43:33, petkov wrote:
> Thinking forward, if you log time between crashes, you could avoid an
> enumeration histogram -- you could have one histogram for time between user
> crashes and one histogram for time between kernel crashes. The sample count
> would give you the number of crashes for each kind.
> 
> But having an explicit enumeration histogram would also do it...
> 

It's a good idea.  I think I'll need to hook into your stuff to properly account
for "uptime" between crashes vs real time.  For now this prototypes it.

http://codereview.chromium.org/1702008/diff/19001/20002#newcode35
src/platform/crash/crash_reporter.cc:35: static bool
IsMetricsCollectionAllowed() {
On 2010/04/30 15:06:09, petkov wrote:
> Actually, I do have some concerns about this. Is this the check for
> opt-in/opt-out? If so, it seems wasteful to do it in every package that
> generates metrics. So, back to the original comment -- it would be best if the
> check is done only in Chrome or, a bit worse, only in Chrome and the metrics
> library.
> 
> If this is not the check for opt-in/opt-out, please ignore my concerns.
> 

See below - yep, we should do it in the metrics library and we should make it a
fast check.  Until R8 it's a noop.

http://codereview.chromium.org/1702008/diff/19001/20002#newcode36
src/platform/crash/crash_reporter.cc:36: // TODO(kmixter): Eventually check
system tainted state.
On 2010/04/29 21:43:33, petkov wrote:
> Do you actually need to check this? Wouldn't this be part of Chrome -- i.e.,
you
> could still collect metrics but Chrome will not be sending them out if there's
> no opt-in.
> 

Chrome will do it based on the current user, but the time of the stats
collection might not be attributable to any user.  I added more to the todo to
reflect that we want this functionality to live directly within metrics_library,
though still exported so that I can also use it for determining when to send
crash dumps and logs.

http://codereview.chromium.org/1702008/diff/19001/20009
File src/platform/crash/user_collector.h (right):

http://codereview.chromium.org/1702008/diff/19001/20009#newcode27
src/platform/crash/user_collector.h:27: 
On 2010/04/29 22:49:44, sosa wrote:
> Kill this extra line

Done.

http://codereview.chromium.org/1702008/diff/19001/20012
File src/platform/init/crash-reporter.conf (right):

http://codereview.chromium.org/1702008/diff/19001/20012#newcode9
src/platform/init/crash-reporter.conf:9: start on stopped dump-boot-stats
On 2010/04/29 21:43:33, petkov wrote:
> You'd be initting every time the login screen is presented. I assume that's
OK.
> 

Ugh.  It's no good as is.  This will cause it to think there was an unclean
shutdown every time you log out.  Good catch.  I've "fixed" it for now by but
more importantly have added an integration test for it.

[petkov, 2010-05-03 06:16:10]

LGTM

[petkov, 2010-05-04 16:21:55]

http://codereview.chromium.org/1702008/diff/18002/29004
File src/platform/crash/make_tests.sh (right):

http://codereview.chromium.org/1702008/diff/18002/29004#newcode11
src/platform/crash/make_tests.sh:11: make tests
This should signal build failure to the ebuild or build_tests.sh would think it
succeeded even if "make tests" failed.

So maybe add "set -e".