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 91080b116cf0a9afaf03a9dea66c5477915b56d5..19d10906f172604c5649fd29e60549ed05fd4fb0 100644
--- a/content/renderer/render_view_browsertest.cc
+++ b/content/renderer/render_view_browsertest.cc
@@ -58,6 +58,7 @@
 #include "content/shell/browser/shell.h"
 #include "content/shell/browser/shell_browser_context.h"
 #include "content/test/mock_keyboard.h"
+#include "content/test/mock_weburlloader.h"
 #include "content/test/test_render_frame.h"
 #include "net/base/net_errors.h"
 #include "net/cert/cert_status_flags.h"
@@ -97,6 +98,7 @@
 
 #include "url/url_constants.h"
 
+using base::TimeDelta;
 using blink::WebFrame;
 using blink::WebFrameContentDumper;
 using blink::WebInputEvent;
@@ -189,6 +191,22 @@ FrameReplicationState ReconstructReplicationStateForTesting(
   return result;
 }
 
+// Returns CommonNavigationParams for a normal navigation to a data: url, with
+// navigation_start set to Now() plus the given offet.

[nasko, 2016/07/18 22:20:00]

nit: "offset"

[Alexander Semashko, 2016/07/19 10:59:07]

Done.

+CommonNavigationParams MakeCommonNavigationParams(
+    TimeDelta navigation_start_offset) {
+  CommonNavigationParams params;
+  params.url = GURL("data:text/html,<div>Page</div>");
+  params.navigation_start = base::TimeTicks::Now() + navigation_start_offset;
+  params.navigation_type = FrameMsg_Navigate_Type::NORMAL;
+  params.transition = ui::PAGE_TRANSITION_TYPED;
+  return params;
+}
+
+blink::WebURLLoader* CreateMockURLLoader() {
+  return new testing::NiceMock<MockWebURLLoader>();
+}
+
 }  // namespace
 
 class RenderViewImplTest : public RenderViewTest {
@@ -247,7 +265,9 @@ class RenderViewImplTest : public RenderViewTest {
     const IPC::Message* message =
         render_thread_->sink().GetUniqueMessageMatching(T::ID);
     typename T::Param param;
-    T::Read(message, &param);
+    EXPECT_TRUE(message);
+    if (message)
+      T::Read(message, &param);
     return param;
   }
 
@@ -376,6 +396,16 @@ class RenderViewImplTest : public RenderViewTest {
         PageMsg_SetZoomLevel_Command::USE_CURRENT_TEMPORARY_MODE, level);
   }
 
+  // Closes a view created during the test, i.e. not the |view()|. Checks that
+  // the main frame is detached and deleted, and makes sure the view does not
+  // leak.
+  void CloseRenderView(RenderViewImpl* new_view) {
+    new_view->Close();
+    EXPECT_FALSE(new_view->GetMainRenderFrame());
+    // Clean up so we don't leak it.
+    new_view->Release();
+  }
+
  private:
   std::unique_ptr<MockKeyboard> mock_keyboard_;
 };
@@ -539,12 +569,8 @@ TEST_F(RenderViewImplTest, RenderFrameClearedAfterClose) {
       blink::WebNavigationPolicyNewForegroundTab, false);
   RenderViewImpl* new_view = RenderViewImpl::FromWebView(new_web_view);
 
-  // Close the view, causing the main RenderFrame to be detached and deleted.
-  new_view->Close();
-  EXPECT_FALSE(new_view->GetMainRenderFrame());
-
-  // Clean up after the new view so we don't leak it.
-  new_view->Release();
+  // Checks that the frame is deleted properly and cleans up the view.
+  CloseRenderView(new_view);
 }
 
 // Test that we get form state change notifications when input fields change.
@@ -821,9 +847,7 @@ TEST_F(RenderViewImplTest, DecideNavigationPolicyForWebUI) {
       decidePolicyForNavigation(popup_policy_info);
   EXPECT_EQ(blink::WebNavigationPolicyIgnore, policy);
 
-  // Clean up after the new view so we don't leak it.
-  new_view->Close();
-  new_view->Release();
+  CloseRenderView(new_view);
 }
 
 // Verify that security origins are replicated properly to RenderFrameProxies
@@ -951,8 +975,7 @@ TEST_F(RenderViewImplTest, PaintAfterSwapOut) {
   // Simulate getting painted after swapping out.
   new_view->DidFlushPaint();
 
-  new_view->Close();
-  new_view->Release();
+  CloseRenderView(new_view);
 }
 
 // Verify that the renderer process doesn't crash when device scale factor
@@ -2038,19 +2061,36 @@ TEST_F(RenderViewImplTest, OnSetAccessibilityMode) {
   ASSERT_TRUE(frame()->render_accessibility());
 }
 
+TEST_F(RenderViewImplTest, RendererNavigationStartTransmittedToBrowser) {
+  base::TimeTicks lower_bound_navigation_start(base::TimeTicks::Now());
+  frame()->GetWebFrame()->loadHTMLString(
+      "hello world", blink::WebURL(GURL("data:text/html,")));
+
+  FrameHostMsg_DidStartProvisionalLoad::Param host_nav_params =
+      ProcessAndReadIPC<FrameHostMsg_DidStartProvisionalLoad>();
+  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, BrowserNavigationStart) {
+  auto common_params = MakeCommonNavigationParams(-TimeDelta::FromSeconds(1));
+
+  frame()->Navigate(common_params, StartNavigationParams(),
+                    RequestNavigationParams());
+  FrameHostMsg_DidStartProvisionalLoad::Param nav_params =
+      ProcessAndReadIPC<FrameHostMsg_DidStartProvisionalLoad>();
+  EXPECT_EQ(common_params.navigation_start, std::get<1>(nav_params));
+}
+
 // 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);
+  auto late_common_params = MakeCommonNavigationParams(TimeDelta::FromDays(42));
   late_common_params.method = "POST";
 
   frame()->Navigate(late_common_params, StartNavigationParams(),
@@ -2064,20 +2104,84 @@ TEST_F(RenderViewImplTest, NavigationStartOverride) {
   EXPECT_LE(late_nav_reported_start, after_navigation);
 }
 
-TEST_F(RenderViewImplTest, RendererNavigationStartTransmittedToBrowser) {
-  base::TimeTicks lower_bound_navigation_start;
-  frame()->GetWebFrame()->loadHTMLString(
-      "hello world", blink::WebURL(GURL("data:text/html,")));
+// Checks that a browser-initiated navigation in an initial document that was
+// not accessed uses browser-side timestamp.
+TEST_F(RenderViewImplTest,
+       BrowserNavigationStartUsedIfInitialDocumentWasNotAccessed) {
+  blink_platform_impl_.SetWebURLLoaderFactory(base::Bind(&CreateMockURLLoader));
+  // Open a page that will be navigated later.
+  ExecuteJavaScriptForTests("window.open('https://google.com/');");
+  // The new page starts a provisional load (it will not commit).
   ProcessPendingMessages();
-  const IPC::Message* frame_navigate_msg =
-      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);
+  render_thread_->sink().ClearMessages();
+
+  auto* new_main_frame =
+      static_cast<TestRenderFrame*>(RenderFrame::FromRoutingID(
+          render_thread_->new_window_main_frame_routing_id()));
+  ASSERT_TRUE(new_main_frame);
+
+  auto common_params = MakeCommonNavigationParams(-TimeDelta::FromSeconds(1));
+  new_main_frame->Navigate(common_params, StartNavigationParams(),
+                           RequestNavigationParams());
+
+  FrameHostMsg_DidStartProvisionalLoad::Param nav_params =
+      ProcessAndReadIPC<FrameHostMsg_DidStartProvisionalLoad>();
+  EXPECT_EQ(common_params.navigation_start, std::get<1>(nav_params));
+
+  CloseRenderView(
+      RenderViewImpl::FromRoutingID(render_thread_->new_window_routing_id()));
+}
+
+// Checks that a browser-initiated navigation in an initial document that has
+// been accessed does not use browser-side timestamp (there may be arbitrary
+// content and/or scripts injected, including beforeunload handler that shows
+// a confirmation dialog).
+TEST_F(RenderViewImplTest,
+       BrowserNavigationStartDiscardedIfInitialDocumentWasAccessed) {
+  blink_platform_impl_.SetWebURLLoaderFactory(base::Bind(&CreateMockURLLoader));
+  // Open a page that will be navigated later.
+  ExecuteJavaScriptForTests(
+      "var w = window.open('https://google.com/');"
+      "w.document.body.appendChild(w.document.createElement('div'));");
+  // The new page starts a provisional load (it will not commit).
+  ProcessPendingMessages();
+  render_thread_->sink().ClearMessages();
+
+  auto* new_main_frame =
+      static_cast<TestRenderFrame*>(RenderFrame::FromRoutingID(
+          render_thread_->new_window_main_frame_routing_id()));
+  ASSERT_TRUE(new_main_frame);
+
+  auto common_params = MakeCommonNavigationParams(-TimeDelta::FromSeconds(1));
+  new_main_frame->Navigate(common_params, StartNavigationParams(),
+                           RequestNavigationParams());
+
+  FrameHostMsg_DidStartProvisionalLoad::Param nav_params =
+      ProcessAndReadIPC<FrameHostMsg_DidStartProvisionalLoad>();
+  EXPECT_GT(std::get<1>(nav_params), common_params.navigation_start);
+
+  CloseRenderView(
+      RenderViewImpl::FromRoutingID(render_thread_->new_window_routing_id()));
+}
+
+TEST_F(RenderViewImplTest,
+       FiringBeforeUnloadInSubFrameDiscardsBrowserNavigationStart) {
+  // Add a beforeunload handler in the initial document.
+  ExecuteJavaScriptForTests(
+      "var frame = document.createElement('iframe');"
+      "document.body.appendChild(frame);"
+      "frame.contentWindow.onbeforeunload = function() { return null; };");
+  // Need to drop the DidStartProvisionalLoad message from the child frame.
+  ProcessPendingMessages();
+  render_thread_->sink().ClearMessages();
+
+  auto common_params = MakeCommonNavigationParams(-TimeDelta::FromSeconds(1));
+  frame()->Navigate(common_params, StartNavigationParams(),
+                    RequestNavigationParams());
+
+  FrameHostMsg_DidStartProvisionalLoad::Param host_nav_params =
+      ProcessAndReadIPC<FrameHostMsg_DidStartProvisionalLoad>();
+  EXPECT_GT(std::get<1>(host_nav_params), common_params.navigation_start);
 }
 
 TEST_F(RenderViewImplTest, BrowserNavigationStartNotUsedForReload) {
@@ -2093,17 +2197,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 will
+  // be fired during Navigate.
   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();
@@ -2113,6 +2219,8 @@ TEST_F(RenderViewImplTest, BrowserNavigationStartNotUsedForHistoryNavigation) {
   render_thread_->sink().ClearMessages();
 
   // Go back.
+  // The browser navigation_start should not be used because beforeunload will
+  // be fired during GoToOffsetWithParams.
   CommonNavigationParams common_params_back;
   common_params_back.url =
       GURL("data:text/html;charset=utf-8,<div id=pagename>Page B</div>");
@@ -2126,6 +2234,8 @@ TEST_F(RenderViewImplTest, BrowserNavigationStartNotUsedForHistoryNavigation) {
   render_thread_->sink().ClearMessages();
 
   // Go forward.
+  // The browser navigation_start should not be used because beforeunload will
+  // be fired during GoToOffsetWithParams.
   CommonNavigationParams common_params_forward;
   common_params_forward.url =
       GURL("data:text/html;charset=utf-8,<div id=pagename>Page C</div>");
@@ -2138,14 +2248,20 @@ TEST_F(RenderViewImplTest, BrowserNavigationStartNotUsedForHistoryNavigation) {
                common_params_forward.navigation_start);
 }
 
-TEST_F(RenderViewImplTest, BrowserNavigationStartSuccessfullyTransmitted) {
-  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;
+TEST_F(RenderViewImplTest,
+       BrowserNavigationStartUsedForCrossProcessHistoryNavigation) {
+  auto common_params = MakeCommonNavigationParams(-TimeDelta::FromSeconds(1));
+  common_params.transition = ui::PAGE_TRANSITION_FORWARD_BACK;
 
-  frame()->Navigate(common_params, StartNavigationParams(),
-                    RequestNavigationParams());
+  RequestNavigationParams request_params;
+  request_params.page_state =
+      PageState::CreateForTesting(common_params.url, false, nullptr, nullptr);
+  request_params.page_id = 1;
+  request_params.nav_entry_id = 42;
+  request_params.pending_history_list_offset = 1;
+  request_params.current_history_list_offset = 0;
+  request_params.current_history_list_length = 1;
+  frame()->Navigate(common_params, StartNavigationParams(), request_params);
 
   FrameHostMsg_DidStartProvisionalLoad::Param host_nav_params =
       ProcessAndReadIPC<FrameHostMsg_DidStartProvisionalLoad>();