Don't update subframes on parent frame commits.

This reverts r371031.

This reverts a genuine bugfix, but the bug is needed to mask a much deeper and severe problem. Discussion is ongoing there.

BUG=598043, 542299, 600534, whatwg/html#1191
TEST=disabling
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/1949213002
Cr-Commit-Position: refs/heads/master@{#391906}

content/browser/frame_host/navigation_controller_impl.cc

@@ -1878,6 +1878,21 @@ void NavigationControllerImpl::FindFramesToNavigate(
  new_item->document_sequence_number() ==
  old_item->document_sequence_number()) {
  same_document_loads->push_back(std::make_pair(frame, new_item));
+
+ // TODO(avi, creis): This is a bug; we should not return here. Rather, we
+ // should continue on and navigate all child frames which have also
+ // changed. This bug is the cause of <https://crbug.com/542299>, which is
+ // a NC_IN_PAGE_NAVIGATION renderer kill.
+ //
+ // However, this bug is a bandaid over a deeper and worse problem. Doing a
+ // pushState immediately after loading a subframe is a race, one that no
+ // web page author expects. If we fix this bug, many large websites break.
+ // For example, see <https://crbug.com/598043> and the spec discussion at
+ // <https:/whatwg/html/issues/1191>.
+ //
+ // For now, we accept this bug, and hope to resolve the race in a
+ // different way that will one day allow us to fix this.
+ return;
  } else {
  different_document_loads->push_back(std::make_pair(frame, new_item));
  // For a different document, the subframes will be destroyed, so there's

content/browser/frame_host/navigation_controller_impl_browsertest.cc

@@ -3715,6 +3715,7 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, ReloadOriginalRequest) {
 // 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:/whatwg/html/issues/546 .
+// TODO(avi, creis): This test is partially neutered; fix it.
 IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
  BackToAboutBlankIframe) {
  GURL original_url(embedded_test_server()->GetURL(
@@ -3774,6 +3775,13 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
 
  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.
+ //
+ // See the discussion in NavigationControllerImpl::FindFramesToNavigate for
+ // more information.
+
+#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.
 
@@ -3787,12 +3795,14 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
  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.
+// TODO(avi, creis): This test is partially neutered; fix it.
 IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
  BackToIframeWithContent) {
  GURL links_url(embedded_test_server()->GetURL(
@@ -3857,6 +3867,13 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
 
  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.
+
+#if 0
  // Go back two entries. The original frame URL should be back.
 
  TestFrameNavigationObserver observer(frame);
@@ -3869,6 +3886,7 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
  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

content/renderer/history_controller.cc

@@ -146,6 +146,11 @@ void HistoryController::RecursiveGoToEntry(
  new_item.documentSequenceNumber() ==
  old_item.documentSequenceNumber()) {
  same_document_loads.push_back(std::make_pair(frame, new_item));
+
+ // Returning here (and omitting child frames which have also changed) is
+ // wrong, but not returning here is worse. See the discussion in
+ // NavigationControllerImpl::FindFramesToNavigate for more information.
+ return;
  } else {
  different_document_loads.push_back(std::make_pair(frame, new_item));
  // For a different document, the subframes will be destroyed, so there's