Skip to content

Commit

Permalink
Reland "Use TestActivationManager for all page activations"
Browse files Browse the repository at this point in the history 
This is a reland of commit c907065

[Reland Note] This CL was reverted as it depended on changes
https://crrev.com/c/3452603 which was reverted, breaking the build. That
CL was relanded in https://crrev.com/c/3494487 so this can now be
relanded as well. No changes from original.

Original change's description:
> Use TestActivationManager for all page activations
>
> https://crrev.com/b650c95b330f added a new test helper,
> TestActivationManager, to be used for navigations that are page
> activating (e.g. BFCache restore, prerender activation).
>
> This CL makes the use of TestActivationManager and TestNavigationManager
> mutually exclusive by DCHECKing in TestNavigationManager that a
> navigation is *not* page activating. Tests must now use the correct
> manager class depending on what kind of navigation they're trying to
> control. This simplifies the TestNavigationManager and makes tests more
> explicit about what's happening.
>
> This CL converts remaining tests that were using TestNavigationManager
> to observe a page activating navigation. In cases where the test doesn't
> control the navigation but only  observes whether or not activation
> occurred, the PrerenderHostObserver helper is used instead.
>
> Change-Id: Ib86008bd539e1e6b13a43b510f37ae98b29657ec
> Bug: 1199724
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3472129
> Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
> Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
> Reviewed-by: Francois Pierre Doray <fdoray@chromium.org>
> Commit-Queue: David Bokan <bokan@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#975493}

Bug: 1199724
Change-Id: I5853ff20cb010c564b3f42a50fe5648e1f79b7f2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3513332
Reviewed-by: Francois Pierre Doray <fdoray@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Auto-Submit: David Bokan <bokan@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/main@{#979413}
  • Loading branch information
@bokand
bokand authored and Chromium LUCI CQ committed Mar 9, 2022
1 parent 48a6114 commit 1bf7cb0
Show file tree
Hide file tree
Showing 5 changed files with 48 additions and 114 deletions.
14 changes: 6 additions & 8 deletions chrome/browser/prerender/prerender_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,14 +88,13 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderAndActivate) {
prerender_helper().AddPrerender(prerender_url);

// Activate.
content::TestNavigationManager navigation_manager(GetActiveWebContents(),
content::TestActivationManager activation_manager(GetActiveWebContents(),
prerender_url);
ASSERT_TRUE(
content::ExecJs(GetActiveWebContents()->GetMainFrame(),
content::JsReplace("location = $1", prerender_url)));
navigation_manager.WaitForNavigationFinished();
EXPECT_TRUE(navigation_manager.was_prerendered_page_activation());
EXPECT_TRUE(navigation_manager.was_successful());
activation_manager.WaitForNavigationFinished();
EXPECT_TRUE(activation_manager.was_activated());

histogram_tester.ExpectUniqueSample(
"Prerender.Experimental.PrerenderHostFinalStatus.SpeculationRule",
Expand Down Expand Up @@ -124,17 +123,16 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest,
*GetActiveWebContents(), prerender_url);

// Activate.
content::TestNavigationManager navigation_manager(GetActiveWebContents(),
content::TestActivationManager activation_manager(GetActiveWebContents(),
prerender_url);
// Simulate a browser-initiated navigation.
GetActiveWebContents()->OpenURL(content::OpenURLParams(
prerender_url, content::Referrer(), WindowOpenDisposition::CURRENT_TAB,
ui::PageTransitionFromInt(ui::PAGE_TRANSITION_TYPED |
ui::PAGE_TRANSITION_FROM_ADDRESS_BAR),
/*is_renderer_initiated=*/false));
navigation_manager.WaitForNavigationFinished();
EXPECT_TRUE(navigation_manager.was_prerendered_page_activation());
EXPECT_TRUE(navigation_manager.was_successful());
activation_manager.WaitForNavigationFinished();
EXPECT_TRUE(activation_manager.was_activated());

histogram_tester.ExpectUniqueSample(
"Prerender.Experimental.PrerenderHostFinalStatus.Embedder_DirectURLInput",
Expand Down
7 changes: 3 additions & 4 deletions components/performance_manager/prerendering_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,11 +132,10 @@ IN_PROC_BROWSER_TEST_F(PerformanceManagerPrerenderingBrowserTest,
// its main frame, and the original frame tree is gone.
content::RenderFrameDeletedObserver deleted_observer(
web_contents()->GetMainFrame());
content::TestNavigationManager navigation_manager(web_contents(),
kPrerenderingUrl);
content::test::PrerenderHostObserver prerender_observer(*web_contents(),
kPrerenderingUrl);
prerender_helper_.NavigatePrimaryPage(kPrerenderingUrl);
navigation_manager.WaitForNavigationFinished();
ASSERT_TRUE(navigation_manager.was_prerendered_page_activation());
ASSERT_TRUE(prerender_observer.was_activated());
deleted_observer.WaitUntilDeleted();
RunInGraph([&](Graph*) {
ASSERT_TRUE(page_node);
Expand Down
49 changes: 23 additions & 26 deletions content/browser/prerender/prerender_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2107,14 +2107,12 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, MojoCapabilityControl_LoosenMode) {

// 5. Activate the prerendered page and listen to the DidFinishNavigation
// event, to ensure the Activate IPC is sent.
TestNavigationManager prerendered_activation_navigation(web_contents(),
TestActivationManager prerendered_activation_navigation(web_contents(),
prerendering_url);
ASSERT_TRUE(ExecJs(web_contents()->GetMainFrame(),
JsReplace("location = $1", prerendering_url)));
prerendered_activation_navigation.WaitForNavigationFinished();
EXPECT_TRUE(
prerendered_activation_navigation.was_prerendered_page_activation());
EXPECT_TRUE(prerendered_activation_navigation.was_successful());
EXPECT_TRUE(prerendered_activation_navigation.was_activated());

// 6. Renderers attempt to build Mojo connections for kCancel interfaces.
// This part simulates some subframe documents start sending kCancel
Expand Down Expand Up @@ -2424,7 +2422,7 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest,
// frame tree commits.
{
// Start navigation in primary page to kPrerenderingUrl.
TestNavigationManager primary_page_manager(shell()->web_contents(),
TestActivationManager primary_page_manager(shell()->web_contents(),
kPrerenderingUrl);
ASSERT_TRUE(ExecJs(shell()->web_contents()->GetMainFrame(),
JsReplace("location = $1", kPrerenderingUrl)));
Expand All @@ -2433,9 +2431,11 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest,
web_contents_impl()->GetPrimaryFrameTree().root()->navigation_request();

// Wait until the navigation is deferred by CommitDeferringCondition.
// TODO(nhiroki): Avoid using base::RunUntilIdle() and instead use some
// explicit signal of the running condition.
base::RunLoop().RunUntilIdle();
ASSERT_TRUE(primary_page_manager.WaitForBeforeChecks());
primary_page_manager.ResumeActivation();

// TODO(bokan): This could be any CommitDeferringCondition, we should have
// a way to pause on a specific CommitDeferringCondition.
EXPECT_TRUE(request->IsCommitDeferringConditionDeferredForTesting());

// The navigation should not have proceeded past NOT_STARTED because the
Expand Down Expand Up @@ -3342,16 +3342,18 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, DidFailLoadCancelsPrerendering) {

// The prerender host for the URL should be deleted as DidFailLoad cancels
// prerendering.
TestNavigationManager activation_observer(shell()->web_contents(),
test::PrerenderHostObserver prerender_observer(*web_contents(),
kPrerenderingUrl);
TestNavigationManager navigation_observer(shell()->web_contents(),
kPrerenderingUrl);
EXPECT_FALSE(HasHostForUrl(kPrerenderingUrl));

// Now navigate the primary page to the prerendered URL. Cancelling the
// prerender disables the activation due to DidFailLoad.
ASSERT_TRUE(ExecJs(web_contents()->GetMainFrame(),
JsReplace("location = $1", kPrerenderingUrl)));
activation_observer.WaitForNavigationFinished();
EXPECT_FALSE(activation_observer.was_prerendered_page_activation());
navigation_observer.WaitForNavigationFinished();
EXPECT_FALSE(prerender_observer.was_activated());

ExpectFinalStatusForSpeculationRule(PrerenderHost::FinalStatus::kDidFailLoad);
}
Expand Down Expand Up @@ -5216,10 +5218,9 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest,
EXPECT_EQ(current_frame_host()->GetFrameName(), "");

// 4. Activate prerender.
TestNavigationManager activation_manager(web_contents(), kPrerenderingUrl);
test::PrerenderHostObserver host_observer(*web_contents(), kPrerenderingUrl);
NavigatePrimaryPage(kPrerenderingUrl);
activation_manager.WaitForNavigationFinished();
EXPECT_TRUE(activation_manager.was_prerendered_page_activation());
EXPECT_TRUE(host_observer.was_activated());
EXPECT_EQ(web_contents()->GetLastCommittedURL(), kPrerenderingUrl);

// 5. Ensure that the window.name is preserved.
Expand Down Expand Up @@ -5371,10 +5372,9 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest,
network::mojom::WebSandboxFlags::kNone);
}

TestNavigationManager activation_manager(web_contents(), kPrerenderingUrl);
test::PrerenderHostObserver host_observer(*web_contents(), kPrerenderingUrl);
NavigatePrimaryPage(kPrerenderingUrl);
activation_manager.WaitForNavigationFinished();
EXPECT_TRUE(activation_manager.was_prerendered_page_activation());
EXPECT_TRUE(host_observer.was_activated());
EXPECT_EQ(web_contents()->GetLastCommittedURL(), kPrerenderingUrl);

// Check that CSP was set on the prerendered page after activation.
Expand Down Expand Up @@ -5424,10 +5424,9 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest,
~network::mojom::WebSandboxFlags::kAutomaticFeatures);
}

TestNavigationManager activation_manager(web_contents(), kPrerenderingUrl);
test::PrerenderHostObserver host_observer(*web_contents(), kPrerenderingUrl);
NavigatePrimaryPage(kPrerenderingUrl);
activation_manager.WaitForNavigationFinished();
EXPECT_TRUE(activation_manager.was_prerendered_page_activation());
EXPECT_TRUE(host_observer.was_activated());
EXPECT_EQ(web_contents()->GetLastCommittedURL(), kPrerenderingUrl);

// Check that CSP was set on the prerendered page after activation.
Expand Down Expand Up @@ -5472,10 +5471,9 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, VerifyPrerenderProcessVisibility) {
EXPECT_TRUE(prerender_process_host->IsProcessBackgrounded());

// Activate the prerendered page.
TestNavigationManager activation_manager(web_contents(), kPrerenderingUrl);
test::PrerenderHostObserver host_observer(*web_contents(), kPrerenderingUrl);
NavigatePrimaryPage(kPrerenderingUrl);
activation_manager.WaitForNavigationFinished();
EXPECT_TRUE(activation_manager.was_prerendered_page_activation());
EXPECT_TRUE(host_observer.was_activated());
// Expect the change in the ChildProcessLauncherPriority to become visible.
EXPECT_FALSE(prerender_process_host->IsProcessBackgrounded());
}
Expand Down Expand Up @@ -5607,10 +5605,9 @@ IN_PROC_BROWSER_TEST_F(PrerenderPurposePrefetchBrowserTest, ResourceRequests) {
EXPECT_TRUE(TestPurposePrefetchHeader(cross_origin_url1));

// Activate the prerendered page.
TestNavigationManager activation_manager(web_contents(), kPrerenderingUrl);
test::PrerenderHostObserver host_observer(*web_contents(), kPrerenderingUrl);
NavigatePrimaryPage(kPrerenderingUrl);
activation_manager.WaitForNavigationFinished();
EXPECT_TRUE(activation_manager.was_prerendered_page_activation());
EXPECT_TRUE(host_observer.was_activated());

// Issue iframe and subresource requests in the activated page.
EXPECT_TRUE(ExecJs(prerender_main_frame.get(), "run('after');",
Expand Down
45 changes: 10 additions & 35 deletions content/public/test/browser_test_utils.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3092,23 +3092,17 @@ void TestNavigationManager::DidStartNavigation(NavigationHandle* handle) {
if (!ShouldMonitorNavigation(handle))
return;

was_prerendered_page_activation_ = handle->IsPrerenderedPageActivation();
DCHECK(!handle->IsPageActivation())
<< "For PageActivating navigations, use TestActivationManager.";

request_ = NavigationRequest::From(handle);
if (request_->IsPageActivation()) {
// For activating navigations, we have no way of stopping at
// WillStartRequest since we don't run throttles. Callers should use
// WaitForResponse() or WaitForFirstYieldAfterDidStartNavigation().
DCHECK_NE(desired_state_, NavigationState::STARTED);
} else {
auto throttle = std::make_unique<TestNavigationManagerThrottle>(
request_,
base::BindOnce(&TestNavigationManager::OnWillStartRequest,
weak_factory_.GetWeakPtr()),
base::BindOnce(&TestNavigationManager::OnWillProcessResponse,
weak_factory_.GetWeakPtr()));
request_->RegisterThrottleForTesting(std::move(throttle));
}
auto throttle = std::make_unique<TestNavigationManagerThrottle>(
request_,
base::BindOnce(&TestNavigationManager::OnWillStartRequest,
weak_factory_.GetWeakPtr()),
base::BindOnce(&TestNavigationManager::OnWillProcessResponse,
weak_factory_.GetWeakPtr()));
request_->RegisterThrottleForTesting(std::move(throttle));

current_state_ = NavigationState::WILL_START;

Expand All @@ -3130,8 +3124,6 @@ void TestNavigationManager::DidFinishNavigation(NavigationHandle* handle) {
return;
was_committed_ = handle->HasCommitted();
was_successful_ = was_committed_ && !handle->IsErrorPage();
DCHECK_EQ(was_prerendered_page_activation_.value(),
request_->IsPrerenderedPageActivation());
current_state_ = NavigationState::FINISHED;
navigation_paused_ = false;
request_ = nullptr;
Expand All @@ -3156,14 +3148,6 @@ void TestNavigationManager::OnWillProcessResponse() {
OnNavigationStateChanged();
}

void TestNavigationManager::OnRunningCommitDeferringConditions(
base::OnceClosure resume_closure) {
current_state_ = NavigationState::RESPONSE;
commit_deferring_condition_resume_closure_ = std::move(resume_closure);
navigation_paused_ = true;
OnNavigationStateChanged();
}

// TODO(csharrison): Remove CallResumeForTesting method calls in favor of doing
// it through the throttle.
bool TestNavigationManager::WaitForDesiredState() {
Expand Down Expand Up @@ -3191,12 +3175,6 @@ void TestNavigationManager::OnNavigationStateChanged() {
TRACE_EVENT("test", "TestNavigationManager::OnNavigationStateChanged", "this",
this);

if (request_ && request_->IsPageActivation()) {
DCHECK_NE(desired_state_, NavigationState::STARTED)
<< "Cannot use WaitForRequestStart() when managing an activating "
"navigation. Use either WaitForFirstYieldAfterDidStartNavigation() "
"or WaitForResponse()";
}
// If the state the user was waiting for has been reached, exit the message
// loop.
if (current_state_ >= desired_state_) {
Expand All @@ -3217,10 +3195,7 @@ void TestNavigationManager::ResumeIfPaused() {

navigation_paused_ = false;

if (!request_->IsPageActivation())
request_->GetNavigationThrottleRunnerForTesting()->CallResumeForTesting();
else if (commit_deferring_condition_resume_closure_)
std::move(commit_deferring_condition_resume_closure_).Run();
request_->GetNavigationThrottleRunnerForTesting()->CallResumeForTesting();
}

bool TestNavigationManager::ShouldMonitorNavigation(NavigationHandle* handle) {
Expand Down
47 changes: 6 additions & 41 deletions content/public/test/browser_test_utils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1581,23 +1581,9 @@ class FrameDeletedObserver {
// navigation it will ignore all subsequent navigations. Explicitly create
// multiple instances of this class if you want to pause multiple navigations.
//
// Note2: For a prerender activation, this class cannot pause the navigation as
// the activation doesn't run NavigationThrottles and runs
// CommitDeferringConditions before StartNavigation() that WebContentsObserver
// cannot observe. Use TestActivationManager in these cases instead.
//
// Note3: For a BFCache restore navigation, the navigation will not run
// NavigationThrottles. The manager in this case uses a
// CommitDeferringCondition for pausing the navigation at the equivalent of
// WillProcessResponse. However, in this navigation you cannot use
// WaitForRequestStart; if you want to yield before WillProcessResponse, use
// WaitForFirstYieldAfterDidStartNavigation.
// TODO(bokan): Hopefully BFCache will soon work like prerender activation
// does. Prefer TestActivationManager for BFCache activations as well.
// https://crbug.com/1226442.
//
// TODO(bokan): Remove uses of this class for page activations and add a DCHECK
// that the navigation isn't page activating.
// Note2: This class cannot be used with page activating navigations (e.g.
// BFCache, prerendering, etc.) as the activation doesn't run
// NavigationThrottles. Use TestActivationManager in these cases instead.
class TestNavigationManager : public WebContentsObserver {
public:
// Monitors any frame in WebContents.
Expand All @@ -1612,19 +1598,14 @@ class TestNavigationManager : public WebContentsObserver {
// WaitForRequestStart, this can be used to wait for a pause in cases where a
// test expects a NavigationThrottle to defer in WillStartRequest. In cases
// where throttles run and none defer, this will break at the same time as
// WaitForRequestStart. Also unlike WaitForRequestStart, this can be used to
// wait on a page activating navigation to start. Note: since we won't know
// which throttle deferred, don't use ResumeNavigation() after this call since
// it assumes we paused from the TestNavigationManagerThrottle.
// WaitForRequestStart. Note: since we won't know which throttle deferred,
// don't use ResumeNavigation() after this call since it assumes we paused
// from the TestNavigationManagerThrottle.
void WaitForFirstYieldAfterDidStartNavigation();

// Waits until the navigation request is ready to be sent to the network
// stack. This will wait until all NavigationThrottles have proceeded through
// WillStartRequest. Returns false if the request was aborted before starting.
// Note: RequestStart is never reached for page activating navigations (e.g.
// prerender activation, BFCache restore). In those cases you should either
// use WaitForFirstYieldAfterDidStartNavigation or WaitForResponse. See
// TestNavigationManager class comment for more detail.
[[nodiscard]] bool WaitForRequestStart();

// Waits until the navigation response's headers have been received. This
Expand Down Expand Up @@ -1652,11 +1633,6 @@ class TestNavigationManager : public WebContentsObserver {
// Whether the navigation successfully committed and was not an error page.
bool was_successful() const { return was_successful_; }

// Whether the navigation activated a prerendered page.
bool was_prerendered_page_activation() const {
return was_prerendered_page_activation_.value();
}

// Allows nestable tasks when running a message loop in the Wait* functions.
// This is useful for utilizing this class from within another message loop.
void AllowNestableTasks();
Expand Down Expand Up @@ -1690,10 +1666,6 @@ class TestNavigationManager : public WebContentsObserver {
// WillProcessResponse.
void OnWillProcessResponse();

// Called when the navigation pauses in the MockCommitDeferringCondition. This
// happens only for page activating navigations like a prerender activation.
void OnRunningCommitDeferringConditions(base::OnceClosure resume_closure);

// Waits for the desired state. Returns false if the desired state cannot be
// reached (eg the navigation finishes before reaching this state).
bool WaitForDesiredState();
Expand All @@ -1712,16 +1684,9 @@ class TestNavigationManager : public WebContentsObserver {
NavigationState desired_state_ = NavigationState::WILL_START;
bool was_committed_ = false;
bool was_successful_ = false;
absl::optional<bool> was_prerendered_page_activation_;
base::OnceClosure quit_closure_;
base::RunLoop::Type message_loop_type_ = base::RunLoop::Type::kDefault;

// In a page activating navigation (prerender activation, back-forward cache
// activation), the navigation will be stopped in a commit deferring condition
// (since NavigationThrottles aren't run in a page activation). When that
// happens, the navigation can be resumed using this closure.
base::OnceClosure commit_deferring_condition_resume_closure_;

base::WeakPtrFactory<TestNavigationManager> weak_factory_{this};
};

Expand Down

0 comments on commit 1bf7cb0

Please sign in to comment.