Fix for the NavigationControllerTest.BackSubframe test failures with PlzNavigate

content/browser/frame_host/navigation_controller_impl_unittest.cc

 // Copyright 2013 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 "content/browser/frame_host/navigation_controller_impl.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <memory>
@@ skipping 2545 matching lines @@
   // Go back one.
   controller.GoBack();
   params.page_id = 2;
   params.nav_entry_id = entry2->GetUniqueID();
   params.frame_unique_name = unique_name;
   params.did_create_new_entry = false;
   params.url = url2;
   params.transition = ui::PAGE_TRANSITION_AUTO_SUBFRAME;
   params.page_state = PageState::CreateFromURL(url2);
   params.item_sequence_number = item_sequence_number2;
-  subframe->PrepareForCommit();
+  // If PlzNavigate is enabled, the TestRenderFrameHost::PrepareForCommit()
+  // function expects the navigation request to be available in the
+  // FrameTreeNode. This may not always be true. Please refer to the
+  // NavigationControllerImpl::NavigateToPendingEntryInternal() function for
+  // more information. The gist is we may run out of frames to navigate due to
+  // lack of available history or we are unable to match a frame by name.
+  subframe->PrepareForCommitIfNecessary();

[clamy, 2016/09/06 12:32:08]

This test used to work with PlzNavigate enabled. We should create a
NavigationRequest when asked to go back so this should not be an issue. Instead,
we should investigate why we don't actually start a navigation in the subframe.

Considering when it started failing, I suspect this is due to
https://codereview.chromium.org/2294343002 landing. In particular, this CL
changes the way in which the item & document sequence number are set up in the
FrameNavigationEntry. I think this is why we no longer set them up properly in
the unit tests, and because of that the history code interpretes the go-back
command as being a same-document navigation instead of a cross-document one.

[ananta, 2016/09/07 02:43:37]

Thanks for the tip. It does fail because of the change in the way the sequence
numbers are setup in the navigation entry. I added code in
FrameNavigationEntry::SetPageState to honor the existing values in the sequence
numbers if the exploded ones don't contain values greater than 0. That fixes the
problem.

For context in chrome, the page state is updated recursively using the Web
navigation state from the renderer and is sent to the browser via the
FrameHostMsg_UpdateState IPC message

Please check if this approach is reasonable for tests.

   subframe->SendNavigateWithParams(&params);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
   navigation_entry_committed_counter_ = 0;
   EXPECT_EQ(entry2, controller.GetLastCommittedEntry());
   EXPECT_EQ(3, controller.GetEntryCount());
   EXPECT_EQ(1, controller.GetCurrentEntryIndex());
   EXPECT_EQ(-1, controller.GetPendingEntryIndex());
   EXPECT_FALSE(controller.GetPendingEntry());
 
   // Go back one more.
   controller.GoBack();
   params.page_id = 1;
   params.nav_entry_id = entry1->GetUniqueID();
   params.frame_unique_name = unique_name;
   params.did_create_new_entry = false;
   params.url = subframe_url;
   params.transition = ui::PAGE_TRANSITION_AUTO_SUBFRAME;
   params.page_state = PageState::CreateFromURL(subframe_url);
   params.item_sequence_number = item_sequence_number1;
-  subframe->PrepareForCommit();
+  // Please refer to the comments for the test case above as to why we are
+  // using PrepareForCommitIfNecessary() instead of PrepareForCommit().
+  subframe->PrepareForCommitIfNecessary();
   subframe->SendNavigateWithParams(&params);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
   navigation_entry_committed_counter_ = 0;
   EXPECT_EQ(entry1, controller.GetLastCommittedEntry());
   EXPECT_EQ(3, controller.GetEntryCount());
   EXPECT_EQ(0, controller.GetCurrentEntryIndex());
   EXPECT_EQ(-1, controller.GetPendingEntryIndex());
   EXPECT_FALSE(controller.GetPendingEntry());
 }
 
@@ skipping 2721 matching lines @@
   // means that occasionally a navigation conflict will end up with one entry
   // bubbling to the end of the entry list, but that's the least-bad option.
   EXPECT_EQ(3, controller.GetEntryCount());
   EXPECT_EQ(2, controller.GetCurrentEntryIndex());
   EXPECT_EQ(url_a, controller.GetEntryAtIndex(0)->GetURL());
   EXPECT_EQ(url_c, controller.GetEntryAtIndex(1)->GetURL());
   EXPECT_EQ(url_b, controller.GetEntryAtIndex(2)->GetURL());
 }
 
 }  // namespace content