New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 661401 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Feature

Blocked on:
issue 661405
issue 661441



Sign in to add a comment

Type mismatches of UMA_HISTOGRAM_ENUMERATION should be caught

Project Member Reported by wychen@chromium.org, Nov 2 2016

Issue description

The API of UMA enum histogram is:
  UMA_HISTOGRAM_ENUMERATION("name", sample, max)

If |sample| and |max| are of different enum, it's likely a bug.

We had a similar bug in Blink (https://codereview.chromium.org/2465783002/), and it might be beneficial to somehow catch this kind of type mismatch.

A design doc is here:
https://docs.google.com/document/d/1NTi9c4d8pFxYZyq_CR4ED3mMfAqHVU13FUM05WltT44/edit
 
A CL doing static_assert() is here, but it can only land after all the mismatches are fixed.
https://codereview.chromium.org/2469993002/
Blockedon: 661405
Blockedon: 661441
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 2 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/48e95c8ed1c285048d172fdd00361673f808bfee

commit 48e95c8ed1c285048d172fdd00361673f808bfee
Author: wychen <wychen@chromium.org>
Date: Wed Nov 02 07:10:19 2016

Put all reasons in MainThreadScrollingReason into one enum

This is to avoid a false positive in enum type equality checking in
https://codereview.chromium.org/2469993002/.

No intended binary or behavior changes.

BUG=661401
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://codereview.chromium.org/2469013004
Cr-Commit-Position: refs/heads/master@{#429226}

[modify] https://crrev.com/48e95c8ed1c285048d172fdd00361673f808bfee/cc/input/main_thread_scrolling_reason.h

Project Member

Comment 5 by bugdroid1@chromium.org, Nov 2 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a4484a668c810bc78360cfe4ea03b573dcdf4dc8

commit a4484a668c810bc78360cfe4ea03b573dcdf4dc8
Author: wychen <wychen@chromium.org>
Date: Wed Nov 02 21:33:50 2016

Fix some UMA enum type mismatches

These only affects the number of buckets.
The number of buckets were too large, so the bug didn't affect UMA analysis.

This was discovered by the checks implemented in this CL:
https://codereview.chromium.org/2469993002/

BUG=661401

Review-Url: https://codereview.chromium.org/2469133003
Cr-Commit-Position: refs/heads/master@{#429406}

[modify] https://crrev.com/a4484a668c810bc78360cfe4ea03b573dcdf4dc8/chrome/browser/ui/webui/signin/inline_login_handler_impl.cc
[modify] https://crrev.com/a4484a668c810bc78360cfe4ea03b573dcdf4dc8/net/disk_cache/simple/simple_synchronous_entry.cc

Project Member

Comment 7 by bugdroid1@chromium.org, May 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/10e7f397ba671b138eab9796bbe3e44158ee3d1e

commit 10e7f397ba671b138eab9796bbe3e44158ee3d1e
Author: wychen <wychen@chromium.org>
Date: Wed May 17 05:29:27 2017

Add no-compile test for UmaHistogramEnumeration

BUG=661401

Review-Url: https://codereview.chromium.org/2891613002
Cr-Commit-Position: refs/heads/master@{#472340}

[modify] https://crrev.com/10e7f397ba671b138eab9796bbe3e44158ee3d1e/base/metrics/histogram_unittest.nc

Project Member

Comment 8 by bugdroid1@chromium.org, May 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9e472e822debee5894442de2b77dfd46d0ae9547

commit 9e472e822debee5894442de2b77dfd46d0ae9547
Author: wychen <wychen@chromium.org>
Date: Wed May 17 05:33:58 2017

Add more no-compile test for UMA_HISTOGRAM_ENUMERATION

BUG=661401

Review-Url: https://codereview.chromium.org/2886733003
Cr-Commit-Position: refs/heads/master@{#472341}

[modify] https://crrev.com/9e472e822debee5894442de2b77dfd46d0ae9547/base/metrics/histogram_unittest.nc

Project Member

Comment 9 by bugdroid1@chromium.org, Jul 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/223326c134a0cbe5358bd317b2a51bb2c7a4fe67

commit 223326c134a0cbe5358bd317b2a51bb2c7a4fe67
Author: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Date: Fri Jul 21 02:08:28 2017

Retain types used by UMA_HISTOGRAM_ENUMERATION

There will be a stricter type checking in UMA_HISTOGRAM_ENUMERATION:
if |boundary| is enum, |sample| must be an enum as well.

One common type of violation is unnecessary casting to int. This CL
fixes them by retaining the types.

Bug: 661401

TBR: bartfab@chromium.org,dtrainor@chromium.org,xiyuan@chromium.org,cpu@chromium.org,boliu@chromium.org,piman@chromium.org,msw@chromium.org,sdefresne@chromium.org,mmenke@chromium.org
Change-Id: Ib1294421896e73ba89a2c294022697686fee3199
Reviewed-on: https://chromium-review.googlesource.com/575380
Commit-Queue: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Reviewed-by: Alexei Svitkine (slow) <asvitkine@chromium.org>
Reviewed-by: Bo Liu <boliu@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488551}
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/chrome/browser/android/metrics/launch_metrics.cc
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/chrome/browser/captive_portal/captive_portal_service.cc
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/chrome/browser/chromeos/login/reauth_stats.cc
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/chrome/browser/chromeos/login/screens/reset_screen.cc
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/chrome/browser/diagnostics/diagnostics_test.cc
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/chrome/browser/policy/cloud/cloud_policy_invalidator.cc
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/chrome/browser/policy/cloud/cloud_policy_invalidator.h
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/chrome/browser/prerender/prerender_link_manager.cc
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/chrome/browser/ui/toolbar/app_menu_model.cc
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/chrome/browser/ui/toolbar/app_menu_model.h
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/chrome/browser/ui/webui/app_launcher_login_handler.cc
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/chrome/browser/ui/webui/app_launcher_login_handler.h
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/chrome/browser/ui/webui/signin/inline_login_handler_impl.cc
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/components/leveldb/public/cpp/util.cc
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/components/leveldb/public/cpp/util.h
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/components/metrics/stability_metrics_helper.cc
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/components/signin/core/browser/signin_metrics.cc
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/components/signin/core/browser/signin_metrics.h
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/content/browser/browser_child_process_host_impl.cc
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/content/browser/browser_child_process_host_impl.h
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/content/browser/download/download_item_impl.cc
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/content/browser/download/download_stats.cc
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/content/browser/download/download_stats.h
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/content/renderer/input/render_widget_input_handler.cc
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/ios/chrome/browser/ui/contextual_search/contextual_search_metrics.mm
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/ios/chrome/browser/ui/sync/sync_util.mm
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/net/dns/host_resolver_impl.cc
[modify] https://crrev.com/223326c134a0cbe5358bd317b2a51bb2c7a4fe67/net/ssl/ssl_connection_status_flags.h

Project Member

Comment 10 by bugdroid1@chromium.org, Jul 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3330991f694a9aced6e5bed98756d32e91844e43

commit 3330991f694a9aced6e5bed98756d32e91844e43
Author: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Date: Thu Jul 27 17:25:57 2017

Use stricter type checking in UMA_HISTOGRAM_ENUMERATION

Add one more type checking: if |boundary| is enum, |sample| must be
an enum as well.

Common violations:
1) Casting to int for convenience or bitfield: fixed by static_cast'ing.
2) Ranges: use UMA_HISTOGRAM_EXACT_LINEAR instead.

Bug: 661401

TBR: droger@chromium.org,stevenjb@chromium.org,calamity@chromium.org,tommi@chromium.org,gab@chromium.org,oshima@chromium.org,olivierrobin@chromium.org,piman@chromium.org,juliatuttle@chromium.org,asvitkine@chromium.org,caitkp@chromium.org,zea@chromium.org
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: Id143d15bd66d363d84c2ad2edd6feb5b47fe72ef
Reviewed-on: https://chromium-review.googlesource.com/575428
Commit-Queue: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Reviewed-by: calamity <calamity@chromium.org>
Reviewed-by: Olivier Robin <olivierrobin@chromium.org>
Reviewed-by: Nicolas Zea <zea@chromium.org>
Reviewed-by: Alexei Svitkine (OOO July28-Aug6) <asvitkine@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Julia Tuttle <juliatuttle@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#490008}
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/ash/metrics/user_metrics_recorder.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/ash/shelf/shelf_view.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/base/android/library_loader/library_loader_hooks.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/base/debug/activity_tracker.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/base/metrics/histogram_functions_unittest.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/base/metrics/histogram_macros_internal.h
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/base/metrics/histogram_unittest.nc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/chrome/browser/android/metrics/launch_metrics.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/chrome/browser/chromeos/app_mode/kiosk_app_launch_error.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/chrome/browser/chromeos/policy/upload_job_impl.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/chrome/browser/engagement/important_sites_util.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/chrome/browser/media/android/remote/record_cast_action.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/chrome/browser/metrics/android_metrics_provider.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/chrome/browser/notifications/message_center_stats_collector.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/components/password_manager/core/browser/password_syncable_service.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/components/payments/core/journey_logger.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/components/policy/core/common/cloud/device_management_service.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/components/search_provider_logos/logo_tracker.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/components/sync/driver/async_directory_type_controller.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/components/sync/driver/data_type_manager_impl.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/components/sync/driver/frontend_data_type_controller.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/components/sync/driver/model_association_manager.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/components/sync/driver/model_type_controller.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/components/sync/driver/startup_controller.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/components/sync/engine_impl/model_type_registry.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/components/sync/model/data_type_error_handler_impl.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/components/sync_bookmarks/bookmark_model_associator.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/content/browser/browser_child_process_host_impl.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/content/browser/gpu/gpu_process_host.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/ios/chrome/today_extension/today_view_controller.mm
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/media/audio/audio_output_resampler.cc
[modify] https://crrev.com/3330991f694a9aced6e5bed98756d32e91844e43/net/quic/chromium/quic_connection_logger.cc

Project Member

Comment 11 by sheriffbot@chromium.org, Jul 30

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
wychen@: fixed?
Cc: wychen@chromium.org
Owner: ----
Status: Available (was: Untriaged)
The majority of the issue is addressed, but there are still quite some TODOs linking to this bug.

Sign in to add a comment