Fix for the NavigationControllerTest.BackSubframe test failures with PlzNavigate

content/browser/frame_host/navigation_controller_impl_unittest.cc

Index: content/browser/frame_host/navigation_controller_impl_unittest.cc
diff --git a/content/browser/frame_host/navigation_controller_impl_unittest.cc b/content/browser/frame_host/navigation_controller_impl_unittest.cc
index b01157bf13be2272044ab388e6344253a3bf35f3..6acff34e861f9f4a53dae1cecaab7c7f8c740d06 100644
--- a/content/browser/frame_host/navigation_controller_impl_unittest.cc
+++ b/content/browser/frame_host/navigation_controller_impl_unittest.cc
@@ -2563,7 +2563,13 @@ TEST_F(NavigationControllerTest, BackSubframe) {
   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;
@@ -2583,7 +2589,9 @@ TEST_F(NavigationControllerTest, BackSubframe) {
   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;