Skip to content

Commit

Permalink
Don't suppress RenderFrameHostStateChanged for subframes
Browse files Browse the repository at this point in the history 
The early return in WebContentsImpl::RenderFrameHostStateChanged
for subframes only appears relevant for resetting the color chooser, but
it also caused RenderFrameHostStateChanged to be skipped for subframes.

We also add tests for lifecycle state transitions of subframes in
bfcache, which includes testing that RenderFrameHostStateChanged is
called.

Bug: 1073144
Change-Id: Ibaf6cc8e7e118833c558588d7352e722be5d9160
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2854955
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#877924}
  • Loading branch information
@kjmcnee
kjmcnee authored and Chromium LUCI CQ committed Apr 30, 2021
1 parent 2f91730 commit 0c88f7a
Show file tree
Hide file tree
Showing 6 changed files with 544 additions and 9 deletions.
156 changes: 153 additions & 3 deletions content/browser/back_forward_cache_browsertest.cc
View file
Expand Up @@ -60,6 +60,7 @@
#include "content/public/test/content_browser_test.h"
#include "content/public/test/content_browser_test_utils.h"
#include "content/public/test/idle_test_utils.h"
#include "content/public/test/mock_web_contents_observer.h"
#include "content/public/test/navigation_handle_observer.h"
#include "content/public/test/test_navigation_observer.h"
#include "content/public/test/test_navigation_throttle.h"
Expand Down Expand Up @@ -8575,7 +8576,22 @@ IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,

// 2) Navigate to B, now A enters BackForwardCache. Check the
// LifecycleStateImpl of both RenderFrameHost A and B.
EXPECT_TRUE(NavigateToURL(shell(), url_b));
{
testing::NiceMock<MockWebContentsObserver> state_change_observer(
web_contents());
EXPECT_CALL(state_change_observer,
RenderFrameHostStateChanged(
rfh_a, RenderFrameHost::LifecycleState::kActive,
RenderFrameHost::LifecycleState::kInBackForwardCache));
// We don't know |rfh_b| yet, so we'll match any frame.
EXPECT_CALL(state_change_observer,
RenderFrameHostStateChanged(
testing::Not(rfh_a),
RenderFrameHost::LifecycleState::kPendingCommit,
RenderFrameHost::LifecycleState::kActive));

EXPECT_TRUE(NavigateToURL(shell(), url_b));
}
RenderFrameHostImpl* rfh_b = current_frame_host();
EXPECT_TRUE(rfh_a->IsInBackForwardCache());
EXPECT_EQ(RenderFrameHostImpl::LifecycleStateImpl::kInBackForwardCache,
Expand All @@ -8589,15 +8605,149 @@ IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,

// 3) Go back to A and check again the LifecycleStateImpl of both
// RenderFrameHost A and B.
web_contents()->GetController().GoBack();
EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
{
testing::NiceMock<MockWebContentsObserver> state_change_observer(
web_contents());
EXPECT_CALL(state_change_observer,
RenderFrameHostStateChanged(
rfh_a, RenderFrameHost::LifecycleState::kInBackForwardCache,
RenderFrameHost::LifecycleState::kActive));
EXPECT_CALL(state_change_observer,
RenderFrameHostStateChanged(
rfh_b, RenderFrameHost::LifecycleState::kActive,
RenderFrameHost::LifecycleState::kInBackForwardCache));

web_contents()->GetController().GoBack();
EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
}
EXPECT_EQ(RenderFrameHostImpl::LifecycleStateImpl::kActive,
rfh_a->lifecycle_state());
EXPECT_TRUE(rfh_b->IsInBackForwardCache());
EXPECT_EQ(RenderFrameHostImpl::LifecycleStateImpl::kInBackForwardCache,
rfh_b->lifecycle_state());
}

IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
CheckLifecycleStateTransitionWithSubframes) {
IsolateAllSitesForTesting(base::CommandLine::ForCurrentProcess());
ASSERT_TRUE(embedded_test_server()->Start());
GURL url_a(embedded_test_server()->GetURL(
"a.com", "/cross_site_iframe_factory.html?a(b)"));
GURL url_c(embedded_test_server()->GetURL(
"c.com", "/cross_site_iframe_factory.html?c(d)"));

// Navigate to A(B) and check the lifecycle states of A and B.
EXPECT_TRUE(NavigateToURL(shell(), url_a));
RenderFrameHostImpl* rfh_a = current_frame_host();
RenderFrameHostImpl* rfh_b = rfh_a->child_at(0)->current_frame_host();
EXPECT_FALSE(rfh_a->IsInBackForwardCache());
EXPECT_FALSE(rfh_b->IsInBackForwardCache());
EXPECT_EQ(RenderFrameHostImpl::LifecycleStateImpl::kActive,
rfh_a->lifecycle_state());
EXPECT_EQ(RenderFrameHost::LifecycleState::kActive,
rfh_a->GetLifecycleState());
EXPECT_EQ(RenderFrameHostImpl::LifecycleStateImpl::kActive,
rfh_b->lifecycle_state());
EXPECT_EQ(RenderFrameHost::LifecycleState::kActive,
rfh_b->GetLifecycleState());

// Navigate to C(D), now A(B) enters BackForwardCache.
{
testing::NiceMock<MockWebContentsObserver> state_change_observer(
web_contents());
EXPECT_CALL(state_change_observer,
RenderFrameHostStateChanged(
rfh_a, RenderFrameHost::LifecycleState::kActive,
RenderFrameHost::LifecycleState::kInBackForwardCache));
EXPECT_CALL(state_change_observer,
RenderFrameHostStateChanged(
rfh_b, RenderFrameHost::LifecycleState::kActive,
RenderFrameHost::LifecycleState::kInBackForwardCache));
// We don't know |rfh_c| and |rfh_d| yet, so we'll match any frame.
EXPECT_CALL(state_change_observer,
RenderFrameHostStateChanged(
testing::Not(testing::AnyOf(rfh_a, rfh_b)),
RenderFrameHost::LifecycleState::kPendingCommit,
RenderFrameHost::LifecycleState::kActive))
.Times(2);
// Deletion of frame D's initial RFH.
EXPECT_CALL(state_change_observer,
RenderFrameHostStateChanged(
testing::Not(testing::AnyOf(rfh_a, rfh_b)),
RenderFrameHost::LifecycleState::kActive,
RenderFrameHost::LifecycleState::kPendingDeletion));

EXPECT_TRUE(NavigateToURL(shell(), url_c));
}
RenderFrameHostImpl* rfh_c = current_frame_host();
RenderFrameHostImpl* rfh_d = rfh_c->child_at(0)->current_frame_host();
EXPECT_TRUE(rfh_a->IsInBackForwardCache());
EXPECT_TRUE(rfh_b->IsInBackForwardCache());
EXPECT_FALSE(rfh_c->IsInBackForwardCache());
EXPECT_FALSE(rfh_d->IsInBackForwardCache());
EXPECT_EQ(RenderFrameHostImpl::LifecycleStateImpl::kInBackForwardCache,
rfh_a->lifecycle_state());
EXPECT_EQ(RenderFrameHost::LifecycleState::kInBackForwardCache,
rfh_a->GetLifecycleState());
EXPECT_EQ(RenderFrameHostImpl::LifecycleStateImpl::kInBackForwardCache,
rfh_b->lifecycle_state());
EXPECT_EQ(RenderFrameHost::LifecycleState::kInBackForwardCache,
rfh_b->GetLifecycleState());
EXPECT_EQ(RenderFrameHostImpl::LifecycleStateImpl::kActive,
rfh_c->lifecycle_state());
EXPECT_EQ(RenderFrameHost::LifecycleState::kActive,
rfh_c->GetLifecycleState());
EXPECT_EQ(RenderFrameHostImpl::LifecycleStateImpl::kActive,
rfh_d->lifecycle_state());
EXPECT_EQ(RenderFrameHost::LifecycleState::kActive,
rfh_d->GetLifecycleState());

// Go back to A(B), A(B) is restored and C(D) enters BackForwardCache.
{
testing::NiceMock<MockWebContentsObserver> state_change_observer(
web_contents());
EXPECT_CALL(state_change_observer,
RenderFrameHostStateChanged(
rfh_a, RenderFrameHost::LifecycleState::kInBackForwardCache,
RenderFrameHost::LifecycleState::kActive));
EXPECT_CALL(state_change_observer,
RenderFrameHostStateChanged(
rfh_b, RenderFrameHost::LifecycleState::kInBackForwardCache,
RenderFrameHost::LifecycleState::kActive));
EXPECT_CALL(state_change_observer,
RenderFrameHostStateChanged(
rfh_c, RenderFrameHost::LifecycleState::kActive,
RenderFrameHost::LifecycleState::kInBackForwardCache));
EXPECT_CALL(state_change_observer,
RenderFrameHostStateChanged(
rfh_d, RenderFrameHost::LifecycleState::kActive,
RenderFrameHost::LifecycleState::kInBackForwardCache));

web_contents()->GetController().GoBack();
EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
}
EXPECT_FALSE(rfh_a->IsInBackForwardCache());
EXPECT_FALSE(rfh_b->IsInBackForwardCache());
EXPECT_TRUE(rfh_c->IsInBackForwardCache());
EXPECT_TRUE(rfh_d->IsInBackForwardCache());
EXPECT_EQ(RenderFrameHostImpl::LifecycleStateImpl::kActive,
rfh_a->lifecycle_state());
EXPECT_EQ(RenderFrameHost::LifecycleState::kActive,
rfh_a->GetLifecycleState());
EXPECT_EQ(RenderFrameHostImpl::LifecycleStateImpl::kActive,
rfh_b->lifecycle_state());
EXPECT_EQ(RenderFrameHost::LifecycleState::kActive,
rfh_b->GetLifecycleState());
EXPECT_EQ(RenderFrameHostImpl::LifecycleStateImpl::kInBackForwardCache,
rfh_c->lifecycle_state());
EXPECT_EQ(RenderFrameHost::LifecycleState::kInBackForwardCache,
rfh_c->GetLifecycleState());
EXPECT_EQ(RenderFrameHostImpl::LifecycleStateImpl::kInBackForwardCache,
rfh_d->lifecycle_state());
EXPECT_EQ(RenderFrameHost::LifecycleState::kInBackForwardCache,
rfh_d->GetLifecycleState());
}

IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
DoesNotCacheIfSpeechRecognitionIsStarted) {
ASSERT_TRUE(embedded_test_server()->Start());
Expand Down
5 changes: 3 additions & 2 deletions content/browser/renderer_host/render_frame_host_impl.cc
View file
Expand Up @@ -10922,8 +10922,9 @@ void RenderFrameHostImpl::SetLifecycleState(LifecycleStateImpl state) {
LifecycleState old_lifecycle_state = GetLifecycleStateFromImpl(old_state);
LifecycleState new_lifecycle_state = GetLifecycleState();

// old and new lifecycle state can be equal for kPendingDeletion state.
// Don't notify the observers in such cases.
// Old and new lifecycle states can be equal due to the same LifecycleState
// representing multiple LifecycleStateImpls, for example the
// kPendingDeletion state. Don't notify the observers in such cases.
if (old_lifecycle_state != new_lifecycle_state) {
delegate_->RenderFrameHostStateChanged(this, old_lifecycle_state,
new_lifecycle_state);
Expand Down
5 changes: 1 addition & 4 deletions content/browser/web_contents/web_contents_impl.cc
View file
Expand Up @@ -8674,11 +8674,8 @@ void WebContentsImpl::RenderFrameHostStateChanged(
dict.Add("old", old_state);
dict.Add("new", new_state);
});
if (render_frame_host->GetParent())
return;

if (old_state == LifecycleState::kActive &&
new_state != LifecycleState::kActive) {
if (old_state == LifecycleState::kActive && !render_frame_host->GetParent()) {
// TODO(sreejakshetty): Remove this reset when ColorChooser becomes
// per-frame.
// Close the color chooser popup when RenderFrameHost changes state from
Expand Down
13 changes: 13 additions & 0 deletions content/public/test/mock_web_contents_observer.cc
View file
@@ -0,0 +1,13 @@
// Copyright 2021 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "content/public/test/mock_web_contents_observer.h"

namespace content {

MockWebContentsObserver::MockWebContentsObserver(WebContents* web_contents)
: WebContentsObserver(web_contents) {}
MockWebContentsObserver::~MockWebContentsObserver() = default;

} // namespace content

0 comments on commit 0c88f7a

Please sign in to comment.