Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(741)

Issue 2128004: Adding in-process crash dumping library and crash uploader (Closed)

Created:
10 years, 7 months ago by kmixter1
Modified:
9 years, 7 months ago
Reviewers:
petkov, sosa
CC:
chromium-os-reviews_chromium.org
Base URL:
ssh://git@chromiumos-git//chromeos
Visibility:
Public.

Description

libcrash_dumper.so is created so any application can just link against it and call CrashDumper::Enable() to start collecting minidumps. Eventually crash_sender is run hourly to upload any enqueued minidumps. BUG=1513

Patch Set 1 #

Patch Set 2 : Improve documentation, clean up sender script #

Patch Set 3 : Only remove if successful send #

Patch Set 4 : Compiles #

Patch Set 5 : Update the Google Breakpad version #

Patch Set 6 : Passes test #

Patch Set 7 : remove some debugging #

Total comments: 42

Patch Set 8 : new and improved tests pass #

Patch Set 9 : respond to review #

Total comments: 16

Patch Set 10 : more nits #

Total comments: 7

Patch Set 11 : respond to petkov review #

Patch Set 12 : Use urandom #

Patch Set 13 : use 32b for random #

Patch Set 14 : respond to review and add another test #

Unified diffs Side-by-side diffs Delta from patch set Stats (+398 lines, -5 lines) Patch
M DEPS View 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M src/platform/crash/Makefile View 1 2 3 4 5 6 7 2 chunks +11 lines, -3 lines 0 comments Download
A src/platform/crash/crash_dumper.h View 1 2 3 4 5 6 7 8 1 chunk +46 lines, -0 lines 0 comments Download
A src/platform/crash/crash_dumper.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +127 lines, -0 lines 0 comments Download
M src/platform/crash/crash_reporter.cc View 4 5 1 chunk +2 lines, -0 lines 0 comments Download
A src/platform/crash/crash_sender View 6 7 8 9 10 11 12 1 chunk +198 lines, -0 lines 0 comments Download
A src/platform/crash/crash_sender.hourly View 6 7 1 chunk +11 lines, -0 lines 0 comments Download
M src/platform/crash/user_collector.cc View 4 5 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 13 (0 generated)
kmixter1
10 years, 7 months ago (2010-05-17 17:42:01 UTC) #1
sosa
Mostly nits http://codereview.chromium.org/2128004/diff/17001/9004 File src/platform/crash/crash_dumper.cc (right): http://codereview.chromium.org/2128004/diff/17001/9004#newcode18 src/platform/crash/crash_dumper.cc:18: const char kSystemCrashPath[] = "/var/spool/crash"; Will need ...
10 years, 7 months ago (2010-05-17 18:29:01 UTC) #2
petkov
Mostly nits. http://codereview.chromium.org/2128004/diff/17001/9003 File src/platform/crash/Makefile (right): http://codereview.chromium.org/2128004/diff/17001/9003#newcode37 src/platform/crash/Makefile:37: rm -rf *.o $(CRASH_BIN) $(TEST_BINS) Add CRASH_LIB ...
10 years, 7 months ago (2010-05-17 18:33:06 UTC) #3
kmixter1
PTAL http://codereview.chromium.org/2128004/diff/17001/9003 File src/platform/crash/Makefile (right): http://codereview.chromium.org/2128004/diff/17001/9003#newcode37 src/platform/crash/Makefile:37: rm -rf *.o $(CRASH_BIN) $(TEST_BINS) On 2010/05/17 18:33:06, ...
10 years, 7 months ago (2010-05-18 23:05:11 UTC) #4
sosa
Mostly nits http://codereview.chromium.org/2128004/diff/27001/15011 File src/platform/crash/crash_dumper.cc (right): http://codereview.chromium.org/2128004/diff/27001/15011#newcode27 src/platform/crash/crash_dumper.cc:27: Add a comment to why these are ...
10 years, 7 months ago (2010-05-19 00:38:33 UTC) #5
kmixter1
PTAL http://codereview.chromium.org/2128004/diff/27001/15011 File src/platform/crash/crash_dumper.cc (right): http://codereview.chromium.org/2128004/diff/27001/15011#newcode27 src/platform/crash/crash_dumper.cc:27: On 2010/05/19 00:38:33, sosa wrote: > Add a ...
10 years, 7 months ago (2010-05-19 03:15:40 UTC) #6
petkov
One doc issue. Otherwise, LGTM in regard to my comments. http://codereview.chromium.org/2128004/diff/35001/36004 File src/platform/crash/crash_dumper.h (right): http://codereview.chromium.org/2128004/diff/35001/36004#newcode34 ...
10 years, 7 months ago (2010-05-19 05:11:54 UTC) #7
kmixter1
http://codereview.chromium.org/2128004/diff/35001/36004 File src/platform/crash/crash_dumper.h (right): http://codereview.chromium.org/2128004/diff/35001/36004#newcode34 src/platform/crash/crash_dumper.h:34: // Enable crash detection and dumping. Aborts if already ...
10 years, 7 months ago (2010-05-19 08:24:20 UTC) #8
sosa
learned something today about static const pointer business, thanks. LGTM
10 years, 7 months ago (2010-05-19 15:31:59 UTC) #9
petkov
still lgtm... http://codereview.chromium.org/2128004/diff/35001/36006 File src/platform/crash/crash_sender (right): http://codereview.chromium.org/2128004/diff/35001/36006#newcode74 src/platform/crash/crash_sender:74: local random=$((0x$(openssl rand -hex 4))) On 2010/05/19 ...
10 years, 7 months ago (2010-05-19 15:39:21 UTC) #10
sosa
Mostly nits, quick question: it seems like you're doing unit testing using autotest, should we ...
10 years, 7 months ago (2010-05-19 16:31:50 UTC) #11
sosa
My bad wrong CL. I have too many code review tabs open. On Wed, May ...
10 years, 7 months ago (2010-05-19 16:45:40 UTC) #12
kmixter1
10 years, 7 months ago (2010-05-19 17:19:40 UTC) #13
On 2010/05/19 16:31:50, sosa wrote:
> Mostly nits, quick question:  it seems like you're doing unit testing using
> autotest, should we encourage this?

I don't think we should encourage or prohibit it.  If it's a test and runs
automatically and quickly, I don't personally mind it being in autotest or unit
test.  In this case I'm already unit testing crash_reporter, but the
crash_sender script I feel is better to test on the system itself and not in
isolation as a unit test.  This doesn't give 100% coverage because I'm never
actually sending a crash report, but that very last step I think we can rely on
manual testing for.

Powered by Google App Engine
This is Rietveld 408576698