Set navigationStart correctly for all load types.

content/renderer/render_view_browsertest.cc

Index: content/renderer/render_view_browsertest.cc
diff --git a/content/renderer/render_view_browsertest.cc b/content/renderer/render_view_browsertest.cc
index 57747020cd76fa2e412df893ef676a95cfbfb620..3e032ae657eb216a1bf9b592cae18ecea3368d16 100644
--- a/content/renderer/render_view_browsertest.cc
+++ b/content/renderer/render_view_browsertest.cc
@@ -2039,10 +2039,33 @@ TEST_F(RenderViewImplTest, OnSetAccessibilityMode) {
   ASSERT_TRUE(frame()->render_accessibility());
 }
 
+TEST_F(RenderViewImplTest, BrowserNavigationStart) {
+  base::TimeTicks browser_navigation_start =
+      base::TimeTicks::Now() - base::TimeDelta::FromSeconds(1);
+  CommonNavigationParams common_params;
+  common_params.url = GURL("data:text/html,<div>Page</div>");
+  common_params.navigation_type = FrameMsg_Navigate_Type::NORMAL;
+  common_params.transition = ui::PAGE_TRANSITION_TYPED;
+  common_params.navigation_start = browser_navigation_start;
+
+  frame()->Navigate(common_params, StartNavigationParams(),
+                    RequestNavigationParams());
+  ProcessPendingMessages();
+
+  blink::WebDataSource* ds = frame()->GetWebFrame()->dataSource();
+  ASSERT_TRUE(ds);
+  DocumentState* document_state = DocumentState::FromDataSource(ds);

[Charlie Harrison, 2016/06/28 14:01:43]

Getting the navigation start from the document_state is a bit of an
implementation detail. Do you mind writing this test so we get it from the more
public APIs like IPC or WebPerformance?

[Alexander Semashko, 2016/06/28 14:47:45]

It's ok to take it from the IPC (to avoid comparing after lossy conversions),
but I think it would be better to move the
RendererNavigationStartTransmittedToBrowser test above it then (to keep the
tests in a natural order), WDYT?

[Charlie Harrison, 2016/06/28 15:54:15]

Sounds good to me.

+  base::TimeTicks navigation_start =
+      static_cast<NavigationStateImpl*>(document_state->navigation_state())
+          ->common_params().navigation_start;
+
+  EXPECT_EQ(browser_navigation_start, navigation_start);
+}
+
 // Sanity check for the Navigation Timing API |navigationStart| override. We
 // are asserting only most basic constraints, as TimeTicks (passed as the
 // override) are not comparable with the wall time (returned by the Blink API).
-TEST_F(RenderViewImplTest, NavigationStartOverride) {
+TEST_F(RenderViewImplTest, BrowserNavigationStartSanitized) {
   // Verify that a navigation that claims to have started in the future - 42
   // days from now is *not* reported as one that starts in the future; as we
   // sanitize the override allowing a maximum of ::Now().
@@ -2081,6 +2104,32 @@ TEST_F(RenderViewImplTest, RendererNavigationStartTransmittedToBrowser) {
   EXPECT_LT(lower_bound_navigation_start, transmitted_start);
 }
 
+TEST_F(RenderViewImplTest, FiringBeforeUnloadDiscardsBrowserNavigationStart) {
+  const char url_string[] = "data:text/html,<script>"
+                            "window.onbeforeunload = function() {"
+                            "  var finishTime = new Date().getTime() + 100;"
+                            "  while (new Date() < finishTime) {}"
+                            "  return null;"
+                            "};"
+                            "</script>";
+  LoadHTML(url_string);
+  ProcessPendingMessages();
+  render_thread_->sink().ClearMessages();
+
+  CommonNavigationParams common_params;
+  common_params.url = GURL("data:text/html,<div>Another page</div>");
+  common_params.navigation_type = FrameMsg_Navigate_Type::NORMAL;
+  common_params.transition = ui::PAGE_TRANSITION_TYPED;
+
+  frame()->Navigate(common_params, StartNavigationParams(),
+                    RequestNavigationParams());
+
+  FrameHostMsg_DidStartProvisionalLoad::Param host_nav_params =
+      ProcessAndReadIPC<FrameHostMsg_DidStartProvisionalLoad>();
+  EXPECT_PRED2(TimeTicksGT, std::get<1>(host_nav_params),
+               common_params.navigation_start);
+}
+
 TEST_F(RenderViewImplTest, BrowserNavigationStartNotUsedForReload) {
   const char url_string[] = "data:text/html,<div>Page</div>";
   // Navigate once, then reload.
@@ -2094,17 +2143,19 @@ TEST_F(RenderViewImplTest, BrowserNavigationStartNotUsedForReload) {
       FrameMsg_Navigate_Type::RELOAD_ORIGINAL_REQUEST_URL;
   common_params.transition = ui::PAGE_TRANSITION_RELOAD;
 
+  // The browser navigation_start should not be used because beforeunload was
+  // not fired yet.

[Charlie Harrison, 2016/06/28 14:01:43]

This comment is unclear. If we're getting the DidStartProvisionalLoad IPC then
we probably should have fired beforeunload at some point. Either the comment
should change or the test should change.

[Alexander Semashko, 2016/06/28 14:47:45]

I meant that it is not fired at the moment where the comment is (before the call
to Navigate). I'll rephrase the comment.

   frame()->Navigate(common_params, StartNavigationParams(),
                     RequestNavigationParams());
 
   FrameHostMsg_DidStartProvisionalLoad::Param host_nav_params =
       ProcessAndReadIPC<FrameHostMsg_DidStartProvisionalLoad>();
-  // The true timestamp is later than the browser initiated one.
   EXPECT_PRED2(TimeTicksGT, std::get<1>(host_nav_params),
                common_params.navigation_start);
 }
 
-TEST_F(RenderViewImplTest, BrowserNavigationStartNotUsedForHistoryNavigation) {
+TEST_F(RenderViewImplTest,
+       BrowserNavigationStartNotUsedForSameProcessHistoryNavigation) {
   LoadHTML("<div id=pagename>Page A</div>");
   LoadHTML("<div id=pagename>Page B</div>");
   PageState back_state = GetCurrentPageState();