Don't update subframes on parent frame commits.

content/browser/frame_host/navigation_controller_impl_browsertest.cc

 // Copyright 2014 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 <stdint.h>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 3484 matching lines @@
   }
 
   // Make sure the renderer is still alive.
   EXPECT_TRUE(
       ExecuteScript(shell()->web_contents(), "console.log('Success');"));
 }
 
 // This tests that 1) the initial "about:blank" URL is elided from the
 // navigation history of a subframe when it is loaded, and 2) that that initial
 // "about:blank" returns if it is navigated to as part of a history navigation.
 // See http://crbug.com/542299 and https://github.com/whatwg/html/issues/546 .

[Charlie Reis, 2016/05/05 17:45:42]

// TODO(avi, creis): Find another fix for the NC_IN_PAGE_NAVIGATION crash.

[Avi (use Gerrit), 2016/05/05 19:06:21]

Done.

 IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
                        BackToAboutBlankIframe) {
   GURL original_url(embedded_test_server()->GetURL(
       "/navigation_controller/simple_page_1.html"));
   NavigateToURL(shell(), original_url);
   EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
 
   NavigationController& controller = shell()->web_contents()->GetController();
   FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
                             ->GetFrameTree()
@@ skipping 39 matching lines @@
       "/navigation_controller/simple_page_2.html");
   NavigateFrameToURL(frame, frame_url);
   EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
 
   EXPECT_EQ(2, controller.GetEntryCount());
   EXPECT_EQ(2, RendererHistoryLength(shell()));
   EXPECT_EQ(1, controller.GetLastCommittedEntryIndex());
 
   EXPECT_EQ(frame_url, frame->current_url());
 
+  // At this point the rest of the test is inapplicable. The bug that it tests
+  // to be gone had to be reintroduced.

[Charlie Reis, 2016/05/05 17:45:42]

Can you clarify that the code below repros a NC_IN_PAGE_NAVIGATION kill
(assuming that it does)?

[Avi (use Gerrit), 2016/05/05 19:06:21]

It does not.

This browsertest tested the basic functionality of getting the about:blank frame
reset, which, if done, would prevent the kill. It doesn't actually go as far as
testing the kill, which would be more complicated.

[Charlie Reis, 2016/05/05 19:34:22]

Oh!  I must have misunderstood when reviewing the original CL.

Presumably we'll reopen 542299 as a result of landing this, so can you mention
there what's needed to test the actual renderer kill?

[Avi (use Gerrit), 2016/05/05 19:52:15]

Yep.
The renderer kill is when the iframe does an in-page navigation; if it navigates
from about:blank we're fine, but if the iframe fails to reset to about:blank
then it in-page navigates from the old URL and dies.

I didn't look to see how they make about:blank navigate in-frame.

+  //
+  // See the discussion in NavigationControllerImpl::FindFramesToNavigate for
+  // more information.
+  return;

[Charlie Reis, 2016/05/05 17:45:42]

nit: No need for return or the blank line below, given the #if 0.

[Avi (use Gerrit), 2016/05/05 19:06:21]

Done.

+
+#if 0
   // Go back. Because the old state had an empty frame, that should be restored
   // even though it was replaced in the second navigation entry.
 
   TestFrameNavigationObserver observer(frame);
   ASSERT_TRUE(controller.CanGoBack());
   controller.GoBack();
   observer.Wait();
 
   EXPECT_EQ(2, controller.GetEntryCount());
   EXPECT_EQ(2, RendererHistoryLength(shell()));
   EXPECT_EQ(0, controller.GetLastCommittedEntryIndex());
 
   EXPECT_EQ(GURL(url::kAboutBlankURL), frame->current_url());
+#endif
 }
 
 // This test is similar to "BackToAboutBlankIframe" above, except that a
 // fragment navigation is used rather than pushState (both create an in-page
 // navigation, so we need to test both), and an initial 'src' is given to the
 // iframe to test proper restoration in that case.
 IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
                        BackToIframeWithContent) {
   GURL links_url(embedded_test_server()->GetURL(
       "/navigation_controller/page_with_links.html"));
@@ skipping 50 matching lines @@
       "/navigation_controller/simple_page_2.html");
   NavigateFrameToURL(frame, frame_url_2);
   EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
 
   EXPECT_EQ(3, controller.GetEntryCount());
   EXPECT_EQ(3, RendererHistoryLength(shell()));
   EXPECT_EQ(2, controller.GetLastCommittedEntryIndex());
 
   EXPECT_EQ(frame_url_2, frame->current_url());
 
+  // At this point the rest of the test is inapplicable. The bug that it tests
+  // to be gone had to be reintroduced.
+  //
+  // See the discussion in NavigationControllerImpl::FindFramesToNavigate for
+  // more information.
+  return;
+

[Charlie Reis, 2016/05/05 17:45:42]

Same nits.

[Avi (use Gerrit), 2016/05/05 19:06:21]

Done.

+#if 0
   // Go back two entries. The original frame URL should be back.
 
   TestFrameNavigationObserver observer(frame);
   ASSERT_TRUE(controller.CanGoToOffset(-2));
   controller.GoToOffset(-2);
   observer.Wait();
 
   EXPECT_EQ(3, controller.GetEntryCount());
   EXPECT_EQ(3, RendererHistoryLength(shell()));
   EXPECT_EQ(0, controller.GetLastCommittedEntryIndex());
 
   EXPECT_EQ(frame_url_1, frame->current_url());
+#endif
 }
 
 // Ensure that we do not corrupt a NavigationEntry's PageState if a subframe
 // forward navigation commits after we've already started another forward
 // navigation in the main frame.  See https://crbug.com/597322.
 IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
                        ForwardInSubframeWithPendingForward) {
   // Navigate to a page with an iframe.
   GURL url_a(embedded_test_server()->GetURL(
       "/navigation_controller/page_with_data_iframe.html"));
@@ skipping 187 matching lines @@
     // TODO(clamy): Check the post id as well when PlzNavigate handles it
     // properly.
     if (!IsBrowserSideNavigationEnabled())
       EXPECT_NE(-1, frame_entry->post_id());
     EXPECT_FALSE(entry->GetHasPostData());
     EXPECT_EQ(-1, entry->GetPostID());
   }
 }
 
 }  // namespace content