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); }