From 836596d5da4458b675160abf535db4d074152d4e Mon Sep 17 00:00:00 2001 From: Avi Drissman Date: Fri, 6 May 2016 14:39:36 -0400 Subject: [PATCH] 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, https://github.com/whatwg/html/issues/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} (cherry picked from commit b48cb31b164c516c109e751f1bd8d9f6eb24b147) Review URL: https://codereview.chromium.org/1954213003 . Cr-Commit-Position: refs/branch-heads/2661@{#662} Cr-Branched-From: ef6f6ae5e4c96622286b563658d5cd62a6cf1197-refs/heads/master@{#378081} --- .../frame_host/navigation_controller_impl.cc | 15 +++++++++++++++ .../navigation_controller_impl_browsertest.cc | 18 ++++++++++++++++++ content/renderer/history_controller.cc | 5 +++++ 3 files changed, 38 insertions(+) diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc index f2e503f27193b..798d964d2aead 100644 --- a/content/browser/frame_host/navigation_controller_impl.cc +++ b/content/browser/frame_host/navigation_controller_impl.cc @@ -1886,6 +1886,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 , 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 and the spec discussion at + // . + // + // 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 diff --git a/content/browser/frame_host/navigation_controller_impl_browsertest.cc b/content/browser/frame_host/navigation_controller_impl_browsertest.cc index 60794bef841c3..ad47a238084f0 100644 --- a/content/browser/frame_host/navigation_controller_impl_browsertest.cc +++ b/content/browser/frame_host/navigation_controller_impl_browsertest.cc @@ -3277,6 +3277,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://github.com/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( @@ -3336,6 +3337,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. @@ -3349,12 +3357,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( @@ -3419,6 +3429,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); @@ -3431,6 +3448,7 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, EXPECT_EQ(0, controller.GetLastCommittedEntryIndex()); EXPECT_EQ(frame_url_1, frame->current_url()); +#endif } } // namespace content diff --git a/content/renderer/history_controller.cc b/content/renderer/history_controller.cc index bf007154053f1..fe915bf59809c 100644 --- a/content/renderer/history_controller.cc +++ b/content/renderer/history_controller.cc @@ -144,6 +144,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