[PageLoadMetrics] Handle case where navigation aborts

chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc

Index: chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc
diff --git a/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc b/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc
index f6a08a182e620ff14f156528597e3cf507d7550c..37cdf23cff25b60fff28ca68779683a5d46dd60f 100644
--- a/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc
+++ b/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc
@@ -23,25 +23,35 @@ const base::Feature kAdsFeature{"AdsMetrics",
                                 base::FEATURE_DISABLED_BY_DEFAULT};
 
 bool FrameIsAd(content::NavigationHandle* navigation_handle) {
-  content::RenderFrameHost* current_frame_host =
-      navigation_handle->GetRenderFrameHost();
-  DCHECK(current_frame_host);
-  const std::string& name = current_frame_host->GetFrameName();
-  const GURL& url = navigation_handle->GetURL();
-
   // Because sub-resource filtering isn't always enabled, and doesn't work
   // well in monitoring mode (no CSS enforcement), it's difficult to identify
   // ads. Google ads are prevalent and easy to track, so we'll start by
   // tracking those. Note that the frame name can be very large, so be careful
   // to avoid full string searches if possible.
   // TODO(jkarlin): Track other ad networks that are easy to identify.
-  return base::StartsWith(name, "google_ads_iframe",
-                          base::CompareCase::SENSITIVE) ||
-         base::StartsWith(name, "google_ads_frame",
-                          base::CompareCase::SENSITIVE) ||
-         (url.host_piece() == "tpc.googlesyndication.com" &&
-          base::StartsWith(url.path_piece(), "/safeframe",
-                           base::CompareCase::SENSITIVE));
+
+  // In case the navigation aborted, look up the RFH by the Frame Tree Node
+  // ID. If there is no RFH remaining, then the frame is gone. We use the
+  // unsafe method of FindFrameByFrameTreeNodeId because we're not concerned
+  // with which process the frame lives on (we're just measuring bytes and not
+  // granting security priveleges).
+  content::RenderFrameHost* current_frame_host =
+      navigation_handle->GetWebContents()->UnsafeFindFrameByFrameTreeNodeId(

[Bryan McQuade, 2017/05/12 12:43:59]

does this only return frames for committed loads? or will it return frames if a
provisional load is in flight as well? maybe add a comment to explain.

[jkarlin, 2017/05/12 14:31:42]

Good question. It returns the committed RFH for the given frame id. But really
it's silly of me to check the previously committed frame for its frame name. So
instead, only checking frame name on committed navigations. PTAL.

+          navigation_handle->GetFrameTreeNodeId());
+  if (current_frame_host) {
+    const std::string& frame_name = current_frame_host->GetFrameName();
+    if (base::StartsWith(frame_name, "google_ads_iframe",
+                         base::CompareCase::SENSITIVE) ||
+        base::StartsWith(frame_name, "google_ads_frame",
+                         base::CompareCase::SENSITIVE)) {
+      return true;
+    }
+  }
+
+  const GURL& url = navigation_handle->GetURL();
+  return url.host_piece() == "tpc.googlesyndication.com" &&
+         base::StartsWith(url.path_piece(), "/safeframe",
+                          base::CompareCase::SENSITIVE);
 }
 
 }  // namespace
@@ -77,22 +87,13 @@ AdsPageLoadMetricsObserver::OnCommit(
 page_load_metrics::PageLoadMetricsObserver::ObservePolicy
 AdsPageLoadMetricsObserver::OnDidFinishSubFrameNavigation(
     content::NavigationHandle* navigation_handle) {
+  // Determine if the frame is part of an existing ad, the root of a new ad,
+  // or a non-ad frame. Once a frame is labled as an ad, it is always
+  // considered an ad, even if it navigates to a non-ad page. This function
+  // labels all of a page's frames, even those that fail to commit.
   FrameTreeNodeId frame_tree_node_id = navigation_handle->GetFrameTreeNodeId();
-
-  if (!navigation_handle->HasCommitted() ||
-      navigation_handle->IsSameDocument() || navigation_handle->IsErrorPage()) {
-    // We're not interested in tracking this navigation. In case we've seen a
-    // resource for the navigation before this message, clear it from
-    // ongoing_navigation_resources_.
-    ongoing_navigation_resources_.erase(frame_tree_node_id);
-    return CONTINUE_OBSERVING;
-  }
-
   content::RenderFrameHost* parent_frame_host =
-      navigation_handle->GetRenderFrameHost()->GetParent();
-  DCHECK(parent_frame_host);
-
-  bool top_level_subframe = !parent_frame_host->GetParent();
+      navigation_handle->GetParentFrame();
 
   const auto& id_and_data = ad_frames_data_.find(frame_tree_node_id);
   if (id_and_data != ad_frames_data_.end()) {
@@ -107,7 +108,6 @@ AdsPageLoadMetricsObserver::OnDidFinishSubFrameNavigation(
             "PageLoad.Clients.Ads.Google.Navigations.AdFrameRenavigatedToAd",
             FrameIsAd(navigation_handle));
       }
-
       return CONTINUE_OBSERVING;
     }
     // This frame was previously not an ad, process it as usual. If it had
@@ -115,7 +115,7 @@ AdsPageLoadMetricsObserver::OnDidFinishSubFrameNavigation(
     UMA_HISTOGRAM_BOOLEAN(
         "PageLoad.Clients.Ads.Google.Navigations.NonAdFrameRenavigatedToAd",
         FrameIsAd(navigation_handle));
-  } else if (top_level_subframe) {
+  } else if (navigation_handle->IsParentMainFrame()) {
     top_level_subframe_count_ += 1;
   }
 
@@ -133,7 +133,7 @@ AdsPageLoadMetricsObserver::OnDidFinishSubFrameNavigation(
 
   ad_frames_data_[frame_tree_node_id] = ad_data;
 
-  if (top_level_subframe && ad_data)
+  if (navigation_handle->IsParentMainFrame() && ad_data)
     top_level_ad_frame_count_ += 1;
 
   ProcessOngoingNavigationResource(frame_tree_node_id);
@@ -176,12 +176,13 @@ void AdsPageLoadMetricsObserver::ProcessLoadedResource(
   const auto& id_and_data =
       ad_frames_data_.find(extra_request_info.frame_tree_node_id);
   if (id_and_data == ad_frames_data_.end()) {
-    // This resouce is for a frame that hasn't yet committed. It must be the
-    // main document for the frame. Hold onto it and once it commits we'll run
-    // it in ProcessOngoingNavigationResource.
-    // TODO(jkarlin): Plumb the resource type through and DCHECK that the type
-    // is document.
-    auto it_and_success = ongoing_navigation_resources_.emplace(
+    // This resource is for a frame that hasn't ever finished a navigation. We
+    // expect it to be the frame's main resource but don't have enough
+    // confidence in that to dcheck it. For example, there might be races
+    // between doc.written resources and navigation failure.
+    // TODO(jkarlin): Add UMA to measure how often we see multiple resources
+    // for a frame before navigation finishes.
+    ongoing_navigation_resources_.emplace(
         std::piecewise_construct,
         std::forward_as_tuple(extra_request_info.frame_tree_node_id),
         std::forward_as_tuple(
@@ -189,7 +190,6 @@ void AdsPageLoadMetricsObserver::ProcessLoadedResource(
             extra_request_info.was_cached, extra_request_info.raw_body_bytes,
             extra_request_info.original_network_content_length, nullptr,
             extra_request_info.resource_type));
-    DCHECK(it_and_success.second);
     return;
   }