int32 overflow in time histogram |
|
Issue descriptionrecordTimesHistogram() and siblings use |long|, but UMA only accepts int32, so the values are clamped in function recordCustomTimesHistogramMilliseconds(). It isn't immediately obvious for developers if a new UMA would exceed int32 range. This issue was discovered in CL https://chromium-review.googlesource.com/c/chromium/src/+/834990. Compile-time checking doesn't seem possible, so assertion might be the next best thing.
,
Jan 18 2018
Assertion is added in CL https://chromium-review.googlesource.com/c/chromium/src/+/834763. There's is only one known violation discovered in CQ, but it's possible that we'll discover more in waterfall or even later in the pipeline. We'll have to revert the assertion if there's any breakage in waterfall.
,
Jan 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c11e9287e2c10548a34b393a29a50f29a9711508 commit c11e9287e2c10548a34b393a29a50f29a9711508 Author: Wei-Yin Chen (陳威尹) <wychen@chromium.org> Date: Wed Jan 24 19:40:08 2018 Assert time histogram ranges are within int32 recordTimesHistogram() and siblings use |long|, but UMA only accepts int32, so the values are clamped in function recordCustomTimesHistogramMilliseconds(). Assert the ranges are within int32 to be safe. Bug: 803541 Change-Id: I498b08f343391858586b2931442cf15eb39fd3c3 Reviewed-on: https://chromium-review.googlesource.com/834763 Commit-Queue: Wei-Yin Chen (陳威尹) <wychen@chromium.org> Reviewed-by: Yaron Friedman <yfriedman@chromium.org> Reviewed-by: Ilya Sherman <isherman@chromium.org> Cr-Commit-Position: refs/heads/master@{#531647} [modify] https://crrev.com/c11e9287e2c10548a34b393a29a50f29a9711508/base/android/java/src/org/chromium/base/metrics/RecordHistogram.java |
|
►
Sign in to add a comment |
|
Comment 1 by wychen@chromium.org
, Jan 18 2018