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

Issue 2767193002: Postmortem report collection: validate internal state (Closed)

Created:
3 years, 9 months ago by manzagop (departed)
Modified:
3 years, 8 months ago
Reviewers:
bcwhite, Steven Holte
CC:
chromium-reviews, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Postmortem report collection: validate internal state This change introduces checking of the underlying allocator's state (initialized, deleted, corrupt, valid). Stability files are no longer deleted on clean exit but instead marked as deleted. BUG=691595 Review-Url: https://codereview.chromium.org/2767193002 Cr-Commit-Position: refs/heads/master@{#466561} Committed: https://chromium.googlesource.com/chromium/src/+/0793c6c441b24c31bf2c890d243b1875a51abe80

Patch Set 1 #

Total comments: 13

Patch Set 2 : Address comments #

Patch Set 3 : Merge #

Patch Set 4 : Introduce MarkDeleted on tracker #

Patch Set 5 : Address comments #

Patch Set 6 : Compile fixup #

Total comments: 2

Patch Set 7 : Address comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+99 lines, -10 lines) Patch
M base/debug/activity_analyzer.h View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
M base/debug/activity_analyzer.cc View 1 2 3 4 5 6 6 chunks +53 lines, -6 lines 0 comments Download
M base/debug/activity_tracker.h View 1 2 3 4 1 chunk +4 lines, -1 line 0 comments Download
M base/debug/activity_tracker.cc View 1 2 3 4 2 chunks +5 lines, -1 line 0 comments Download
M chrome/browser/metrics/chrome_metrics_service_client.cc View 1 2 3 4 3 chunks +7 lines, -0 lines 0 comments Download
M components/browser_watcher/postmortem_report_collector_unittest.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M components/browser_watcher/postmortem_report_extractor.cc View 1 2 3 4 2 chunks +3 lines, -0 lines 0 comments Download
M components/browser_watcher/stability_report.proto View 1 2 3 4 1 chunk +5 lines, -1 line 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 2 chunks +17 lines, -0 lines 0 comments Download

Messages

Total messages: 46 (30 generated)
manzagop (departed)
Hi Brian, This adds some allocator validity checks. Please have a look. Thanks! Pierre
3 years, 9 months ago (2017-03-22 19:53:44 UTC) #2
bcwhite
https://codereview.chromium.org/2767193002/diff/1/base/debug/activity_analyzer.h File base/debug/activity_analyzer.h (right): https://codereview.chromium.org/2767193002/diff/1/base/debug/activity_analyzer.h#newcode166 base/debug/activity_analyzer.h:166: const PersistentMemoryAllocator& allocator() { return *allocator_; } For various ...
3 years, 9 months ago (2017-03-23 11:32:45 UTC) #3
bcwhite
https://codereview.chromium.org/2767193002/diff/1/components/browser_watcher/postmortem_report_extractor.cc File components/browser_watcher/postmortem_report_extractor.cc (right): https://codereview.chromium.org/2767193002/diff/1/components/browser_watcher/postmortem_report_extractor.cc#newcode218 components/browser_watcher/postmortem_report_extractor.cc:218: uint8_t allocator_state = global_analyzer->allocator().GetMemoryState(); > MEMORY_FULL = 99, > ...
3 years, 9 months ago (2017-03-23 11:35:41 UTC) #4
manzagop (departed)
Thanks! Another look? https://codereview.chromium.org/2767193002/diff/1/base/debug/activity_analyzer.h File base/debug/activity_analyzer.h (right): https://codereview.chromium.org/2767193002/diff/1/base/debug/activity_analyzer.h#newcode166 base/debug/activity_analyzer.h:166: const PersistentMemoryAllocator& allocator() { return *allocator_; ...
3 years, 9 months ago (2017-03-23 19:18:55 UTC) #9
bcwhite
https://codereview.chromium.org/2767193002/diff/1/components/browser_watcher/postmortem_report_extractor.cc File components/browser_watcher/postmortem_report_extractor.cc (right): https://codereview.chromium.org/2767193002/diff/1/components/browser_watcher/postmortem_report_extractor.cc#newcode218 components/browser_watcher/postmortem_report_extractor.cc:218: uint8_t allocator_state = global_analyzer->allocator().GetMemoryState(); On 2017/03/23 19:18:54, manzagop wrote: ...
3 years, 9 months ago (2017-03-23 21:10:44 UTC) #12
manzagop (departed)
Thanks! Some replies. https://codereview.chromium.org/2767193002/diff/1/components/browser_watcher/postmortem_report_extractor.cc File components/browser_watcher/postmortem_report_extractor.cc (right): https://codereview.chromium.org/2767193002/diff/1/components/browser_watcher/postmortem_report_extractor.cc#newcode218 components/browser_watcher/postmortem_report_extractor.cc:218: uint8_t allocator_state = global_analyzer->allocator().GetMemoryState(); On 2017/03/23 ...
3 years, 9 months ago (2017-03-23 21:25:02 UTC) #13
bcwhite
https://codereview.chromium.org/2767193002/diff/1/components/browser_watcher/postmortem_report_extractor.cc File components/browser_watcher/postmortem_report_extractor.cc (right): https://codereview.chromium.org/2767193002/diff/1/components/browser_watcher/postmortem_report_extractor.cc#newcode218 components/browser_watcher/postmortem_report_extractor.cc:218: uint8_t allocator_state = global_analyzer->allocator().GetMemoryState(); > > > > > ...
3 years, 9 months ago (2017-03-24 12:49:04 UTC) #14
manzagop (departed)
I agree the clients of the tracker/analyzer shouldn't deal with the allocator. I've introduced a ...
3 years, 8 months ago (2017-04-04 16:09:42 UTC) #16
manzagop (departed)
Ok, I've switched things as discussed so most of the allocator state checking happens in ...
3 years, 8 months ago (2017-04-19 16:27:30 UTC) #22
bcwhite
lgtm https://codereview.chromium.org/2767193002/diff/140001/base/debug/activity_analyzer.cc File base/debug/activity_analyzer.cc (right): https://codereview.chromium.org/2767193002/diff/140001/base/debug/activity_analyzer.cc#newcode30 base/debug/activity_analyzer.cc:30: kInvalidMemoryMappedFile = 0, It's standard practice not to ...
3 years, 8 months ago (2017-04-19 18:54:07 UTC) #31
manzagop (departed)
Thanks! https://codereview.chromium.org/2767193002/diff/140001/base/debug/activity_analyzer.cc File base/debug/activity_analyzer.cc (right): https://codereview.chromium.org/2767193002/diff/140001/base/debug/activity_analyzer.cc#newcode30 base/debug/activity_analyzer.cc:30: kInvalidMemoryMappedFile = 0, On 2017/04/19 18:54:07, bcwhite wrote: ...
3 years, 8 months ago (2017-04-19 19:26:57 UTC) #34
manzagop (departed)
Hi Steven! Could you have an OWNERS' look at: chrome\browser\metrics\chrome_metrics_service_client.cc tools\metrics\histograms\histograms.xml Thanks! Pierre
3 years, 8 months ago (2017-04-19 19:27:46 UTC) #36
Steven Holte
lgtm
3 years, 8 months ago (2017-04-19 23:27:31 UTC) #39
manzagop (departed)
Thanks for the review!
3 years, 8 months ago (2017-04-23 11:04:05 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2767193002/160001
3 years, 8 months ago (2017-04-23 11:04:25 UTC) #43
commit-bot: I haz the power
3 years, 8 months ago (2017-04-23 12:10:57 UTC) #46
Message was sent while issue was closed.
Committed patchset #7 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/0793c6c441b24c31bf2c890d243b...

Powered by Google App Engine
This is Rietveld 408576698