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

Issue 760733 link

Starred by 1 user
Status: Fixed
Owner:
erikc...@chromium.org
Closed: Sep 2017
Cc:
█ bmcquade@chromium.org
█ l...@chromium.org
etienneb@chromium.org
oysteine@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Fix recently introduced UKM source bug.

Project Member Reported by erikc...@chromium.org, Aug 30 2017 
A recent refactor broke UKM source setting.

"""
+ lpy, bmcquade

> The actual association of that Source to URL is still left to PageLoadMetricsObserver/PageLoadTracker, though bmcquade is working on moving that to a Ukm owned WebContentsObserver.  If your unittest doesn't run that code, you might not be getting those URLs either.

The code that sets the UkmSource (WebContentsImpl::UpdateUrlForUkmSource) is not being called in the browser test. It gets called from a Getter (!?) RenderWidgetHostLatencyTracker::GetUkmSourceId, which in turn is hooked up to LatencyTracker, ... etc.

This is a behavior change from the original code I wrote, which sets the UKM source id on every URL commit. This new behavior seems incorrect [as indicated by the broken browser test], since it doesn't necessarily update the URL at the appropriate times. 
"""

Comment 1 by erikc...@chromium.org, Aug 30 2017

Cc: bmcquade@chromium.org l...@chromium.org etienneb@chromium.org oysteine@chromium.org
Owner: erikc...@chromium.org
Status: Started (was: stat)
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 31 2017 

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

commit 50ce88d08865c2216f70d85dcbad50e40460d72f
Author: Erik Chen <erikchen@chromium.org>
Date: Thu Aug 31 23:00:00 2017

Fix bug in ResourceCoordinatorWebContentsObserver.

ProcessMemoryMetricsEmitterBrowserTest was failing to catch the null case -
where no UKM sources were being created. This allowed a bug in
ResourceCoordinatorWebContentsObserver sneak through which caused no URL to be
emitted for sources.

Bug:  760733 
Change-Id: Ib59139d88763156d2b8dbe18b24416bd711d5cb0
Reviewed-on: https://chromium-review.googlesource.com/644388
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Zhen Wang <zhenw@chromium.org>
Reviewed-by: lpy <lpy@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499059}
[modify] https://crrev.com/50ce88d08865c2216f70d85dcbad50e40460d72f/chrome/browser/metrics/process_memory_metrics_emitter.cc
[modify] https://crrev.com/50ce88d08865c2216f70d85dcbad50e40460d72f/chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc
[modify] https://crrev.com/50ce88d08865c2216f70d85dcbad50e40460d72f/chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc
[modify] https://crrev.com/50ce88d08865c2216f70d85dcbad50e40460d72f/chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.h
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 1 2017 

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

commit ba0aeefa9190a13567c764f237df5d86349e71a5
Author: Erik Chen <erikchen@chromium.org>
Date: Fri Sep 01 14:53:03 2017

Revert "Fix bug in ResourceCoordinatorWebContentsObserver."

This reverts commit 50ce88d08865c2216f70d85dcbad50e40460d72f.

Missing nullptr checks.
https://bugs.chromium.org/p/chromium/issues/detail?id=761234#c2

Original change's description:
> Fix bug in ResourceCoordinatorWebContentsObserver.
> 
> ProcessMemoryMetricsEmitterBrowserTest was failing to catch the null case -
> where no UKM sources were being created. This allowed a bug in
> ResourceCoordinatorWebContentsObserver sneak through which caused no URL to be
> emitted for sources.
> 
> Bug:  760733 
> Change-Id: Ib59139d88763156d2b8dbe18b24416bd711d5cb0
> Reviewed-on: https://chromium-review.googlesource.com/644388
> Reviewed-by: Steven Holte <holte@chromium.org>
> Reviewed-by: Zhen Wang <zhenw@chromium.org>
> Reviewed-by: lpy <lpy@chromium.org>
> Commit-Queue: Erik Chen <erikchen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#499059}

TBR=zhenw@chromium.org,erikchen@chromium.org,holte@chromium.org,bmcquade@chromium.org,lpy@chromium.org

Change-Id: I9901d0a4b6bb4a4f131d977365cc072cec09c7ff
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  760733 
Reviewed-on: https://chromium-review.googlesource.com/647906
Reviewed-by: Erik Chen <erikchen@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499195}
[modify] https://crrev.com/ba0aeefa9190a13567c764f237df5d86349e71a5/chrome/browser/metrics/process_memory_metrics_emitter.cc
[modify] https://crrev.com/ba0aeefa9190a13567c764f237df5d86349e71a5/chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc
[modify] https://crrev.com/ba0aeefa9190a13567c764f237df5d86349e71a5/chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc
[modify] https://crrev.com/ba0aeefa9190a13567c764f237df5d86349e71a5/chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.h
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 1 2017

Labels: merge-merged-3202
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8c311d3ac331f73061333313873bee4b38387137

commit 8c311d3ac331f73061333313873bee4b38387137
Author: Erik Chen <erikchen@chromium.org>
Date: Fri Sep 01 16:43:39 2017

[Merge to 3202] Revert "Fix bug in ResourceCoordinatorWebContentsObserver."

This reverts commit 50ce88d08865c2216f70d85dcbad50e40460d72f.

Missing nullptr checks.
https://bugs.chromium.org/p/chromium/issues/detail?id=761234#c2

Original change's description:
> Fix bug in ResourceCoordinatorWebContentsObserver.
>
> ProcessMemoryMetricsEmitterBrowserTest was failing to catch the null case -
> where no UKM sources were being created. This allowed a bug in
> ResourceCoordinatorWebContentsObserver sneak through which caused no URL to be
> emitted for sources.
>
> Bug:  760733 
> Change-Id: Ib59139d88763156d2b8dbe18b24416bd711d5cb0
> Reviewed-on: https://chromium-review.googlesource.com/644388
> Reviewed-by: Steven Holte <holte@chromium.org>
> Reviewed-by: Zhen Wang <zhenw@chromium.org>
> Reviewed-by: lpy <lpy@chromium.org>
> Commit-Queue: Erik Chen <erikchen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#499059}

TBR=bmcquade@chromium.org, erikchen@chromium.org, holte@chromium.org, lpy@chromium.org, zhenw@chromium.org

(cherry picked from commit ba0aeefa9190a13567c764f237df5d86349e71a5)

Change-Id: I9901d0a4b6bb4a4f131d977365cc072cec09c7ff
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  760733 
Reviewed-on: https://chromium-review.googlesource.com/647906
Reviewed-by: Erik Chen <erikchen@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#499195}
Reviewed-on: https://chromium-review.googlesource.com/647770
Reviewed-by: Steven Holte <holte@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#5}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/8c311d3ac331f73061333313873bee4b38387137/chrome/browser/metrics/process_memory_metrics_emitter.cc
[modify] https://crrev.com/8c311d3ac331f73061333313873bee4b38387137/chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc
[modify] https://crrev.com/8c311d3ac331f73061333313873bee4b38387137/chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc
[modify] https://crrev.com/8c311d3ac331f73061333313873bee4b38387137/chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.h

Comment 5 by erikc...@chromium.org, Sep 1 2017

Status: Fixed (was: Started)
►

Sign in to add a comment