chrome/browser/previews/previews_infobar_tab_helper.cc - Issue 2952343004: Adding previews information to PLM UKM

Unified Diff: chrome/browser/previews/previews_infobar_tab_helper.cc

Issue 2952343004: Adding previews information to PLM UKM (Closed)

Patch Set: stop observing on background Created 3 years, 6 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Index: chrome/browser/previews/previews_infobar_tab_helper.cc

diff --git a/chrome/browser/previews/previews_infobar_tab_helper.cc b/chrome/browser/previews/previews_infobar_tab_helper.cc

index 41dd2ffa62bf54af4d7f9743104233b9679b31fd..70f8a6bc0a8053f2eb65de3ffbcb65d32e8821d3 100644

--- a/chrome/browser/previews/previews_infobar_tab_helper.cc

+++ b/chrome/browser/previews/previews_infobar_tab_helper.cc

@@ -9,6 +9,7 @@

#include "chrome/browser/loader/chrome_navigation_data.h"

#include "chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h"

#include "chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_factory.h"

+#include "chrome/browser/page_load_metrics/metrics_web_contents_observer.h"

#include "chrome/browser/previews/previews_infobar_delegate.h"

#include "chrome/browser/previews/previews_service.h"

#include "chrome/browser/previews/previews_service_factory.h"

@@ -71,6 +72,7 @@ void PreviewsInfoBarTabHelper::DidFinishNavigation(

displayed_preview_infobar_ = false;

ClearLastNavigationAsync();

committed_data_saver_navigation_id_.reset();

+ source_id_.reset();

// As documented in content/public/browser/navigation_handle.h, this

// NavigationData is a clone of the NavigationData instance returned from

@@ -89,6 +91,18 @@ void PreviewsInfoBarTabHelper::DidFinishNavigation(

data->page_id().value(), data->session_key());

}

+ page_load_metrics::MetricsWebContentsObserver* metrics_observer =

+ page_load_metrics::MetricsWebContentsObserver::FromWebContents(

+ web_contents());

+ if (metrics_observer) {

+ // Due to undefined ordering between this observer and |metrics_observer|,

+ // whether this navigation is comitted in the PLM framework or tracked as

+ // pending is undefined.

+ source_id_ =

+ metrics_observer->GetUKMSourceIdForNavigationHandle(navigation_handle);

Bryan McQuade 2017/06/28 13:13:40 one risk with this approach is that PLM explicitly

RyanSturm 2017/06/28 16:19:35 I think that is fine as committed_load_ will be em

I think that is fine as committed_load_ will be empty, so source_id_ will be empty after this in that case.

The way that they are ordered is based on compiler generated addresses . See https://cs.chromium.org/chromium/src/content/public/browser/web_contents_user.... This approach as far as I understand it, has the possibility to create different ordering on different compilers for the same code, so it's not something we can rely on I believe. In general, I think the approach goes like this: previews then metrics: It will pull it out of pending_loads_ using NavigationHandle. The only cases where pending_loads_ won't have the navigation are when subframes and ShouldTrackNavigation returns false. Since PLM supports all previews types in general, source_id_ will be valid for any previews navigations. metrics then previews: pending_loads_ won't have the navigation_handle, so it will use committed_load_, and if committed_load_ is non-null, then the source_id_ will be for this navigation_handle_. In general, I think the first case is hairier, because the fallback is somewhat frightening, as it will attribute it to the previous URL. That being said: a) I could store committed_navigation_handle in MWCO and reduce this to GetUKMSourceIdForNavigationHandle() as originally proposed. Then there are reasonable assurances, with the risk of having a dangling reference to a deleted object -- this could be addressed by casting to void* or some other method. b) Get the GlobalRequestID in this class's ReadyToCommit method store it as committing_global_request_id and use GetTrackerOrNullForRequest inside MWCO instead, but I think that carries risk for transfer navigations, as I don't fully understand them. c) csharrison@ was opposed to this, but Posting a task here to this tab helper to ask MWCO the currently committed SourceId after both DidFinishNavigations run.

Bryan McQuade 2017/06/28 16:48:18 Thanks! It sounds like, given the order can vary d

On 2017/06/28 at 16:19:35, RyanSturm wrote: > On 2017/06/28 13:13:40, Bryan McQuade wrote: > > one risk with this approach is that PLM explicitly stops tracking some loads at > > commit, and you won't retain that behavior in your code. For example, we stop > > tracking non-200s, non HTML/XHTML, etc. We could duplicate that filtering logic > > but it'd be better to not have to do that. > > > > I think that is fine as committed_load_ will be empty, so source_id_ will be empty after this in that case. > > > This is an interesting/challenging case, because as you note the previews > > decision is made during DidFinishNavigation, and the ordering of invocations > > between MWCO and the previews observer appears to be not well defined. Are we > > sure ordering is not well defined? Is it the case that tab helper construction > > order guarantees WCO callback order? If that's the case, it may be sensible to > > rely on the ordering and always only use GetUKMSourceForCommittedLoad. We could > > add some tests to make sure this ordering is guaranteed, either at the WCO level > > or just in our specific observer implementations. > > The way that they are ordered is based on compiler generated addresses . See https://cs.chromium.org/chromium/src/content/public/browser/web_contents_user.... > > This approach as far as I understand it, has the possibility to create different ordering on different compilers for the same code, so it's not something we can rely on I believe. > > > In general, I think the approach goes like this: > > previews then metrics: > It will pull it out of pending_loads_ using NavigationHandle. The only cases where pending_loads_ won't have the navigation are when subframes and ShouldTrackNavigation returns false. Since PLM supports all previews types in general, source_id_ will be valid for any previews navigations. > > metrics then previews: > pending_loads_ won't have the navigation_handle, so it will use committed_load_, and if committed_load_ is non-null, then the source_id_ will be for this navigation_handle_. > > In general, I think the first case is hairier, because the fallback is somewhat frightening, as it will attribute it to the previous URL. > > That being said: > a) I could store committed_navigation_handle in MWCO and reduce this to GetUKMSourceIdForNavigationHandle() as originally proposed. Then there are reasonable assurances, with the risk of having a dangling reference to a deleted object -- this could be addressed by casting to void* or some other method. > > b) Get the GlobalRequestID in this class's ReadyToCommit method store it as committing_global_request_id and use GetTrackerOrNullForRequest inside MWCO instead, but I think that carries risk for transfer navigations, as I don't fully understand them. > > c) csharrison@ was opposed to this, but Posting a task here to this tab helper to ask MWCO the currently committed SourceId after both DidFinishNavigations run.

Thanks! It sounds like, given the order can vary depending on compiler, we need to safely handle both orderings as you've done. I'm a little bit concerned about allowing external code to get the source id of uncommitted page loads given that we might filter them out, but I think it's the most straightforward way to address this issue in the near term. We could potentially allow other components in chrome to observe commits at the time they happen in MWCO. We have something like this for tests already: https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/metrics... - I would not want to provide the actual PageLoadTracker to non-test code but we could provide perhaps a PageLoadExtraInfo which would contain the source id. That said, this would make your consumer code more complex since either order is possible. We could define a new object that only 'activates' once both commits have been observed - this would address the ordering issues nicely but is more complex and it's also not always clear that both PLM and previews activate on the same set of page loads. So in the near term can we add a comment on the new GetUKMSourceIdForNavigationHandle and GetUKMSourceForCommittedLoad noting that these are not intended for wide use yet, and consumers should ping code owners before using these APIs? If we get lots of pings asking to call these it suggests we should probably come up with a cleaner way to solve this class of problem; if this new code is the only code that uses it then we may be ok to leave as is. How does this sound? RE: retaining committed_navigation_handle, once an object is free, we're also free to re-use the same address for some other object, so this committed_navigation_handle could collide with a newly allocated navigation handle. So I think we want to avoid using pointers as identifiers after the memory being pointed to has been freed.

RyanSturm 2017/06/28 16:56:56 I'll go ahead and add the comments as suggested be

On 2017/06/28 16:48:18, Bryan McQuade wrote: > On 2017/06/28 at 16:19:35, RyanSturm wrote: > > On 2017/06/28 13:13:40, Bryan McQuade wrote: > > > one risk with this approach is that PLM explicitly stops tracking some loads > at > > > commit, and you won't retain that behavior in your code. For example, we > stop > > > tracking non-200s, non HTML/XHTML, etc. We could duplicate that filtering > logic > > > but it'd be better to not have to do that. > > > > > > > I think that is fine as committed_load_ will be empty, so source_id_ will be > empty after this in that case. > > > > > This is an interesting/challenging case, because as you note the previews > > > decision is made during DidFinishNavigation, and the ordering of invocations > > > between MWCO and the previews observer appears to be not well defined. Are > we > > > sure ordering is not well defined? Is it the case that tab helper > construction > > > order guarantees WCO callback order? If that's the case, it may be sensible > to > > > rely on the ordering and always only use GetUKMSourceForCommittedLoad. We > could > > > add some tests to make sure this ordering is guaranteed, either at the WCO > level > > > or just in our specific observer implementations. > > > > The way that they are ordered is based on compiler generated addresses . See > https://cs.chromium.org/chromium/src/content/public/browser/web_contents_user.... > > > > > This approach as far as I understand it, has the possibility to create > different ordering on different compilers for the same code, so it's not > something we can rely on I believe. > > > > > > In general, I think the approach goes like this: > > > > previews then metrics: > > It will pull it out of pending_loads_ using NavigationHandle. The only cases > where pending_loads_ won't have the navigation are when subframes and > ShouldTrackNavigation returns false. Since PLM supports all previews types in > general, source_id_ will be valid for any previews navigations. > > > > metrics then previews: > > pending_loads_ won't have the navigation_handle, so it will use > committed_load_, and if committed_load_ is non-null, then the source_id_ will be > for this navigation_handle_. > > > > In general, I think the first case is hairier, because the fallback is > somewhat frightening, as it will attribute it to the previous URL. > > > > That being said: > > a) I could store committed_navigation_handle in MWCO and reduce this to > GetUKMSourceIdForNavigationHandle() as originally proposed. Then there are > reasonable assurances, with the risk of having a dangling reference to a deleted > object -- this could be addressed by casting to void* or some other method. > > > > b) Get the GlobalRequestID in this class's ReadyToCommit method store it as > committing_global_request_id and use GetTrackerOrNullForRequest inside MWCO > instead, but I think that carries risk for transfer navigations, as I don't > fully understand them. > > > > c) csharrison@ was opposed to this, but Posting a task here to this tab helper > to ask MWCO the currently committed SourceId after both DidFinishNavigations > run. > > Thanks! It sounds like, given the order can vary depending on compiler, we need > to safely handle both orderings as you've done. > > I'm a little bit concerned about allowing external code to get the source id of > uncommitted page loads given that we might filter them out, but I think it's the > most straightforward way to address this issue in the near term. > > We could potentially allow other components in chrome to observe commits at the > time they happen in MWCO. We have something like this for tests already: > https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/metrics... > - I would not want to provide the actual PageLoadTracker to non-test code but we > could provide perhaps a PageLoadExtraInfo which would contain the source id. > > That said, this would make your consumer code more complex since either order is > possible. We could define a new object that only 'activates' once both commits > have been observed - this would address the ordering issues nicely but is more > complex and it's also not always clear that both PLM and previews activate on > the same set of page loads. > > So in the near term can we add a comment on the new > GetUKMSourceIdForNavigationHandle and GetUKMSourceForCommittedLoad noting that > these are not intended for wide use yet, and consumers should ping code owners > before using these APIs? If we get lots of pings asking to call these it > suggests we should probably come up with a cleaner way to solve this class of > problem; if this new code is the only code that uses it then we may be ok to > leave as is. > > How does this sound? > > RE: retaining committed_navigation_handle, once an object is free, we're also > free to re-use the same address for some other object, so this > committed_navigation_handle could collide with a newly allocated navigation > handle. So I think we want to avoid using pointers as identifiers after the > memory being pointed to has been freed.

I'll go ahead and add the comments as suggested because that approach SGTM. Agreed about the dangling reference for a lot of reasons.

RyanSturm 2017/07/11 21:46:41 Done.

On 2017/06/28 16:56:56, RyanSturm wrote: > On 2017/06/28 16:48:18, Bryan McQuade wrote: > > On 2017/06/28 at 16:19:35, RyanSturm wrote: > > > On 2017/06/28 13:13:40, Bryan McQuade wrote: > > > > one risk with this approach is that PLM explicitly stops tracking some > loads > > at > > > > commit, and you won't retain that behavior in your code. For example, we > > stop > > > > tracking non-200s, non HTML/XHTML, etc. We could duplicate that filtering > > logic > > > > but it'd be better to not have to do that. > > > > > > > > > > I think that is fine as committed_load_ will be empty, so source_id_ will be > > empty after this in that case. > > > > > > > This is an interesting/challenging case, because as you note the previews > > > > decision is made during DidFinishNavigation, and the ordering of > invocations > > > > between MWCO and the previews observer appears to be not well defined. Are > > we > > > > sure ordering is not well defined? Is it the case that tab helper > > construction > > > > order guarantees WCO callback order? If that's the case, it may be > sensible > > to > > > > rely on the ordering and always only use GetUKMSourceForCommittedLoad. We > > could > > > > add some tests to make sure this ordering is guaranteed, either at the WCO > > level > > > > or just in our specific observer implementations. > > > > > > The way that they are ordered is based on compiler generated addresses . See > > > https://cs.chromium.org/chromium/src/content/public/browser/web_contents_user.... > > > > > > > > This approach as far as I understand it, has the possibility to create > > different ordering on different compilers for the same code, so it's not > > something we can rely on I believe. > > > > > > > > > In general, I think the approach goes like this: > > > > > > previews then metrics: > > > It will pull it out of pending_loads_ using NavigationHandle. The only cases > > where pending_loads_ won't have the navigation are when subframes and > > ShouldTrackNavigation returns false. Since PLM supports all previews types in > > general, source_id_ will be valid for any previews navigations. > > > > > > metrics then previews: > > > pending_loads_ won't have the navigation_handle, so it will use > > committed_load_, and if committed_load_ is non-null, then the source_id_ will > be > > for this navigation_handle_. > > > > > > In general, I think the first case is hairier, because the fallback is > > somewhat frightening, as it will attribute it to the previous URL. > > > > > > That being said: > > > a) I could store committed_navigation_handle in MWCO and reduce this to > > GetUKMSourceIdForNavigationHandle() as originally proposed. Then there are > > reasonable assurances, with the risk of having a dangling reference to a > deleted > > object -- this could be addressed by casting to void* or some other method. > > > > > > b) Get the GlobalRequestID in this class's ReadyToCommit method store it as > > committing_global_request_id and use GetTrackerOrNullForRequest inside MWCO > > instead, but I think that carries risk for transfer navigations, as I don't > > fully understand them. > > > > > > c) csharrison@ was opposed to this, but Posting a task here to this tab > helper > > to ask MWCO the currently committed SourceId after both DidFinishNavigations > > run. > > > > Thanks! It sounds like, given the order can vary depending on compiler, we > need > > to safely handle both orderings as you've done. > > > > I'm a little bit concerned about allowing external code to get the source id > of > > uncommitted page loads given that we might filter them out, but I think it's > the > > most straightforward way to address this issue in the near term. > > > > We could potentially allow other components in chrome to observe commits at > the > > time they happen in MWCO. We have something like this for tests already: > > > https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/metrics... > > - I would not want to provide the actual PageLoadTracker to non-test code but > we > > could provide perhaps a PageLoadExtraInfo which would contain the source id. > > > > That said, this would make your consumer code more complex since either order > is > > possible. We could define a new object that only 'activates' once both commits > > have been observed - this would address the ordering issues nicely but is more > > complex and it's also not always clear that both PLM and previews activate on > > the same set of page loads. > > > > So in the near term can we add a comment on the new > > GetUKMSourceIdForNavigationHandle and GetUKMSourceForCommittedLoad noting that > > these are not intended for wide use yet, and consumers should ping code owners > > before using these APIs? If we get lots of pings asking to call these it > > suggests we should probably come up with a cleaner way to solve this class of > > problem; if this new code is the only code that uses it then we may be ok to > > leave as is. > > > > How does this sound? > > > > RE: retaining committed_navigation_handle, once an object is free, we're also > > free to re-use the same address for some other object, so this > > committed_navigation_handle could collide with a newly allocated navigation > > handle. So I think we want to avoid using pointers as identifiers after the > > memory being pointed to has been freed. > > I'll go ahead and add the comments as suggested because that approach SGTM. > > Agreed about the dangling reference for a lot of reasons.

Done.

+ if (!source_id_)

+ source_id_ = metrics_observer->GetUKMSourceForCommittedLoad();

+ }

#if defined(OS_ANDROID)

offline_pages::OfflinePageTabHelper* tab_helper =

« chrome/browser/previews/previews_infobar_delegate.h ('K') | « chrome/browser/previews/previews_infobar_tab_helper.h ('k') | chrome/test/BUILD.gn » ('j') | no next file with comments »