|
|
Created:
3 years, 8 months ago by bcwhite Modified:
3 years, 7 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, danakj+watch_chromium.org, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd 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) #
Messages
Total messages: 24 (17 generated)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bcwhite@chromium.org changed reviewers: + asvitkine@chromium.org
lgtm https://codereview.chromium.org/2836993002/diff/1/base/metrics/histogram_snap... File base/metrics/histogram_snapshot_manager.cc (right): https://codereview.chromium.org/2836993002/diff/1/base/metrics/histogram_snap... base/metrics/histogram_snapshot_manager.cc:97: void HistogramSnapshotManager::InspectLoggedSamplesInconsistency( I don't see this being called anywhere. Can you remove this at the same time while you're touching this file?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2836993002/diff/1/base/metrics/histogram_snap... File base/metrics/histogram_snapshot_manager.cc (right): https://codereview.chromium.org/2836993002/diff/1/base/metrics/histogram_snap... base/metrics/histogram_snapshot_manager.cc:97: void HistogramSnapshotManager::InspectLoggedSamplesInconsistency( On 2017/04/24 16:33:09, Alexei Svitkine (slow) wrote: > I don't see this being called anywhere. Can you remove this at the same time > while you're touching this file? Done.
lgtm https://codereview.chromium.org/2836993002/diff/20001/base/metrics/histogram_... File base/metrics/histogram_snapshot_manager.h (right): https://codereview.chromium.org/2836993002/diff/20001/base/metrics/histogram_... base/metrics/histogram_snapshot_manager.h:82: HistogramSamples* logged_samples); Remove this too. :)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2836993002/diff/20001/base/metrics/histogram_... File base/metrics/histogram_snapshot_manager.h (right): https://codereview.chromium.org/2836993002/diff/20001/base/metrics/histogram_... base/metrics/histogram_snapshot_manager.h:82: HistogramSamples* logged_samples); On 2017/04/24 19:29:07, Alexei Svitkine (slow) wrote: > Remove this too. :) Oops. Didn't save the file. :-)
The CQ bit was unchecked by bcwhite@chromium.org
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2836993002/#ps40001 (title: "remove dead code (InspectLoggedSamplesInconsistency)")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493065724079900, "parent_rev": "82f29400ec0c56897e7e464cf91af7a1ce86e145", "commit_rev": "1c8b1fb10b26ef237db82604f611bc4396c45f17"}
Message was sent while issue was closed.
Description was changed from ========== Add ThreadChecker to HistogramSnapshotManager. BUG=600717 ========== to ========== Add ThreadChecker to HistogramSnapshotManager. BUG=600717 Review-Url: https://codereview.chromium.org/2836993002 Cr-Commit-Position: refs/heads/master@{#466760} Committed: https://chromium.googlesource.com/chromium/src/+/1c8b1fb10b26ef237db82604f611... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/1c8b1fb10b26ef237db82604f611...
Message was sent while issue was closed.
Description was changed from ========== Add ThreadChecker to HistogramSnapshotManager. BUG=600717 Review-Url: https://codereview.chromium.org/2836993002 Cr-Commit-Position: refs/heads/master@{#466760} Committed: https://chromium.googlesource.com/chromium/src/+/1c8b1fb10b26ef237db82604f611... ========== to ========== 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/+/1c8b1fb10b26ef237db82604f611... ========== |