Adding previews information to PLM UKM

chrome/browser/previews/previews_infobar_tab_helper.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/previews/previews_infobar_tab_helper.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #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"
 #include "chrome/browser/profiles/profile.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h"
 #include "components/previews/core/previews_experiments.h"
 #include "components/previews/core/previews_ui_service.h"
@@ skipping 42 matching lines @@
 
 void PreviewsInfoBarTabHelper::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
   // Only show the infobar if this is a full main frame navigation.
   if (!navigation_handle->IsInMainFrame() ||
       !navigation_handle->HasCommitted() || navigation_handle->IsSameDocument())
     return;
   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
   // ResourceDispatcherHostDelegate::GetNavigationData during commit.
   // Because ChromeResourceDispatcherHostDelegate always returns a
   // ChromeNavigationData, it is safe to static_cast here.
   ChromeNavigationData* chrome_navigation_data =
       static_cast<ChromeNavigationData*>(
           navigation_handle->GetNavigationData());
   if (chrome_navigation_data) {
     data_reduction_proxy::DataReductionProxyData* data =
         chrome_navigation_data->GetDataReductionProxyData();
 
     if (data && data->page_id()) {
       committed_data_saver_navigation_id_ = data_reduction_proxy::NavigationID(
           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 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.

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.

[RyanSturm, 2017/06/28 16:19:35]

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 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 because that approach SGTM.

Agreed about the dangling reference for a lot of reasons.

[RyanSturm, 2017/07/11 21:46:41]

Done.

+    if (!source_id_)
+      source_id_ = metrics_observer->GetUKMSourceForCommittedLoad();
+  }
 
 #if defined(OS_ANDROID)
   offline_pages::OfflinePageTabHelper* tab_helper =
       offline_pages::OfflinePageTabHelper::FromWebContents(web_contents());
 
   if (tab_helper && tab_helper->IsShowingOfflinePreview()) {
     if (navigation_handle->IsErrorPage()) {
       // TODO(ryansturm): Add UMA for errors.
       return;
     }
@@ skipping 35 matching lines @@
       DataReductionProxyChromeSettingsFactory::GetForBrowserContext(
           browser_context_);
   if (!data_reduction_proxy_settings ||
       !data_reduction_proxy_settings->data_reduction_proxy_service()) {
     return;
   }
   data_reduction_proxy_settings->data_reduction_proxy_service()
       ->pingback_client()
       ->ClearNavigationKeyAsync(committed_data_saver_navigation_id_.value());
 }