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 64556e6969084e2caa7e133c395db71bbe7633f2..2633119e568833da87e77bc9c9ff07c2c051bfb8 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 offset.
+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.

[clamy, 2016/08/17 13:03:55]

nit: blank line above.
nit: avoid using we in comments.

[Alexander Semashko, 2016/08/17 17:10:25]

Ok. I just removed the comment since it duplicates the comment above the
function.

+    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
@@ -2023,19 +2046,36 @@ TEST_F(RenderViewImplTest, OnSetAccessibilityMode) {
   ASSERT_TRUE(frame()->render_accessibility());
 }
 
+TEST_F(RenderViewImplTest, RendererNavigationStartTransmittedToBrowser) {

[clamy, 2016/08/17 13:03:55]

Could you add comments explaining what the test does? It's not clear to me what
it is doing there.

[Alexander Semashko, 2016/08/17 17:10:25]

Done.

+  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) {

[clamy, 2016/08/17 13:03:55]

Isn't this test testing that the browser navigation start is used if the initial
document was not accessed? If so, can we remove
BrowserNavigationStartUsedIfInitialDocumentWasNotAccessed, which looks a lot
more complicated?

If this the case, can we also make a similar test for initial document access
where we just access the document of the current frame?

[Alexander Semashko, 2016/08/17 17:10:25]

Assuming that these tests will always start with an untouched initial doc, it's
fine to remove. Done.
This means relying on that test-only methods for executing script will always
trigger didAccessInitialDocument, and I'm somewhat wary about this. Currently
this test mimics the real-world cases closer. Are you sure it's ok to simplify
the test?

[clamy, 2016/08/24 23:23:09]

Well it was working when you just added a subframe to the testing frame. Also
your current version relies on maintaining testing code (for routing ids) that
is not used outside of that test.

[Alexander Semashko, 2016/08/25 10:44:33]

Yeah, it is working currently. To elaborate more on my concern: we can replace
the current test with a simpler one, but also, I think, less reliable; if the
simplified version breaks in some future moment, there is a high chance that the
test will be disabled/deleted instead of digging and reinventing a complex setup
that makes it working.
Well, I just added setters for values that are implicitly used in many tests.
IMO it is a tiny bit that makes the API more complete and it's better to keep
these setters in any case.

To be clear, these are not strong objections in any way, just considerations;
I'll change the CL whatever way you think suitable.
Just waiting for your decision.

[clamy, 2016/08/25 18:00:14]

Your test is on the commit queue, which means it can only breaks if someone
explicitly disables it in order to land their patch. It is against Chromium
policy to do so: we only disable tests if they are flaky (then we file bugs to
unflake them) or if what they are testing is no longer relevant. So as long as
we want our code to follow the behavior this test is testing, it should remain
unbroken. I do think that the simpler version is easier to understand, an
requires less changes to the testing code. So I would prefer for you to use this
version.

[Alexander Semashko, 2016/08/25 21:32:10]

I meant major refactorings, where it sometimes is hard to tell whether a test is
still relevant, and some mistakes just slip through... Nevermind.

Ok, this test is now as simple as it can be.
Note that the part with routing ids is still in the CL (since you did not say
anything about it).

[clamy, 2016/08/25 22:24:29]

If the routing id part is not used in the tests, then it should be removed from
the CL.

[Alexander Semashko, 2016/08/25 22:50:19]

Done.

+  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(),
@@ -2049,20 +2089,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) {

[clamy, 2016/08/17 13:03:55]

This doesn't seem correct to me: based on how the code is written, if we haven't
yet navigated anywhere we won't fire the BeforeUnload event unless the document
was accessed. So this similar to testing whether the initial document was
accessed or not.

[Alexander Semashko, 2016/08/17 17:10:25]

Well, now the test name does not reflect what is happening, I just forgot to
remove it.
It is passing because ExecuteJavaScriptForTests triggers
didAccessInitialDocument in the main frame.

+  // 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) {

[clamy, 2016/08/17 13:03:55]

Can you rename it NavigationStartForReload?

[Alexander Semashko, 2016/08/17 17:10:25]

Done.

@@ -2078,17 +2182,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);

[clamy, 2016/08/17 13:03:55]

Can you do the following:
if (!IsBrowserSideNavigationEnabled()) {
  // The browser navigation_start should not be used because beforeunload   will
be fired during Navigate.
  EXPECT_PRED2(...
} else {
  // PlzNavigate: the browser navigation_start is always used.
  EXPECT_EQ(common_params.navigation_start, std::get<1>(nav_params));
}

Same in the history test.

[Alexander Semashko, 2016/08/17 17:10:25]

Done.

 }
 
-TEST_F(RenderViewImplTest, BrowserNavigationStartNotUsedForHistoryNavigation) {
+TEST_F(RenderViewImplTest,
+       BrowserNavigationStartNotUsedForSameProcessHistoryNavigation) {

[clamy, 2016/08/17 13:03:55]

Please rename it NavigationStartForSameProcessHistoryNavigation.

[Alexander Semashko, 2016/08/17 17:10:25]

Done. Also renamed the cross-process counterpart and
BrowserNavigationStartDiscardedIfInitialDocumentWasAccessed.

   LoadHTML("<div id=pagename>Page A</div>");
   LoadHTML("<div id=pagename>Page B</div>");
   PageState back_state = GetCurrentPageState();
@@ -2098,6 +2204,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>");
@@ -2111,6 +2219,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>");
@@ -2123,14 +2233,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>();