Issue 2128683002: Collect unclean shutdown debug information

manzagop (departed)

Patchset #1 (id:1) has been deleted

4 years, 5 months ago (2016-07-06 21:09:50 UTC) #1

manzagop (departed)

Description was changed from ========== Introduce PostmortemReportCollector to gain visibility into unclean shutdowns. BUG= ========== ...

4 years, 4 months ago (2016-08-03 19:36:59 UTC) #7

manzagop (departed)

manzagop@chromium.org changed reviewers: + bcwhite@chromium.org, scottmg@chromium.org

4 years, 4 months ago (2016-08-03 21:45:24 UTC) #8

manzagop (departed)

Hi bcwhite@, scottmg@, Could you have a look? Thanks, Pierre

4 years, 4 months ago (2016-08-03 21:45:25 UTC) #9

scottmg

Looks good, haven't read through postmortem_unittest.cc yet. https://codereview.chromium.org/2128683002/diff/200001/components/browser_watcher/BUILD.gn File components/browser_watcher/BUILD.gn (right): https://codereview.chromium.org/2128683002/diff/200001/components/browser_watcher/BUILD.gn#newcode5 components/browser_watcher/BUILD.gn:5: import("//third_party/protobuf/proto_library.gni") So ...

4 years, 4 months ago (2016-08-03 22:47:06 UTC) #10

bcwhite

lgtm https://codereview.chromium.org/2128683002/diff/200001/base/debug/activity_analyzer.cc File base/debug/activity_analyzer.cc (right): https://codereview.chromium.org/2128683002/diff/200001/base/debug/activity_analyzer.cc#newcode7 base/debug/activity_analyzer.cc:7: #include <utility> Why this? https://codereview.chromium.org/2128683002/diff/200001/components/browser_watcher/postmortem.cc File components/browser_watcher/postmortem.cc (right): ...

4 years, 4 months ago (2016-08-04 13:38:11 UTC) #11

manzagop (departed)

Description was changed from ========== Collect unclean shutdown debug information Introduces PostmortemReportCollector which can harvest ...

4 years, 4 months ago (2016-08-10 15:50:38 UTC) #12

manzagop (departed)

Thanks for the review! The new patch registers the minidump with crashpad and adds a ...

4 years, 4 months ago (2016-08-10 15:59:52 UTC) #13

manzagop (departed)

Description was changed from ========== Collect unclean shutdown debug information Introduces PostmortemReportCollector which can harvest ...

4 years, 4 months ago (2016-08-10 16:03:22 UTC) #14

bcwhite

https://codereview.chromium.org/2128683002/diff/240001/components/browser_watcher/postmortem_minidump_writer_win.cc File components/browser_watcher/postmortem_minidump_writer_win.cc (right): https://codereview.chromium.org/2128683002/diff/240001/components/browser_watcher/postmortem_minidump_writer_win.cc#newcode43 components/browser_watcher/postmortem_minidump_writer_win.cc:43: minidump_file_ = minidump_file; Does |minidump_file_| exist just to avoid ...

4 years, 4 months ago (2016-08-10 16:59:53 UTC) #15

manzagop (departed)

Addressed comment! https://codereview.chromium.org/2128683002/diff/240001/components/browser_watcher/postmortem_minidump_writer_win.cc File components/browser_watcher/postmortem_minidump_writer_win.cc (right): https://codereview.chromium.org/2128683002/diff/240001/components/browser_watcher/postmortem_minidump_writer_win.cc#newcode43 components/browser_watcher/postmortem_minidump_writer_win.cc:43: minidump_file_ = minidump_file; On 2016/08/10 16:59:52, bcwhite ...

4 years, 4 months ago (2016-08-11 13:40:47 UTC) #18

manzagop (departed)

manzagop@chromium.org changed reviewers: + siggi@chromium.org

4 years, 4 months ago (2016-08-11 13:42:35 UTC) #19

manzagop (departed)

Hi siggi@, Can you have a look? Thanks, Pierre

4 years, 4 months ago (2016-08-11 13:42:36 UTC) #20

Sigurður Ásgeirsson

sweet - first batch of comments. https://codereview.chromium.org/2128683002/diff/320001/components/browser_watcher/postmortem_minidump_writer.h File components/browser_watcher/postmortem_minidump_writer.h (right): https://codereview.chromium.org/2128683002/diff/320001/components/browser_watcher/postmortem_minidump_writer.h#newcode14 components/browser_watcher/postmortem_minidump_writer.h:14: #include "base/files/scoped_file.h" doesn't ...

4 years, 4 months ago (2016-08-11 17:44:15 UTC) #21

manzagop (departed)

Addressed most comments. I still need to: - move the code to a better home, ...

4 years, 4 months ago (2016-08-12 21:23:23 UTC) #22

Addressed most comments. 

I still need to:
- move the code to a better home, since this will live in the browser
- add uma

Also asking a question about how to get product/version.

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
File components/browser_watcher/postmortem_minidump_writer.h (right):

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/postmortem_minidump_writer.h:14: #include
"base/files/scoped_file.h"
On 2016/08/11 17:44:14, Sigurður Ásgeirsson wrote:
> doesn't look like you need this include?

Done.

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/postmortem_minidump_writer.h:29: using Position =
uint32_t;
On 2016/08/11 17:44:14, Sigurður Ásgeirsson wrote:
> I assume there's a constraint on this type due to it's use in minidump
records?
> Maybe explain why this can't be the same type as fseek takes?

Right, it's used to assign to an RVA. I added a comment.

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/postmortem_minidump_writer.h:33: explicit
PostmortemMinidumpWriter(FILE* minidump_file);
On 2016/08/11 17:44:14, Sigurður Ásgeirsson wrote:
> are there other constraints on minidump_file - I think you may require and/or
> expect it to be empty?

Hm, I'm not sure how to check this from a FILE* to a binary stream (can't seek
to SEEK_END). 

Only added a comment.

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/postmortem_minidump_writer.h:61: bool Write(Position
pos, const DataType& data);
On 2016/08/11 17:44:14, Sigurður Ásgeirsson wrote:
> looks to me this function is used once - maybe better never to implement it?

It's also used in the (templated) Append.

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/postmortem_minidump_writer.h:71: bool
AppendUtf8String(const std::string data, Position* pos);
On 2016/08/11 17:44:14, Sigurður Ásgeirsson wrote:
> string& or StringPiece?

Went with StringPiece. Done.

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/postmortem_minidump_writer.h:78: Position cursor_;
On 2016/08/11 17:44:14, Sigurður Ásgeirsson wrote:
> good comment, but cursor_ has connotations to a write position to my mind. 
> Maybe next_available_byte_ or some such?

Done.

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/postmortem_minidump_writer.h:80: // Structure for the
directory.
On 2016/08/11 17:44:14, Sigurður Ásgeirsson wrote:
> storage for the directory during writes?

Done.

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
File components/browser_watcher/postmortem_minidump_writer_win.cc (right):

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/postmortem_minidump_writer_win.cc:33: : cursor_(0U),
minidump_file_(minidump_file) {}
On 2016/08/11 17:44:14, Sigurður Ásgeirsson wrote:
> DCHECK or even CHECK minidump_file here?

Done.

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/postmortem_minidump_writer_win.cc:50:
report.SerializeToString(&serialized_report);
On 2016/08/11 17:44:14, Sigurður Ásgeirsson wrote:
> Looks like this can fail - check the return value.

Huh. Looks like it's when messages are too large. Done.

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/postmortem_minidump_writer_win.cc:67: //
TODO(manzagop): use product and version from the stability report. The
On 2016/08/11 17:44:14, Sigurður Ásgeirsson wrote:
> I think you want to pass the product and version into this function, and you'd
> want to use the base product from crash_reporter::CrashReporterClient, I
think?

Now passing in product and version.

Is the version_info component not correct? crash_reporter::CrashReporterClient
seems to have different signatures depending on the platform and requires a
::GetModuleFileName?

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/postmortem_minidump_writer_win.cc:70: {"product",
version_info::GetProductName() + "_Postmortem"},
On 2016/08/11 17:44:14, Sigurður Ásgeirsson wrote:
> Is this crash team's recommended way to disambiguate? Note that you'll need to
> register any new product with the crash backend before you can collect
anything.

Yes, this is what we discussed.

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/postmortem_minidump_writer_win.cc:81: MINIDUMP_HEADER
header;
On 2016/08/11 17:44:15, Sigurður Ásgeirsson wrote:
> so the module information is solely in the protobuf? I guess this implies
> there'll be some duplication for "real" crash reports? Will this make it
harder
> to reuse existing symbolization code, over recording the module information in
> the minidump module information stream?

I've placed a TODO in the .h to revisit where this information should be kept.

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/postmortem_minidump_writer_win.cc:113:
MINIDUMP_LOCATION_DESCRIPTOR simple_annotations;
On 2016/08/11 17:44:15, Sigurður Ásgeirsson wrote:
> a simpler way to do this would be to subtract dict_pos from cursor_?

Totally! Done.

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/postmortem_minidump_writer_win.cc:120:
simple_annotations.Rva = dict_pos;
On 2016/08/11 17:44:14, Sigurður Ásgeirsson wrote:
> do you want a non-null RVA for an empty list?

An RVA of 0 signals the absence of a dictionary. Adding a comment.

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/postmortem_minidump_writer_win.cc:135:
MINIDUMP_DIRECTORY crashpad_directory = {0};
On 2016/08/11 17:44:14, Sigurður Ásgeirsson wrote:
> maybe pull the directory extension code out to a utility function?

Done.

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/postmortem_minidump_writer_win.cc:175:
DCHECK_NE(base::kInvalidPlatformFile, minidump_file_);
On 2016/08/11 17:44:15, Sigurður Ásgeirsson wrote:
> I don't think this is right.

Done.

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/postmortem_minidump_writer_win.cc:183: // Seek and
write.
On 2016/08/11 17:44:14, Sigurður Ásgeirsson wrote:
> ISTR there's a very non-trivial performance hit to seeking in the MSVC CRT,
even
> when the seek is a noop. This adds up, and this may be running at a ...
> difficult ... time.
> Maybe you can maintain the invariant that the file position is always at the
end
> of file, except for the directory write. This is probably best done with a
> DCHECK(IsPositionAtEnd()) or some such, wherever you write to the file.

Good to know. I've made seeking explicit. Done.

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/postmortem_minidump_writer_win.cc:189: bool
PostmortemMinidumpWriter::AppendUtf8String(const std::string data,
On 2016/08/11 17:44:15, Sigurður Ásgeirsson wrote:
> string&?

Changed to StringPiece.

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
File components/browser_watcher/postmortem_minidump_writer_win_unittest.cc
(right):

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/postmortem_minidump_writer_win_unittest.cc:46:
PostmortemMinidumpWriter writer(file.get());
On 2016/08/11 17:44:15, Sigurður Ásgeirsson wrote:
> nit: I'd separate the actual writing of the report out to a separate function,
> and call it from the test. I'd leave the common setup here, though.

Done.

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/postmortem_minidump_writer_win_unittest.cc:47:
ASSERT_TRUE(writer.WriteDump(expected_report_, expected_client_id_,
On 2016/08/11 17:44:15, Sigurður Ásgeirsson wrote:
> it's awkward that this function derives some of the file contents from the
> environment, another argument for passing it in, or using setters, or
something?

Assuming you mean product/version. Done.

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/postmortem_minidump_writer_win_unittest.cc:83:
std::string recovered_serialized_report;
On 2016/08/11 17:44:15, Sigurður Ásgeirsson wrote:
> if there's a way to get at the serialized report with CrashPad, I think it'd
be
> preferable to ditch this test and do everything with CP.
> Otherwise maybe document why this test is doing things explicitly?

Documented and linked a bug. Done.

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
File components/browser_watcher/postmortem_report_collector.cc (right):

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/postmortem_report_collector.cc:78: FileDeleter
debug_file_deleter(file);
On 2016/08/11 17:44:15, Sigurður Ásgeirsson wrote:
> The intent here seems to be to make at most one reporting attempt per found
> file.
> However, there's a problem with "stuck" files, such as files where for
whatever
> reason this process doesn't have delete permission, or the file's already open
> by someone else, etc.
> 
> In order to avoid reporting a "stuck" file for perpetuity, I'd condition the
> report on being able to delete the file. That way a "stuck" file is never
> reported.
> I think under-reporting is the lesser evil there?

Good point. Done.

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
File components/browser_watcher/postmortem_report_collector.h (right):

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/postmortem_report_collector.h:29: // TODO(manzagop):
UMA metrics and some error logging.
On 2016/08/11 17:44:15, Sigurður Ásgeirsson wrote:
> as we discussed earlier today, it'd be better^Weasier to do this from inside
the
> browser process for now. That way you have the benefit of UMA and finch
without
> trouble, and if you do this from e.g. within the metrics provider, you'll be
> running fairly early in the process lifetime, so stability should be fine.

Sgtm. Let's include moving the code and this TODO as part of this CL.

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/postmortem_report_collector.h:57: // Virtual for
unittesting.
On 2016/08/11 17:44:15, Sigurður Ásgeirsson wrote:
> I do hope there's a test for this method, as it generates file names that are
> later deleted. It'd be ... awkward .. if there's an error in this method that
> fingers the wrong files :O.

Well... there is a test!

The reason: I was thinking of using unique debug state files (e.g.
DebugState-<PID>-<TID>-<creation-time>.pma). 

Brian suggests it would be simpler/sufficient to work with only 2 file names:
"current" and "previous". This would overwrite any debug state that failed to be
registered to crashpad by the subsequent browser run. I'm coming to agree. WDYT?

WDYT?

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/postmortem_report_collector.h:71: base::FilePath
debug_state_dir_;
On 2016/08/11 17:44:15, Sigurður Ásgeirsson wrote:
> there doesn't seem to be any benefit to the debug_* member variables that I
can
> see?

Agreed, removed. The class is now an empty husk. I've kept the functions
non-static as it enables use of gmock.

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
File components/browser_watcher/stability_report.proto (right):

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/stability_report.proto:61: // A stability report
contains information relevant to Chrome's execution at a
On 2016/08/11 17:44:15, Sigurður Ásgeirsson wrote:
> I take this is intended to refer to a single logical instance of a "chrome
> browser", e.g.  a process or a tree of processes that all are subordinates to
a
> single browser?

Correct. Clarified comment.

https://codereview.chromium.org/2128683002/diff/320001/components/browser_wat...
components/browser_watcher/stability_report.proto:65: repeated ProcessState
process_states = 2;
On 2016/08/11 17:44:15, Sigurður Ásgeirsson wrote:
> I think if you want this to be useful for more than a single process at a
time,
> we need some indication of the type of each process and/or a hierarchical
> relationship for the processes?
> If that's a for later, maybe add a comment that this is currently intended
only
> for a single process, and/or a todo for extending the process state?
> 
> Things that'd be interesting to add to a process might include (chrome)
process
> type, start time, pid, parent pid, command line and so forth.
> 
> Also for chrome we specifically want experiment state, which I believe is a
set
> of {uint32_t, uint32_t } or some such. This probably wants to be a TODO,
though,
> as this implies more widely reaching changes?

Totally! This file is only a placeholder at this point. I plan to revise it in a
subsequent CL that fleshes out the contents of the stability report. There must
be no serialized version of it until that point.

I've sprinkled a few TODOs.

manzagop (departed)

Description was changed from ========== Collect unclean shutdown debug information Introduces PostmortemReportCollector which harvests unclean ...

4 years, 4 months ago (2016-08-22 15:11:42 UTC) #23

manzagop (departed)

Description was changed from ========== Collect unclean shutdown debug information Introduces PostmortemReportCollector which harvests unclean ...

4 years, 4 months ago (2016-08-22 15:12:40 UTC) #24

manzagop (departed)

Description was changed from ========== Collect unclean shutdown debug information Introduces PostmortemReportCollector which harvests unclean ...

4 years, 4 months ago (2016-08-24 19:51:47 UTC) #25

Description was changed from

==========
Collect unclean shutdown debug information

Introduces PostmortemReportCollector which harvests unclean shutdown debug
information to a protocol buffer, wraps this proto inside a barebone minidump
and registers the minidump with CrashPad for upload. 

Notes:
- This is experimental and therefore directed at canary only (via Chrome
experiment).
- CrashPad handles consent checking/throttling.
- Manufacturing the minidump is a temporary approach. Longer term, Crashpad
should handle this.
- This first version collects reports for crashes. Longer term, the reports
pertaining to crashes will be bundled with the crash reports.

TODOs for subsequent CL:
- UMA instrumentation
- fleshing out the contents of the stability report

BUG=620813
==========

to

==========
Collect unclean shutdown debug information

Introduces PostmortemReportCollector which harvests unclean shutdown debug
information to a protocol buffer, wraps this proto inside a barebone minidump
and registers the minidump with CrashPad for upload. 

Notes:
- This is experimental work and therefore currently only targets the windows
canary channel (using an experiment).
- PostmortemReportCollector is used in WatcherMetricProvider's
ProvideInitialStabilityMetrics, which enables it to log metrics against the
previous version of chrome (the postmortem report count), as well as metrics
about the collection against the current version. This does place collection on
the critical startup path, which is deemed acceptable if there is negligible
cost when there was no unclean shutdown and small if there was. A metric is
introduced to measure this. 
- CrashPad handles consent checking/throttling.
- Manufacturing the minidump is a temporary approach. Longer term, Crashpad
should handle this.
- This first version collects reports for crashes. Longer term, the reports
pertaining to crashes will be bundled with the crash reports.
- Stability file naming is revised so a unique file name is used every time.

TODOs for subsequent CL:
- fleshing out the contents of the stability report

TEST
- launch chrome with the stability instrumentation
(--enable-features=StabilityDebugging)
- crash chrome by visiting chrome://chrome://inducebrowsercrashforrealz
- Validate the existence of a stability file in <user-data-dir>/Stability
- start chrome again
- validate metrics chrome://histograms/ActivityTracker
- validate the presence of a new crash at chrome://crashes 

BUG=620813
==========

manzagop (departed)

Description was changed from ========== Collect unclean shutdown debug information Introduces PostmortemReportCollector which harvests unclean ...

4 years, 4 months ago (2016-08-24 19:52:36 UTC) #26

Description was changed from

==========
Collect unclean shutdown debug information

Introduces PostmortemReportCollector which harvests unclean shutdown debug
information to a protocol buffer, wraps this proto inside a barebone minidump
and registers the minidump with CrashPad for upload. 

Notes:
- This is experimental work and therefore currently only targets the windows
canary channel (using an experiment).
- PostmortemReportCollector is used in WatcherMetricProvider's
ProvideInitialStabilityMetrics, which enables it to log metrics against the
previous version of chrome (the postmortem report count), as well as metrics
about the collection against the current version. This does place collection on
the critical startup path, which is deemed acceptable if there is negligible
cost when there was no unclean shutdown and small if there was. A metric is
introduced to measure this. 
- CrashPad handles consent checking/throttling.
- Manufacturing the minidump is a temporary approach. Longer term, Crashpad
should handle this.
- This first version collects reports for crashes. Longer term, the reports
pertaining to crashes will be bundled with the crash reports.
- Stability file naming is revised so a unique file name is used every time.

TODOs for subsequent CL:
- fleshing out the contents of the stability report

TEST
- launch chrome with the stability instrumentation
(--enable-features=StabilityDebugging)
- crash chrome by visiting chrome://chrome://inducebrowsercrashforrealz
- Validate the existence of a stability file in <user-data-dir>/Stability
- start chrome again
- validate metrics chrome://histograms/ActivityTracker
- validate the presence of a new crash at chrome://crashes 

BUG=620813
==========

to

==========
Collect unclean shutdown debug information

Introduces PostmortemReportCollector which harvests unclean shutdown debug
information to a protocol buffer, wraps this proto inside a barebone minidump
and registers the minidump with CrashPad for upload. 

Notes:
- This is experimental work that currently only targets the windows canary
channel (using an experiment).
- PostmortemReportCollector is used in WatcherMetricProvider's
ProvideInitialStabilityMetrics, which enables it to log metrics against the
previous version of chrome (the postmortem report count), as well as metrics
about the collection against the current version. This does place collection on
the critical startup path, which is deemed acceptable if there is negligible
cost when there was no unclean shutdown and small if there was. A metric is
introduced to measure this. 
- CrashPad handles consent checking/throttling.
- Manufacturing the minidump is a temporary approach. Longer term, Crashpad
should handle this.
- This first version collects reports for crashes. Longer term, the reports
pertaining to crashes will be bundled with the crash reports.
- Stability file naming is revised so a unique file name is used every time.

TODOs for subsequent CL:
- fleshing out the contents of the stability report

TEST
- launch chrome with the stability instrumentation
(--enable-features=StabilityDebugging)
- crash chrome by visiting chrome://chrome://inducebrowsercrashforrealz
- Validate the existence of a stability file in <user-data-dir>/Stability
- start chrome again
- validate metrics chrome://histograms/ActivityTracker
- validate the presence of a new crash at chrome://crashes 

BUG=620813
==========

manzagop (departed)

Description was changed from ========== Collect unclean shutdown debug information Introduces PostmortemReportCollector which harvests unclean ...

4 years, 3 months ago (2016-09-02 18:02:08 UTC) #28

Description was changed from

==========
Collect unclean shutdown debug information

Introduces PostmortemReportCollector which harvests unclean shutdown debug
information to a protocol buffer, wraps this proto inside a barebone minidump
and registers the minidump with CrashPad for upload. 

Notes:
- This is experimental work that currently only targets the windows canary
channel (using an experiment).
- PostmortemReportCollector is used in WatcherMetricProvider's
ProvideInitialStabilityMetrics, which enables it to log metrics against the
previous version of chrome (the postmortem report count), as well as metrics
about the collection against the current version. This does place collection on
the critical startup path, which is deemed acceptable if there is negligible
cost when there was no unclean shutdown and small if there was. A metric is
introduced to measure this. 
- CrashPad handles consent checking/throttling.
- Manufacturing the minidump is a temporary approach. Longer term, Crashpad
should handle this.
- This first version collects reports for crashes. Longer term, the reports
pertaining to crashes will be bundled with the crash reports.
- Stability file naming is revised so a unique file name is used every time.

TODOs for subsequent CL:
- fleshing out the contents of the stability report

TEST
- launch chrome with the stability instrumentation
(--enable-features=StabilityDebugging)
- crash chrome by visiting chrome://chrome://inducebrowsercrashforrealz
- Validate the existence of a stability file in <user-data-dir>/Stability
- start chrome again
- validate metrics chrome://histograms/ActivityTracker
- validate the presence of a new crash at chrome://crashes 

BUG=620813
==========

to

==========
Collect unclean shutdown debug information

Introduces PostmortemReportCollector which harvests unclean shutdown debug
information to a protocol buffer, wraps this proto inside a barebone minidump
and registers the minidump with CrashPad for upload. 

Notes:
- This is experimental work that currently only targets the windows canary
channel (using an experiment).
- Reports are collected using a MetricsProvider. This does place collection on
the critical startup path, which is deemed acceptable if there is negligible
cost when there was no unclean shutdown and small if there was. A metric is
introduced to measure this. 
- CrashPad handles consent checking/uploading/throttling.
- Manufacturing the minidump is a temporary approach. Longer term, Crashpad
should handle this.
- This first version collects reports for crashes. Longer term, the reports
pertaining to crashes will be bundled with the crash reports.
- Stability file naming is revised so a unique file name is used every time.

TODOs for subsequent CL:
- fleshing out the contents of the stability report
- avoid reports from clean exits on the fast exit path

TEST
- launch chrome with the stability instrumentation
(--enable-features=StabilityDebugging)
- crash chrome by visiting chrome://chrome://inducebrowsercrashforrealz
- Validate the existence of a stability file in <user-data-dir>/Stability
- start chrome again
- validate "ActivityTracker.Collect.*" metrics
chrome://histograms/ActivityTracker
- validate the presence of a new crash at chrome://crashes 

BUG=620813
==========

manzagop (departed)

Description was changed from ========== Collect unclean shutdown debug information Introduces PostmortemReportCollector which harvests unclean ...

4 years, 3 months ago (2016-09-02 18:05:39 UTC) #29

Description was changed from

==========
Collect unclean shutdown debug information

Introduces PostmortemReportCollector which harvests unclean shutdown debug
information to a protocol buffer, wraps this proto inside a barebone minidump
and registers the minidump with CrashPad for upload. 

Notes:
- This is experimental work that currently only targets the windows canary
channel (using an experiment).
- Reports are collected using a MetricsProvider. This does place collection on
the critical startup path, which is deemed acceptable if there is negligible
cost when there was no unclean shutdown and small if there was. A metric is
introduced to measure this. 
- CrashPad handles consent checking/uploading/throttling.
- Manufacturing the minidump is a temporary approach. Longer term, Crashpad
should handle this.
- This first version collects reports for crashes. Longer term, the reports
pertaining to crashes will be bundled with the crash reports.
- Stability file naming is revised so a unique file name is used every time.

TODOs for subsequent CL:
- fleshing out the contents of the stability report
- avoid reports from clean exits on the fast exit path

TEST
- launch chrome with the stability instrumentation
(--enable-features=StabilityDebugging)
- crash chrome by visiting chrome://chrome://inducebrowsercrashforrealz
- Validate the existence of a stability file in <user-data-dir>/Stability
- start chrome again
- validate "ActivityTracker.Collect.*" metrics
chrome://histograms/ActivityTracker
- validate the presence of a new crash at chrome://crashes 

BUG=620813
==========

to

==========
Collect unclean shutdown debug information

Introduces PostmortemReportCollector which harvests unclean shutdown debug
information to a protocol buffer, wraps this proto inside a barebone minidump
and registers the minidump with CrashPad for upload. 

Notes:
- This is experimental work that currently only targets the windows canary
channel (using an experiment).
- Reports are collected using a MetricsProvider. This does place collection on
the critical startup path if an initial report is to be sent, which is deemed
since this should only occur after an unclean shutdown or if a metrics provider
needs it. A metric is introduced to measure collection duration. 
- CrashPad handles consent checking/uploading/throttling.
- Manufacturing the minidump is a temporary approach. Longer term, Crashpad
should handle this.
- This first version collects reports for crashes. Longer term, the reports
pertaining to crashes will be bundled with the crash reports.
- Stability file naming is revised so a unique file name is used every time.

TODOs for subsequent CL:
- fleshing out the contents of the stability report
- avoid reports from clean exits on the fast exit path

TEST
- launch chrome with the stability instrumentation
(--enable-features=StabilityDebugging)
- crash chrome by visiting chrome://chrome://inducebrowsercrashforrealz
- Validate the existence of a stability file in <user-data-dir>/Stability
- start chrome again
- validate "ActivityTracker.Collect.*" metrics
chrome://histograms/ActivityTracker
- validate the presence of a new crash at chrome://crashes 

BUG=620813
==========

manzagop (departed)

Short update: things seem to be working but I need to make some changes to ...

4 years, 3 months ago (2016-09-02 21:08:44 UTC) #30

manzagop (departed)

Description was changed from ========== Collect unclean shutdown debug information Introduces PostmortemReportCollector which harvests unclean ...

4 years, 3 months ago (2016-09-07 20:45:06 UTC) #31

Description was changed from

==========
Collect unclean shutdown debug information

Introduces PostmortemReportCollector which harvests unclean shutdown debug
information to a protocol buffer, wraps this proto inside a barebone minidump
and registers the minidump with CrashPad for upload. 

Notes:
- This is experimental work that currently only targets the windows canary
channel (using an experiment).
- Reports are collected using a MetricsProvider. This does place collection on
the critical startup path if an initial report is to be sent, which is deemed
since this should only occur after an unclean shutdown or if a metrics provider
needs it. A metric is introduced to measure collection duration. 
- CrashPad handles consent checking/uploading/throttling.
- Manufacturing the minidump is a temporary approach. Longer term, Crashpad
should handle this.
- This first version collects reports for crashes. Longer term, the reports
pertaining to crashes will be bundled with the crash reports.
- Stability file naming is revised so a unique file name is used every time.

TODOs for subsequent CL:
- fleshing out the contents of the stability report
- avoid reports from clean exits on the fast exit path

TEST
- launch chrome with the stability instrumentation
(--enable-features=StabilityDebugging)
- crash chrome by visiting chrome://chrome://inducebrowsercrashforrealz
- Validate the existence of a stability file in <user-data-dir>/Stability
- start chrome again
- validate "ActivityTracker.Collect.*" metrics
chrome://histograms/ActivityTracker
- validate the presence of a new crash at chrome://crashes 

BUG=620813
==========

to

==========
Collect unclean shutdown debug information

Introduces PostmortemReportCollector which harvests unclean shutdown debug
information to a protocol buffer, wraps this proto inside a barebone minidump
and registers the minidump with CrashPad for upload.

Notes:
- This is experimental work that currently only targets the windows canary
channel (using an experiment).
- Reports are collected as part of the metrics service client initialization
task chain. 
- CrashPad handles consent checking/uploading/throttling.
- Manufacturing the minidump is a temporary approach. Longer term, Crashpad
should handle this.
- This first version also collects reports for crashes. Longer term, the reports
pertaining to crashes will be bundled with the crash reports.
- Stability file naming is revised so a unique file name is used every time.

TODOs for subsequent CL:
- fleshing out the contents of the stability report
- avoid reports from clean exits on the fast exit path
- fix report collection stats version attribution on upgrade

TEST
- launch chrome with the stability instrumentation
(--enable-features=StabilityDebugging)
- crash chrome by visiting chrome://chrome://inducebrowsercrashforrealz
- Validate the existence of a stability file in <user-data-dir>/Stability
- start chrome again
- validate "ActivityTracker.Collect.*" metrics
chrome://histograms/ActivityTracker
- validate the presence of a new crash at chrome://crashes 

BUG=620813
==========

Sigurður Ásgeirsson

This CL looks pretty large and unwieldy, do you mind breaking it up? I'd suggest ...

4 years, 3 months ago (2016-09-08 15:38:40 UTC) #33

manzagop (departed)

Totatlly agree, sgtm! On Thu, Sep 8, 2016 at 11:38 AM, <siggi@chromium.org> wrote: > This ...

4 years, 3 months ago (2016-09-08 21:02:10 UTC) #34

bcwhite

LGTM for ActivityTracker interface. Looking forward to seeing this in the field!

4 years, 3 months ago (2016-09-16 16:47:51 UTC) #35

manzagop (departed)

4 years, 3 months ago (2016-09-22 18:14:17 UTC) #36

Closing this issue as it was broken up into:
- minidump writer https://codereview.chromium.org/2327043002/
- minidump dumper https://codereview.chromium.org/2339873003/
- report collector https://codereview.chromium.org/2339193003/
- wire it in https://codereview.chromium.org/2344343002/

Issue 2128683002: Collect unclean shutdown debug information (Closed)

Description

Patch Set 1 #

Patch Set 2 : StabilityReport now contains module information. #

Patch Set 3 : Unit testing #

Patch Set 4 : Create minidump #

Patch Set 5 : Minimal collection to proto #

Patch Set 6 : Switch from ReporterDelegate to CrashReportDatabase #

Patch Set 7 : Add CrashpadInfo to minidump #

Patch Set 8 : Address bcwhite comments. #

Patch Set 9 : Merge #

Patch Set 10 : Address Siggi's comments #

Patch Set 11 : Revise stability file naming and wire in the collection #

Patch Set 12 : Merge #

Patch Set 13 : Merge #

Patch Set 14 : Revise changes to metrics provider #

Patch Set 15 : Rewire collection as a metrics init task #

Patch Set 16 : Merge #

Messages