Amend sanitization of browser-side navigationStart override.

content/renderer/render_frame_impl.cc

 // Copyright 2013 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 "content/renderer/render_frame_impl.h"
 
 #include <map>
 #include <string>
 
 #include "base/auto_reset.h"
@@ skipping 868 matching lines @@
       const char* data = NULL;
       if (params.browser_initiated_post_data.size()) {
         data = reinterpret_cast<const char*>(
             &params.browser_initiated_post_data.front());
       }
       http_body.appendData(
           WebData(data, params.browser_initiated_post_data.size()));
       request.setHTTPBody(http_body);
     }
 
+    // Record this before starting the load, we need a lower bound of this time
+    // to sanitize the navigationStart override set below.
+    base::TimeTicks renderer_navigation_start = base::TimeTicks::Now();
     frame->loadRequest(request);
 
     // The browser provides the navigation_start time to bootstrap the
     // Navigation Timing information for the browser-initiated navigations. In
     // case of cross-process navigations, this carries over the time of
     // finishing the onbeforeunload handler of the previous page.
     DCHECK(!params.browser_navigation_start.is_null());
     if (frame->provisionalDataSource()) {
-      // browser_navigation_start is likely before this process existed, so we
-      // can't use InterProcessTimeTicksConverter. Instead, the best we can do
-      // is just ensure we don't report a bogus value in the future.
+      // |browser_navigation_start| is likely before this process existed, so we
+      // can't use InterProcessTimeTicksConverter. We need at least to ensure
+      // that the browser-side navigation start we set is not later than the one
+      // on the renderer side.
       base::TimeTicks navigation_start = std::min(

[pasko, 2014/07/17 13:18:00]

This min() could be a reason of bi-modal distribution in navigation metrics,
which makes it quite hard to analyse the metric with statistical methods.

If we record the time upon sending and receiving the IPC in the corresponding
processes as |browser_sent_navigation| and |renderer_received_navigation|, then
we can account for this offset in renderer-side navigation timings.

Proposal:

navigation_start = renderer_received_navigation - (browser_sent_navigation -
params.browser_navigation_start)

This way we will be discounting for the IPC travel time, which is not ideal, but
I like thinking that IPC travel time depends more on how the OS behaves and is
somewhat out of our control. I have no better way in mind for synchronizing time
across processes, but this min() time feels confusing, non-monotonic and
<censored>.

[ppi, 2014/07/17 13:27:43]

This is an excellent idea, also solving the problem of big clock differences
between processes on Windows. Let me file a bug and let's see what the other
people think.

Thanks!!

-          base::TimeTicks::Now(), params.browser_navigation_start);
+          params.browser_navigation_start, renderer_navigation_start);
       double navigation_start_seconds =
           (navigation_start - base::TimeTicks()).InSecondsF();
       frame->provisionalDataSource()->setNavigationStartTime(
           navigation_start_seconds);
     }
   }
 
   // In case LoadRequest failed before DidCreateDataSource was called.
   render_view_->pending_navigation_params_.reset();
 }
@@ skipping 2666 matching lines @@
 
 #if defined(ENABLE_BROWSER_CDMS)
 RendererCdmManager* RenderFrameImpl::GetCdmManager() {
   if (!cdm_manager_)
     cdm_manager_ = new RendererCdmManager(this);
   return cdm_manager_;
 }
 #endif  // defined(ENABLE_BROWSER_CDMS)
 
 }  // namespace content