Fix wrong RenderView history.

This fixes https://crbug.com/796561 and adds a regression test.

The issue was a race between the browser and renderer processes while
updating the renderer's history.

The renderer's history is updated in
* RenderFrameImpl::CommitNavigation and in
* RenderFrameImpl::DidCommitProvisionalLoad.

 ┌────────┐                  ┌───────┐
 │Renderer│                  │Browser│
 └───┬────┘                  └───┬───┘
     │  BeginNavigation          │
     │   - url=iframe.html       │
     │   - history_offset = 0    │
     │   - history_length = 1    │
1)   │ ──────────────────────────> ┐
     │                           │ │
     │ DidCommitProvisionalLoad  │ │
     │  - "pushState"            │ │
     │  - history_offset = 1     │ │
     │  - history_length = 2     │ │
2)   │ ──────────────────────────> │
     │                           │ │
     │ CommitNavigation          │ │
     │   - url=iframe.html       │ │
     │   - history_offset = 0    │ │
     │   - history_length = 1    │ │
   ┌─│ <───────────────────────────┘
   │ │                           │
   │ │ DidCommitProvisionalLoad  │
   │ │  - url=iframe.hmtl        │
   │ │  - history_offset = 0     │
   │ │  - history_length = 1     │
   └>│ ──────────────────────────>
 ┌───┴────┐                  ┌───┴───┐
 │Renderer│                  │Browser│
 └────────┘                  └───────┘

What happens in the bug.
1) An iframe is created: [history.offset = 0, history.size = 1].
2) history.pushState: history is updated [offset = 1, size = 2].
3) CommitNavigation is received, history is updated using the old
   values transmitted in 1) with BeginNavigation.

This CL update the values sent in CommitNavigation. This reduce
considerably the duration when the race can happen.

Before this CL, the race could happen if a history.pushState is sent in
between BeginNavigation and CommitNavigation (~300ms <-> severals
seconds).  After this CL, the race happens if the history.pushState is
sent between when the CommitNavigation message is sent and when it is
received by the renderer process.

Bug: 796561
Change-Id: I7bf72fadbf9f4946da28126a394adddf051f49a8
Reviewed-on: https://chromium-review.googlesource.com/881171Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533233}

content/browser/browser_side_navigation_browsertest.cc

@@ -800,4 +800,53 @@ IN_PROC_BROWSER_TEST_F(BrowserSideNavigationBrowserTest,
   EXPECT_EQ("#foo", reference_fragment);
 }
 
+// Regression test for https://crbug.com/796561.
+// 1) Start on a document with history.length == 1.
+// 2) Create an iframe and call history.pushState at the same time.
+// 3) history.back() must work.
+IN_PROC_BROWSER_TEST_F(BrowserSideNavigationBrowserTest,
+                       IframeAndPushStateSimultaneously) {
+  GURL main_url = embedded_test_server()->GetURL("/simple_page.html");
+  GURL iframe_url = embedded_test_server()->GetURL("/hello.html");
+
+  // 1) Start on a new document such that history.length == 1.
+  {
+    EXPECT_TRUE(NavigateToURL(shell(), main_url));
+
+    int history_length;
+    EXPECT_TRUE(ExecuteScriptAndExtractInt(
+        shell(), "window.domAutomationController.send(history.length)",
+        &history_length));
+    EXPECT_EQ(1, history_length);
+  }
+
+  // 2) Create an iframe and call history.pushState at the same time.
+  {
+    TestNavigationManager iframe_navigation(shell()->web_contents(),
+                                            iframe_url);
+    ExecuteScriptAsync(shell(),
+                       "let iframe = document.createElement('iframe');"
+                       "iframe.src = '/hello.html';"
+                       "document.body.appendChild(iframe);");
+    EXPECT_TRUE(iframe_navigation.WaitForRequestStart());
+
+    // The iframe navigation is paused. In the meantime, a pushState navigation
+    // begins and ends.
+    TestNavigationManager push_state_navigation(shell()->web_contents(),
+                                                main_url);
+    ExecuteScriptAsync(shell(), "window.history.pushState({}, null);");
+    push_state_navigation.WaitForNavigationFinished();
+
+    // The iframe navigation is resumed.
+    iframe_navigation.WaitForNavigationFinished();
+  }
+
+  // 3) history.back() must work.
+  {
+    TestNavigationObserver navigation_observer(shell()->web_contents());
+    EXPECT_TRUE(ExecuteScript(shell()->web_contents(), "history.back();"));
+    navigation_observer.Wait();
+  }
+}
+
 }  // namespace content

content/browser/frame_host/navigation_request.cc

@@ -1297,6 +1297,7 @@ void NavigationRequest::OnWillProcessResponseChecksComplete(
 void NavigationRequest::CommitErrorPage(
     RenderFrameHostImpl* render_frame_host,
     const base::Optional<std::string>& error_page_content) {
+  UpdateRequestNavigationParamsHistory();
   frame_tree_node_->TransferNavigationRequestOwnership(render_frame_host);
   navigation_handle_->ReadyToCommitNavigation(render_frame_host);
   render_frame_host->FailedNavigation(common_params_, request_params_,
@@ -1305,6 +1306,7 @@ void NavigationRequest::CommitErrorPage(
 }
 
 void NavigationRequest::CommitNavigation() {
+  UpdateRequestNavigationParamsHistory();
   DCHECK(response_ || !IsURLHandledByNetworkStack(common_params_.url) ||
          navigation_handle_->IsSameDocument());
   DCHECK(!common_params_.url.SchemeIs(url::kJavaScriptScheme));
@@ -1452,4 +1454,13 @@ NavigationRequest::CheckLegacyProtocolInSubresource() const {
   return LegacyProtocolInSubresourceCheckResult::BLOCK_REQUEST;
 }
 
+void NavigationRequest::UpdateRequestNavigationParamsHistory() {
+  NavigationController* navigation_controller =
+      frame_tree_node_->navigator()->GetController();
+  request_params_.current_history_list_offset =
+      navigation_controller->GetCurrentEntryIndex();
+  request_params_.current_history_list_length =
+      navigation_controller->GetEntryCount();
+}
+
 }  // namespace content

content/browser/frame_host/navigation_request.h

@@ -294,6 +294,11 @@ class CONTENT_EXPORT NavigationRequest : public NavigationURLLoaderDelegate {
   LegacyProtocolInSubresourceCheckResult CheckLegacyProtocolInSubresource()
       const;
 
+  // Called before a commit. Updates the history index and length held in
+  // RequestNavigationParams. This is used to update this shared state with the
+  // renderer process.
+  void UpdateRequestNavigationParamsHistory();
+
   FrameTreeNode* frame_tree_node_;
 
   // Initialized on creation of the NavigationRequest. Sent to the renderer when

content/renderer/render_frame_impl.cc

@@ -5521,7 +5521,10 @@ bool RenderFrameImpl::UpdateNavigationHistory(
   current_history_item_.SetTarget(
       blink::WebString::FromUTF8(unique_name_helper_.value()));
   bool is_new_navigation = commit_type == blink::kWebStandardCommit;
-  if (is_new_navigation) {
+  if (request_params.should_clear_history_list) {
+    render_view_->history_list_offset_ = 0;
+    render_view_->history_list_length_ = 1;
+  } else if (is_new_navigation) {
     DCHECK(!navigation_state->common_params().should_replace_current_entry ||
            render_view_->history_list_length_ > 0);
     if (!navigation_state->common_params().should_replace_current_entry) {
@@ -6600,10 +6603,6 @@ void RenderFrameImpl::PrepareRenderViewForNavigation(
       request_params.current_history_list_offset;
   render_view_->history_list_length_ =
       request_params.current_history_list_length;
-  if (request_params.should_clear_history_list) {
-    CHECK_EQ(-1, render_view_->history_list_offset_);
-    CHECK_EQ(0, render_view_->history_list_length_);
-  }
 }
 
 void RenderFrameImpl::BeginNavigation(const NavigationPolicyInfo& info) {

content/renderer/render_view_browsertest.cc

@@ -73,6 +73,7 @@
 #include "third_party/WebKit/public/web/WebDeviceEmulationParams.h"
 #include "third_party/WebKit/public/web/WebDocumentLoader.h"
 #include "third_party/WebKit/public/web/WebFrameContentDumper.h"
+#include "third_party/WebKit/public/web/WebGlobalObjectReusePolicy.h"
 #include "third_party/WebKit/public/web/WebHistoryCommitType.h"
 #include "third_party/WebKit/public/web/WebHistoryItem.h"
 #include "third_party/WebKit/public/web/WebInputMethodController.h"
@@ -2224,19 +2225,59 @@ TEST_F(RenderViewImplTest, HistoryIsProperlyUpdatedOnNavigation) {
   EXPECT_EQ(0, view()->HistoryBackListCount() +
                    view()->HistoryForwardListCount() + 1);
 
-  // Receive a Navigate message with history parameters.
+  // Receive a CommitNavigation message with history parameters.
   RequestNavigationParams request_params;
-  request_params.current_history_list_length = 2;
   request_params.current_history_list_offset = 1;
-  request_params.pending_history_list_offset = 2;
+  request_params.current_history_list_length = 2;
   frame()->Navigate(CommonNavigationParams(), request_params);
 
-  // The history list in RenderView should have been updated.
+  // The current history list in RenderView is updated.
   EXPECT_EQ(1, view()->HistoryBackListCount());
   EXPECT_EQ(2, view()->HistoryBackListCount() +
                    view()->HistoryForwardListCount() + 1);
 }
 
+// Ensure the RenderViewImpl history list is properly updated when starting a
+// new history browser-initiated navigation.
+TEST_F(RenderViewImplTest, HistoryIsProperlyUpdatedOnHistoryNavigation) {
+  EXPECT_EQ(0, view()->HistoryBackListCount());
+  EXPECT_EQ(0, view()->HistoryBackListCount() +
+                   view()->HistoryForwardListCount() + 1);
+
+  // Receive a CommitNavigation message with history parameters.
+  RequestNavigationParams request_params;
+  request_params.current_history_list_offset = 1;
+  request_params.current_history_list_length = 25;
+  request_params.pending_history_list_offset = 12;
+  request_params.nav_entry_id = 777;
+  frame()->Navigate(CommonNavigationParams(), request_params);
+
+  // The current history list in RenderView is updated.
+  EXPECT_EQ(12, view()->HistoryBackListCount());
+  EXPECT_EQ(25, view()->HistoryBackListCount() +
+                    view()->HistoryForwardListCount() + 1);
+}
+
+// Ensure the RenderViewImpl history list is properly updated when starting a
+// new history browser-initiated navigation with should_clear_history_list
+TEST_F(RenderViewImplTest, HistoryIsProperlyUpdatedOnShouldClearHistoryList) {
+  EXPECT_EQ(0, view()->HistoryBackListCount());
+  EXPECT_EQ(0, view()->HistoryBackListCount() +
+                   view()->HistoryForwardListCount() + 1);
+
+  // Receive a CommitNavigation message with history parameters.
+  RequestNavigationParams request_params;
+  request_params.current_history_list_offset = 12;
+  request_params.current_history_list_length = 25;
+  request_params.should_clear_history_list = true;
+  frame()->Navigate(CommonNavigationParams(), request_params);
+
+  // The current history list in RenderView is updated.
+  EXPECT_EQ(0, view()->HistoryBackListCount());
+  EXPECT_EQ(1, view()->HistoryBackListCount() +
+                   view()->HistoryForwardListCount() + 1);
+}
+
 // IPC Listener that runs a callback when a console.log() is executed from
 // javascript.
 class ConsoleCallbackFilter : public IPC::Listener {

content/renderer/render_view_impl.cc

@@ -1211,8 +1211,10 @@ void RenderViewImpl::OnUpdateTargetURLAck() {
 
 void RenderViewImpl::OnSetHistoryOffsetAndLength(int history_offset,
                                                  int history_length) {
-  DCHECK_GE(history_offset, -1);
-  DCHECK_GE(history_length, 0);
+  // -1 <= history_offset < history_length <= kMaxSessionHistoryEntries(50).
+  DCHECK_LE(-1, history_offset);
+  DCHECK_LT(history_offset, history_length);
+  DCHECK_LE(history_length, kMaxSessionHistoryEntries);
 
   history_list_offset_ = history_offset;
   history_list_length_ = history_length;