Set navigationStart correctly for all load types.

content/renderer/render_view_browsertest.cc

 // Copyright (c) 2012 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 <stddef.h>
 #include <stdint.h>
 #include <tuple>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 2021 matching lines @@
 
   frame()->SetAccessibilityMode(AccessibilityModeOff);
   ASSERT_EQ(AccessibilityModeOff, frame()->accessibility_mode());
   ASSERT_FALSE(frame()->render_accessibility());
 
   frame()->SetAccessibilityMode(AccessibilityModeComplete);
   ASSERT_EQ(AccessibilityModeComplete, frame()->accessibility_mode());
   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().
   CommonNavigationParams late_common_params;
   late_common_params.url = GURL("data:text/html,<div>Another page</div>");
   late_common_params.navigation_type = FrameMsg_Navigate_Type::NORMAL;
   late_common_params.transition = ui::PAGE_TRANSITION_TYPED;
   late_common_params.navigation_start =
       base::TimeTicks::Now() + base::TimeDelta::FromDays(42);
   late_common_params.method = "POST";
@@ skipping 18 matching lines @@
       render_thread_->sink().GetUniqueMessageMatching(
           FrameHostMsg_DidStartProvisionalLoad::ID);
   FrameHostMsg_DidStartProvisionalLoad::Param host_nav_params;
   FrameHostMsg_DidStartProvisionalLoad::Read(frame_navigate_msg,
                                                      &host_nav_params);
   base::TimeTicks transmitted_start = std::get<1>(host_nav_params);
   EXPECT_FALSE(transmitted_start.is_null());
   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.
   LoadHTML(url_string);
   ProcessPendingMessages();
   render_thread_->sink().ClearMessages();
 
   CommonNavigationParams common_params;
   common_params.url = GURL(url_string);
   common_params.navigation_type =
       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();
   LoadHTML("<div id=pagename>Page C</div>");
   PageState forward_state = GetCurrentPageState();
   ProcessPendingMessages();
   render_thread_->sink().ClearMessages();
 
   // Go back.
   CommonNavigationParams common_params_back;
@@ skipping 378 matching lines @@
   ExpectPauseAndResume(3);
   blink::WebScriptSource source2(
       WebString::fromUTF8("function func2() { func1(); }; func2();"));
   frame()->GetWebFrame()->executeScriptInIsolatedWorld(17, &source2, 1, 1);
 
   EXPECT_FALSE(IsPaused());
   Detach();
 }
 
 }  // namespace content