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

Issue 2836993002: Add ThreadChecker to HistogramSnapshotManager. (Closed)

Created:
3 years, 8 months ago by bcwhite
Modified:
3 years, 7 months ago
CC:
chromium-reviews, danakj+watch_chromium.org, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Add ThreadChecker to HistogramSnapshotManager. There are crashes due to corrupted data structures inside the std::map that contains collected histogram information. This could be just cosmic noise flipping bits, a store-trampler, or concurrent access to this object causing corruption. Limiting access to a single thread will rule out the last. BUG=600717 Review-Url: https://codereview.chromium.org/2836993002 Cr-Commit-Position: refs/heads/master@{#466760} Committed: https://chromium.googlesource.com/chromium/src/+/1c8b1fb10b26ef237db82604f611bc4396c45f17

Patch Set 1 #

Total comments: 2

Patch Set 2 : remove dead code (InspectLoggedSamplesInconsistency) #

Total comments: 2

Patch Set 3 : remove dead code (InspectLoggedSamplesInconsistency) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -21 lines) Patch
M base/metrics/histogram_snapshot_manager.h View 1 2 3 chunks +3 lines, -5 lines 0 comments Download
M base/metrics/histogram_snapshot_manager.cc View 1 2 chunks +1 line, -16 lines 0 comments Download

Messages

Total messages: 24 (17 generated)
bcwhite
3 years, 8 months ago (2017-04-24 16:23:34 UTC) #4
Alexei Svitkine (slow)
lgtm https://codereview.chromium.org/2836993002/diff/1/base/metrics/histogram_snapshot_manager.cc File base/metrics/histogram_snapshot_manager.cc (right): https://codereview.chromium.org/2836993002/diff/1/base/metrics/histogram_snapshot_manager.cc#newcode97 base/metrics/histogram_snapshot_manager.cc:97: void HistogramSnapshotManager::InspectLoggedSamplesInconsistency( I don't see this being called ...
3 years, 8 months ago (2017-04-24 16:33:09 UTC) #5
bcwhite
https://codereview.chromium.org/2836993002/diff/1/base/metrics/histogram_snapshot_manager.cc File base/metrics/histogram_snapshot_manager.cc (right): https://codereview.chromium.org/2836993002/diff/1/base/metrics/histogram_snapshot_manager.cc#newcode97 base/metrics/histogram_snapshot_manager.cc:97: void HistogramSnapshotManager::InspectLoggedSamplesInconsistency( On 2017/04/24 16:33:09, Alexei Svitkine (slow) wrote: ...
3 years, 8 months ago (2017-04-24 19:28:28 UTC) #12
Alexei Svitkine (slow)
lgtm https://codereview.chromium.org/2836993002/diff/20001/base/metrics/histogram_snapshot_manager.h File base/metrics/histogram_snapshot_manager.h (right): https://codereview.chromium.org/2836993002/diff/20001/base/metrics/histogram_snapshot_manager.h#newcode82 base/metrics/histogram_snapshot_manager.h:82: HistogramSamples* logged_samples); Remove this too. :)
3 years, 8 months ago (2017-04-24 19:29:08 UTC) #13
bcwhite
https://codereview.chromium.org/2836993002/diff/20001/base/metrics/histogram_snapshot_manager.h File base/metrics/histogram_snapshot_manager.h (right): https://codereview.chromium.org/2836993002/diff/20001/base/metrics/histogram_snapshot_manager.h#newcode82 base/metrics/histogram_snapshot_manager.h:82: HistogramSamples* logged_samples); On 2017/04/24 19:29:07, Alexei Svitkine (slow) wrote: ...
3 years, 8 months ago (2017-04-24 19:43:05 UTC) #16
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/2836993002/40001
3 years, 8 months ago (2017-04-24 20:29:29 UTC) #20
commit-bot: I haz the power
3 years, 8 months ago (2017-04-24 20:58:52 UTC) #23
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/1c8b1fb10b26ef237db82604f611...

Powered by Google App Engine
This is Rietveld 408576698