Simplify MetricsWebContentsObserver::OnTimingUpdated

chrome/browser/page_load_metrics/metrics_web_contents_observer.cc

Index: chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
diff --git a/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc b/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
index e3f51bf4946a1b2cc9437a0e973f4b77a78e5325..5d59ba4efb20d98dee6b7f252aa3806e3fb9cdda 100644
--- a/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
+++ b/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
@@ -525,11 +525,18 @@ void MetricsWebContentsObserver::OnTimingUpdated(
     content::RenderFrameHost* render_frame_host,
     const PageLoadTiming& timing,
     const PageLoadMetadata& metadata) {
+  if (render_frame_host->GetParent() != nullptr) {

[nasko, 2017/04/04 22:18:14]

Dumping just a bit of context from chat with csharrison@. The check as it is can
receive data from a case that is a bit strange and I'm not sure how or whether
it has impact on these metrics. The case is:

* Main frame starts navigation and commits.
* Subframe is navigated and commits in RFH_1, but does not complete the load.
* Same subframe navigates again cross-process (possible with out-of-process
iframes) and commits in RFH_2, causing the old RFH_1 to be cleaned up.
* As part of its cleanup, RFH_1 sends timing data from the renderer.

In this scenario, the code before would've ignored that IPC, since the for the
subframe the rfh->GetRenderViewHost()->GetMainFrame() will return nullptr. That
is obviously different than the WebContents::GetMainFrame(), so it would have
returned early.

With the new code, the IPC sent during RFH_1 cleanup (if such an IPC is
possibile) will actually be processed here.

csharrison@ indicated in chat that this is fine and the CL can be committed as
is, but I'm documenting it for historical record.

[Bryan McQuade, 2017/04/06 18:46:33]

Thanks for this. It's helpful to know.

I'm assuming that, in addition to RFH_1 having
rfh->GetRenderViewHost()->GetMainFrame() returning nullptr, traversing the
parent chain of RFH_1 to find the topmost frame would similarly not find the
main frame's RFH because RFH_1 has been removed from the frame hierarchy. Is
that right?

+    // Child frames may send PageLoadMetadata updates, but not PageLoadTiming
+    // updates.
+    if (!timing.IsEmpty())
+      RecordInternalError(ERR_TIMING_IPC_FROM_SUBFRAME);
+    committed_load_->UpdateChildFrameMetadata(metadata);
+    return;
+  }
+
   // We may receive notifications from frames that have been navigated away
   // from. We simply ignore them.
-  if (!render_frame_host->GetRenderViewHost() ||
-      render_frame_host->GetRenderViewHost()->GetMainFrame() !=
-          web_contents()->GetMainFrame()) {
+  if (render_frame_host != web_contents()->GetMainFrame()) {
     RecordInternalError(ERR_IPC_FROM_WRONG_FRAME);
     return;
   }
@@ -552,15 +559,6 @@ void MetricsWebContentsObserver::OnTimingUpdated(
   if (error)
     return;
 
-  if (render_frame_host->GetParent() != nullptr) {
-    // Child frames may send PageLoadMetadata updates, but not PageLoadTiming
-    // updates.
-    if (!timing.IsEmpty())
-      RecordInternalError(ERR_TIMING_IPC_FROM_SUBFRAME);
-    committed_load_->UpdateChildFrameMetadata(metadata);
-    return;
-  }
-
   if (!committed_load_->UpdateTiming(timing, metadata)) {
     // If the page load tracker cannot update its timing, something is wrong
     // with the IPC (it's from another load, or it's invalid in some other way).