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

Issue 764852 link

Starred by 1 user
Status: Started
Owner:
█ invernizzi@chromium.org
User never visited
Cc:
█ bmcquade@chromium.org
clamy@chromium.org
█ rkaplow@chromium.org
█ uthakore@chromium.org
skare@chromium.org
isherman@chromium.org
asvitk...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Bit arithmetic in local_network_requests_page_load_metrics_observer.cc is suspicious

Project Member Reported by dcheng@chromium.org, Sep 13 2017 
// These constants are used to generate the UMA histograms for local network
// request metrics. If the enums for |DomainType|, |ResourceType|, or |PortType|
// change, the bitwise arithmetic below must also change.
static const int kNumNonlocalhostHistograms =
    internal::DOMAIN_TYPE_LOCALHOST << 6 |
    internal::RESOURCE_TYPE_LOCALHOST << 1 | true + 1;
static const int kNumLocalhostHistograms =
    internal::DOMAIN_TYPE_LOCALHOST << 5 | internal::PORT_TYPE_DEV << 1 |
    true + 1;

+ has higher precedence than |, so this actually does

(flag << 5) | (flag2 << 1) | (true + 1)

But I believe the intention was to write:

((flag << 5) | (flag2 << 1) | true) + 1

Also, please just write 1 instead of writing true.

Comment 1 by bmcquade@chromium.org, Sep 13 2017

Cc: rkaplow@chromium.org
Owner: invernizzi@chromium.org
Thanks for noticing this!

The author of this code is no longer at Google, though his Chromium account may still be active. Uttam, if you're still monitoring this and you'd like to fix this, feel free to send a change my way.

Otherwise, Luca, you may want to fix this as it would affect recording of your metrics.

Comment 2 by uthakore@chromium.org, Sep 14 2017

Labels: -Pri-2 Pri-3
Thanks for following up on this, Brian!

Thankfully, we currently don't generate histograms for DOMAIN_TYPE_LOCALHOST (we don't care about it for our experiments, but we provide support for others to report it), so this shouldn't impact any of the current metric collection or cause issues for our experiments, but it is an error in the bit arithmetic and should be fixed. Unless someone else intends to change the code to collect localhost page load metrics, this is low priority.

I don't have the development environment for Chromium set up on my machine at the moment, so it would take some time for me to submit the CL. I will submit the change once I get some time to set up the environment.

Comment 3 by uthakore@chromium.org, Oct 4 2017

Status: Started (was: Assigned)
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 4 2017 

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

commit 593e0708e9daf8af6328584030ee7bc3e7c7c75e
Author: Uttam Thakore <uthakore@chromium.org>
Date: Wed Oct 04 18:43:14 2017

Fixing bit arithmetic in local network requests PLMO

This CL fixes the bit arithmetic in the constants used to generate UMA histograms for local network request metrics. The problems with the arithmetic are detailed in the associated bug.

BUG=764852

Change-Id: Ifd06282b05b1893cae7756840893053fc1daa057
Reviewed-on: https://chromium-review.googlesource.com/699703
Reviewed-by: Bryan McQuade <bmcquade@chromium.org>
Commit-Queue: U Thakore <uthakore@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506460}
[modify] https://crrev.com/593e0708e9daf8af6328584030ee7bc3e7c7c75e/chrome/browser/page_load_metrics/observers/local_network_requests_page_load_metrics_observer.cc
►

Sign in to add a comment