From 9bcdc974b98cba650b05d6a8178b51a996c13280 Mon Sep 17 00:00:00 2001
From: Sreeja Kamishetty <sreejakshetty@chromium.org>
Date: Fri, 23 Oct 2020 13:33:21 +0000
Subject: [PATCH] Don't update RenderFrameHostImpl::LifecycleState during
deletion from bfcache
Previously, the lifecycle state was updated to
LifecycleState::kRunningUnloadHandlers/LifecycleState::kReadyToBeDeleted
based on RFH having unload handlers or not during deletion
when the RFH is in BackForwardCache.
This CL removes this transition of LifecycleState and retains it to
kInBackForwardCache during deletion.
BUG=1109742
Change-Id: Ieef3635f80707be569dabb0ab37e38438084638e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2351455
Commit-Queue: Sreeja Kamishetty <sreejakshetty@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#820220}
---
.../browser/back_forward_cache_browsertest.cc | 42 ++++++++++++++++---
.../renderer_host/render_frame_host_impl.cc | 31 ++++++++++----
2 files changed, 58 insertions(+), 15 deletions(-)
diff --git a/content/browser/back_forward_cache_browsertest.cc b/content/browser/back_forward_cache_browsertest.cc
index 036b4a079355b..1915f29043171 100644
--- a/content/browser/back_forward_cache_browsertest.cc
+++ b/content/browser/back_forward_cache_browsertest.cc
@@ -4691,12 +4691,6 @@ IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTestSkipSameSiteUnload,
IN_PROC_BROWSER_TEST_F(
BackForwardCacheBrowserTestSkipSameSiteUnload,
SameSiteNavigationFromPageWithUnloadInCrossSiteSubframe) {
- if (AreAllSitesIsolatedForTesting()) {
- // Currently this test will crash because of a bug that happens when we
- // bfcache pages with subframes that have unload handlers.
- // TODO(crbug.com/1109742): Enable this test once the bug is fixed.
- return;
- }
ASSERT_TRUE(embedded_test_server()->Start());
GURL url_a1(embedded_test_server()->GetURL(
"a.com", "/cross_site_iframe_factory.html?a(b)"));
@@ -4788,6 +4782,42 @@ IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
ExpectUniqueSample(kEligibilityDuringCommitHistogramName, true, 1);
}
+// Sub-frame doesn't transition from LifecycleState::kInBackForwardCache to
+// LifecycleState::kRunningUnloadHandlers even when the sub-frame having unload
+// handlers is being evicted from BackForwardCache.
+IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest, SubframeWithUnloadHandler) {
+ ASSERT_TRUE(embedded_test_server()->Start());
+ GURL main_url(embedded_test_server()->GetURL(
+ "a.com", "/cross_site_iframe_factory.html?a.com(a.com)"));
+ GURL child_url = embedded_test_server()->GetURL(
+ "a.com", "/cross_site_iframe_factory.html?a.com()");
+ GURL url_2(embedded_test_server()->GetURL("a.com", "/title1.html"));
+
+ // 1) Navigate to |main_url|.
+ EXPECT_TRUE(NavigateToURL(shell(), main_url));
+ RenderFrameHostImpl* main_rfh = current_frame_host();
+ ASSERT_EQ(1U, main_rfh->child_count());
+ RenderFrameHostImpl* child_rfh = main_rfh->child_at(0)->current_frame_host();
+ RenderFrameDeletedObserver main_rfh_observer(main_rfh),
+ child_rfh_observer(child_rfh);
+
+ // 2) Add an unload handler to the child RFH.
+ EXPECT_TRUE(ExecJs(child_rfh, "window.onunload = () => {} "));
+
+ // 3) Navigate to |url_2|.
+ EXPECT_TRUE(NavigateToURL(shell(), url_2));
+
+ // 4) The previous main RFH and child RFH should be in the back-forward
+ // cache.
+ EXPECT_FALSE(main_rfh_observer.deleted());
+ EXPECT_FALSE(child_rfh_observer.deleted());
+ EXPECT_TRUE(main_rfh->IsInBackForwardCache());
+ EXPECT_TRUE(child_rfh->IsInBackForwardCache());
+
+ // Destruction of bfcached page happens after shutdown and it should not
+ // trigger unload handlers and be destroyed directly.
+}
+
class GeolocationBackForwardCacheBrowserTest
: public BackForwardCacheBrowserTest {
protected:
diff --git a/content/browser/renderer_host/render_frame_host_impl.cc b/content/browser/renderer_host/render_frame_host_impl.cc
index fb8313f6f8d98..7761e868d9e5c 100644
--- a/content/browser/renderer_host/render_frame_host_impl.cc
+++ b/content/browser/renderer_host/render_frame_host_impl.cc
@@ -2287,16 +2287,26 @@ void RenderFrameHostImpl::DeleteRenderFrame(FrameDeleteIntention intent) {
if (IsPendingDeletion())
return;
+ // In case of BackForwardCache, page is evicted directly from the cache and
+ // deleted immediately, without waiting for unload handlers.
+ bool wait_for_unload_handlers =
+ has_unload_handlers() && !IsInBackForwardCache();
+
if (render_frame_created_) {
Send(new UnfreezableFrameMsg_Delete(routing_id_, intent));
if (!frame_tree_node_->IsMainFrame() && IsCurrent()) {
DCHECK_NE(lifecycle_state(), LifecycleState::kSpeculative);
- // If this subframe has unload handlers (and isn't speculative), ensure
- // that they have a chance to execute by delaying process cleanup. This
- // will prevent the process from shutting down immediately in the case
- // where this is the last active frame in the process.
- // See https://crbug.com/852204.
+ // Documents from the page in the BackForwardCache don't run their unload
+ // handlers, even if they have one. As a result, this should never delay
+ // process shutdown.
+ DCHECK_NE(lifecycle_state(), LifecycleState::kInBackForwardCache);
+
+ // If this document has unload handlers (and isn't speculative or in the
+ // back-forward cache), ensure that they have a chance to execute by
+ // delaying process cleanup. This will prevent the process from shutting
+ // down immediately in the case where this is the last active frame in the
+ // process. See https://crbug.com/852204.
if (has_unload_handlers()) {
RenderProcessHostImpl* process =
static_cast<RenderProcessHostImpl*>(GetProcess());
@@ -2310,7 +2320,7 @@ void RenderFrameHostImpl::DeleteRenderFrame(FrameDeleteIntention intent) {
}
}
- LifecycleState lifecycle_state = has_unload_handlers()
+ LifecycleState lifecycle_state = wait_for_unload_handlers
? LifecycleState::kRunningUnloadHandlers
: LifecycleState::kReadyToBeDeleted;
SetLifecycleState(lifecycle_state);
@@ -5884,10 +5894,13 @@ void RenderFrameHostImpl::StartPendingDeletionOnSubtree() {
local_ancestor->DeleteRenderFrame(FrameDeleteIntention::kNotMainFrame);
if (local_ancestor != child) {
+ // In case of BackForwardCache, page is evicted directly from the cache
+ // and deleted immediately, without waiting for unload handlers.
+ bool wait_for_unload_handlers =
+ child->has_unload_handlers() && !child->IsInBackForwardCache();
LifecycleState child_lifecycle_state =
- child->has_unload_handlers()
- ? LifecycleState::kRunningUnloadHandlers
- : LifecycleState::kReadyToBeDeleted;
+ wait_for_unload_handlers ? LifecycleState::kRunningUnloadHandlers
+ : LifecycleState::kReadyToBeDeleted;
child->SetLifecycleState(child_lifecycle_state);
}