Fix overflow when logging MaxInt32 to a sparse histogram.

Fix overflow when logging MaxInt32 to a sparse histogram.

The problem was that a bucket gets reported with an inclusive
min value and exclusive max value, so max must to be +1 of
the sample value for a sparse histogram value.

Because the code wasn't handling this specifically, it caused the
bucket max to be reported as a MinInt32 due to overflow of a 32
bit value. This CL changes the relevant iterator API to use int64_t
for the max, to support MaxInt32 being logged.

BUG=716600
TBR=jbauman@chromium.org, groby@chromium.org

Review-Url: https://codereview.chromium.org/2853853002
Cr-Commit-Position: refs/heads/master@{#469136}
Committed: https://chromium.googlesource.com/chromium/src/+/3f17b1463420e026459e2d7fde6335cf5e9b1852

[Alexei Svitkine (slow), 2017-05-01 18:15:57]

The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 18:16:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 18:27:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-01 18:27:07]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Alexei Svitkine (slow), 2017-05-01 19:21:27]

The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 19:22:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 19:33:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-01 19:33:42]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Alexei Svitkine (slow), 2017-05-01 19:52:46]

The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 19:53:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexei Svitkine (slow), 2017-05-01 19:57:13]

Patchset #2 (id:20001) has been deleted

[Alexei Svitkine (slow), 2017-05-01 19:57:46]

Description was changed from

==========
Fix overflow when logging MaxInt32 to a sparse histogram.

The problem was that a bucket is reported with an inclusive
min value and exclusive max value, so max had to be +1 of
the sample for a sparse histogram value. Because the code
wasn't handling this specifically, it caused the bucket
max to be reported as a MinInt32 due to overflow of a 32
bit value. This CL changes the relevant iterator API to
use int64_t for the max, to support MaxInt32 being logged.

BUG=716600
==========

to

==========
Fix overflow when logging MaxInt32 to a sparse histogram.

The problem was that a bucket gets reported with an inclusive
min value and exclusive max value, so max had to be +1 of
the sample for a sparse histogram value.

Because the code wasn't handling this specifically, it caused the
bucket max to be reported as a MinInt32 due to overflow of a 32
bit value. This CL changes the relevant iterator API to use int64_t
for the max, to support MaxInt32 being logged.

BUG=716600
==========

[Alexei Svitkine (slow), 2017-05-01 19:58:17]

Description was changed from

==========
Fix overflow when logging MaxInt32 to a sparse histogram.

The problem was that a bucket gets reported with an inclusive
min value and exclusive max value, so max had to be +1 of
the sample for a sparse histogram value.

Because the code wasn't handling this specifically, it caused the
bucket max to be reported as a MinInt32 due to overflow of a 32
bit value. This CL changes the relevant iterator API to use int64_t
for the max, to support MaxInt32 being logged.

BUG=716600
==========

to

==========
Fix overflow when logging MaxInt32 to a sparse histogram.

The problem was that a bucket gets reported with an inclusive
min value and exclusive max value, so max must to be +1 of
the sample value for a sparse histogram value.

Because the code wasn't handling this specifically, it caused the
bucket max to be reported as a MinInt32 due to overflow of a 32
bit value. This CL changes the relevant iterator API to use int64_t
for the max, to support MaxInt32 being logged.

BUG=716600
==========

[commit-bot: I haz the power, 2017-05-01 20:56:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-01 20:56:04]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Alexei Svitkine (slow), 2017-05-02 14:50:47]

The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 14:51:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexei Svitkine (slow), 2017-05-02 14:58:21]

The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 14:58:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexei Svitkine (slow), 2017-05-02 15:28:40]

Patchset #2 (id:40001) has been deleted

[Alexei Svitkine (slow), 2017-05-02 15:28:44]

Patchset #1 (id:1) has been deleted

[Alexei Svitkine (slow), 2017-05-02 15:29:32]

asvitkine@chromium.org changed reviewers:
+ isherman@chromium.org

[commit-bot: I haz the power, 2017-05-02 16:10:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-02 16:10:13]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2017-05-02 21:37:49]

Thanks, Alexei.  LGTM % nits:

https://codereview.chromium.org/2853853002/diff/60001/base/metrics/histogram_...
File base/metrics/histogram_samples.h (right):

https://codereview.chromium.org/2853853002/diff/60001/base/metrics/histogram_...
base/metrics/histogram_samples.h:203: // values up to MAXINT32+1.
Could you add a compile-time assertion somewhere that the max-value for the
recorded-to bucket is less than the max-possible value for the |max| param? 
We've been talking about possibly extending histograms to report 64-bit values,
and I'm a bit worried that we'd accidentally regress this were anyone to
undertake implementing that transition.

https://codereview.chromium.org/2853853002/diff/60001/base/metrics/persistent...
File base/metrics/persistent_sample_map.cc (right):

https://codereview.chromium.org/2853853002/diff/60001/base/metrics/persistent...
base/metrics/persistent_sample_map.cc:80: *max =
static_cast<int64_t>(iter_->first) + 1;
nit: Here and everywhere else that you add a static_cast, could you please use a
strict_cast from base/numerics/safe_conversions.h instead?

https://codereview.chromium.org/2853853002/diff/60001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc
(right):

https://codereview.chromium.org/2853853002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc:44:
bucket->SetInteger("max", static_cast<int>(max));
nit: Could this be a checked_cast?

[Alexei Svitkine (slow), 2017-05-02 21:57:31]

The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 21:58:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 23:36:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-02 23:36:27]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2017-05-03 15:21:11]

Patchset #2 (id:80001) has been deleted

[Alexei Svitkine (slow), 2017-05-03 15:21:53]

https://codereview.chromium.org/2853853002/diff/60001/base/metrics/histogram_...
File base/metrics/histogram_samples.h (right):

https://codereview.chromium.org/2853853002/diff/60001/base/metrics/histogram_...
base/metrics/histogram_samples.h:203: // values up to MAXINT32+1.
On 2017/05/02 21:37:49, Ilya Sherman wrote:
> Could you add a compile-time assertion somewhere that the max-value for the
> recorded-to bucket is less than the max-possible value for the |max| param? 
> We've been talking about possibly extending histograms to report 64-bit
values,
> and I'm a bit worried that we'd accidentally regress this were anyone to
> undertake implementing that transition.

Done.

https://codereview.chromium.org/2853853002/diff/60001/base/metrics/persistent...
File base/metrics/persistent_sample_map.cc (right):

https://codereview.chromium.org/2853853002/diff/60001/base/metrics/persistent...
base/metrics/persistent_sample_map.cc:80: *max =
static_cast<int64_t>(iter_->first) + 1;
On 2017/05/02 21:37:49, Ilya Sherman wrote:
> nit: Here and everywhere else that you add a static_cast, could you please use
a
> strict_cast from base/numerics/safe_conversions.h instead?

Done.

https://codereview.chromium.org/2853853002/diff/60001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc
(right):

https://codereview.chromium.org/2853853002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc:44:
bucket->SetInteger("max", static_cast<int>(max));
On 2017/05/02 21:37:49, Ilya Sherman wrote:
> nit: Could this be a checked_cast?

Done.

[Alexei Svitkine (slow), 2017-05-03 15:21:59]

The CQ bit was checked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2017-05-03 15:22:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2853853002/#ps100001
(title: "Address comments.")

[commit-bot: I haz the power, 2017-05-03 15:22:28]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 15:30:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 15:30:24]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Alexei Svitkine (slow), 2017-05-03 17:37:38]

asvitkine@chromium.org changed reviewers:
+ gab@chromium.org, groby@chromium.org, jbauman@chromium.org

[Alexei Svitkine (slow), 2017-05-03 17:37:38]

+gab for 
chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc
OWNERS

+TBR jbauman@ for trivial change to componets/tracing downstream of base/metrics
API change
+TBR groby@ for trivial change to components/translate downstream of
base/metrics API change

[Alexei Svitkine (slow), 2017-05-03 17:38:03]

Description was changed from

==========
Fix overflow when logging MaxInt32 to a sparse histogram.

The problem was that a bucket gets reported with an inclusive
min value and exclusive max value, so max must to be +1 of
the sample value for a sparse histogram value.

Because the code wasn't handling this specifically, it caused the
bucket max to be reported as a MinInt32 due to overflow of a 32
bit value. This CL changes the relevant iterator API to use int64_t
for the max, to support MaxInt32 being logged.

BUG=716600
==========

to

==========
Fix overflow when logging MaxInt32 to a sparse histogram.

The problem was that a bucket gets reported with an inclusive
min value and exclusive max value, so max must to be +1 of
the sample value for a sparse histogram value.

Because the code wasn't handling this specifically, it caused the
bucket max to be reported as a MinInt32 due to overflow of a 32
bit value. This CL changes the relevant iterator API to use int64_t
for the max, to support MaxInt32 being logged.

BUG=716600
TBR=jbauman@chromium.org, groby@chromium.org
==========

[Primiano Tucci (use gerrit), 2017-05-03 18:07:49]

components/tracing/child/child_trace_message_filter.cc LGTM

[Alexei Svitkine (slow), 2017-05-03 21:25:14]

asvitkine@chromium.org changed reviewers:
+ robliao@chromium.org
- gab@chromium.org

[Alexei Svitkine (slow), 2017-05-03 21:25:14]

-gab +robliao since Rob wrote the task_scheduler_internals_ui.cc code being
changed.

[robliao, 2017-05-03 21:29:05]

chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc
lgtm

[Alexei Svitkine (slow), 2017-05-03 21:29:29]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2017-05-03 21:30:09]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 21:37:45]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493846969953770,
"parent_rev": "61b89c802236da47eb1fc17fdc34c2c66f843d30", "commit_rev":
"3f17b1463420e026459e2d7fde6335cf5e9b1852"}

[commit-bot: I haz the power, 2017-05-03 21:37:53]

Description was changed from

==========
Fix overflow when logging MaxInt32 to a sparse histogram.

The problem was that a bucket gets reported with an inclusive
min value and exclusive max value, so max must to be +1 of
the sample value for a sparse histogram value.

Because the code wasn't handling this specifically, it caused the
bucket max to be reported as a MinInt32 due to overflow of a 32
bit value. This CL changes the relevant iterator API to use int64_t
for the max, to support MaxInt32 being logged.

BUG=716600
TBR=jbauman@chromium.org, groby@chromium.org
==========

to

==========
Fix overflow when logging MaxInt32 to a sparse histogram.

The problem was that a bucket gets reported with an inclusive
min value and exclusive max value, so max must to be +1 of
the sample value for a sparse histogram value.

Because the code wasn't handling this specifically, it caused the
bucket max to be reported as a MinInt32 due to overflow of a 32
bit value. This CL changes the relevant iterator API to use int64_t
for the max, to support MaxInt32 being logged.

BUG=716600
TBR=jbauman@chromium.org, groby@chromium.org

Review-Url: https://codereview.chromium.org/2853853002
Cr-Commit-Position: refs/heads/master@{#469136}
Committed:
https://chromium.googlesource.com/chromium/src/+/3f17b1463420e026459e2d7fde63...
==========

[commit-bot: I haz the power, 2017-05-03 21:37:57]

Committed patchset #2 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/3f17b1463420e026459e2d7fde63...