chromium/src
0
Fork 0
Code Activity

Don't update RenderFrameHostImpl::LifecycleState during deletion from bfcache

Browse Source 
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}
This commit is contained in:
Sreeja Kamishetty
2020-10-23 13:33:21 +00:00
committed by Commit Bot
parent b89a003b89
commit 9bcdc974b9
2 changed files with 58 additions and 15 deletions

42
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:

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