0

test_runner: Improve reset mechanism.

In between two web_tests, Chrome may reuse the current process.
It requests the renderer to reset some state and navigate
to about:blank.

The current flow is:
┌───────┐                               ┌────────┐
│browser│                               │renderer│
└───┬───┘                               └───┬────┘
 (end of previous test)                     │
    │                                       │
    │      ResetRendererAfterWebTest()      │
    │──────────────────────────────────────>│ Store the callback
    │BeginNavigation(blank,opaque initiator)│
    │<──────────────────────────────────────│
    │                                       │
    │           CommitNavigation            │
    │──────────────────────────────────────>│ call the callback.
    │               Callback                │
    │<──────────────────────────────────────│
 (start new test)                           │
┌───┴───┐                               ┌───┴────┐
│browser│                               │renderer│
└───────┘                               └────────┘

It has two problems:

1. Navigation is implemented in the browser process. There are no need
   to request the renderer process to navigate. This is a pure waste of
   time.
2. The navigations to about:blank can commit in a different process,
   which will cause a failure when it happens.

After this patch, the browser initiates the navigation and it doesn't wait on
ResetRendererAfterWebTest() callback anymore.
┌───────┐                               ┌────────────┐
│browser│                               │new renderer│
└───┬───┘                               └───┬────────┘
 (end of previous test)                     │
    │                                       │
    │      CommitNavigation(about:blank)    │
    │──────────────────────────────────────>│
    │      DidCommitNavigation()            │
    │<──────────────────────────────────────│
    │                                       │
    │      ResetRendererAfterWebTest()      │
    │──────────────────────────────────────>│
 (start new test)                           │
┌───┴───┐                               ┌───┴───────┐
│browser│                               │renderer   │
└───────┘                               └───────────┘

Bug: 1190842
Change-Id: Ie5f18d1f338f6fb80a11a8c18df71b0f786edfdf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2784821
Auto-Submit: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#868604}
This commit is contained in:
arthursonzogni
2021-04-01 18:36:03 +00:00
committed by Chromium LUCI CQ
parent 0b885cbced
commit 7ec0febbb9
14 changed files with 96 additions and 101 deletions

@ -111,15 +111,20 @@ TEST(BlinkPlatformTest, CastWebSecurityOrigin) {
{
SCOPED_TRACE(testing::Message() << "null");
blink::WebSecurityOrigin web_origin =
blink::WebSecurityOrigin::CreateUniqueOpaque();
EXPECT_TRUE(web_origin.IsOpaque());
url::Origin url_origin = web_origin;
url::Origin url_origin = url::Origin::Create(GURL(""));
EXPECT_TRUE(url_origin.opaque());
web_origin = url::Origin::Create(GURL(""));
blink::WebSecurityOrigin web_origin = url_origin;
EXPECT_TRUE(web_origin.IsOpaque());
// Test copy constructor:
EXPECT_TRUE(url::Origin(web_origin).opaque());
EXPECT_TRUE(blink::WebSecurityOrigin(url_origin).IsOpaque());
// Test operator=().
EXPECT_TRUE(url::Origin().operator=(web_origin).opaque());
EXPECT_TRUE(blink::WebSecurityOrigin().operator=(url_origin).IsOpaque());
}
}

@ -39,6 +39,7 @@
#include "content/browser/renderer_host/frame_tree.h"
#include "content/browser/renderer_host/frame_tree_node.h"
#include "content/browser/renderer_host/navigation_request.h"
#include "content/browser/web_contents/web_contents_impl.h"
#include "content/public/browser/browser_task_traits.h"
#include "content/public/browser/child_process_security_policy.h"
#include "content/public/browser/child_process_termination_info.h"
@ -98,6 +99,7 @@
#include "ui/shell_dialogs/select_file_dialog.h"
#include "ui/shell_dialogs/select_file_dialog_factory.h"
#include "ui/shell_dialogs/select_file_policy.h"
#include "url/url_constants.h"
#if defined(OS_MAC)
#include "base/mac/foundation_util.h"
@ -107,6 +109,9 @@ namespace content {
namespace {
// The URL used in between two web tests.
const char kAboutBlankResetWebTest[] = "about:blank?reset-web-test";
std::string DumpFrameState(const blink::ExplodedFrameState& frame_state,
size_t indent,
bool is_current_index) {
@ -1241,7 +1246,7 @@ void WebTestControlHost::OnTestFinished() {
ShellContentBrowserClient::Get()->browser_context();
base::RepeatingClosure barrier_closure = base::BarrierClosure(
2, base::BindOnce(&WebTestControlHost::ResetRendererAfterWebTest,
2, base::BindOnce(&WebTestControlHost::PrepareRendererForNextWebTest,
weak_factory_.GetWeakPtr()));
StoragePartition* storage_partition =
@ -1773,26 +1778,53 @@ void WebTestControlHost::CheckForLeakedWindows() {
check_for_leaked_windows_ = true;
}
void WebTestControlHost::ResetRendererAfterWebTest() {
if (main_window_) {
main_window_->web_contents()->Stop();
RenderProcessHost* main_frame_process = main_window_->web_contents()
->GetMainFrame()
->GetRenderViewHost()
->GetProcess();
GetWebTestRenderThreadRemote(main_frame_process)
->ResetRendererAfterWebTest(
base::BindOnce(&WebTestControlHost::ResetRendererAfterWebTestDone,
weak_factory_.GetWeakPtr()));
} else {
// If the window is gone, due to crashes or whatever, we need to make
// progress.
ResetRendererAfterWebTestDone();
void WebTestControlHost::PrepareRendererForNextWebTest() {
// If the window is gone, due to crashes or whatever, we need to make
// progress.
if (!main_window_) {
PrepareRendererForNextWebTestDone();
return;
}
content::WebContentsImpl* web_contents =
static_cast<WebContentsImpl*>(main_window_->web_contents());
// TODO(arthursonzogni): Not sure if this line is needed. It cancels pending
// navigations and pending subresources requests. I guess it increases the
// odds of transitionning from one test to another with no side effects.
// Consider removing it to understand what happens without.
web_contents->Stop();
// Navigate to about:blank in between two consecutive web tests.
//
// Note: this navigation might happen in a new process, depending on the
// COOP policy of the previous document.
NavigationController::LoadURLParams params((GURL(kAboutBlankResetWebTest)));
params.transition_type = ui::PageTransitionFromInt(ui::PAGE_TRANSITION_TYPED);
params.should_clear_history_list = true;
params.initiator_origin = url::Origin(); // Opaque initiator.
web_contents->GetController().LoadURLWithParams(params);
// The navigation might have to wait for before unload handler to execute. The
// remaining of the logic continues in:
// |WebTestControlHost::DidFinishNavigation|.
}
void WebTestControlHost::ResetRendererAfterWebTestDone() {
void WebTestControlHost::DidFinishNavigation(NavigationHandle* navigation) {
if (navigation->GetURL() != GURL(kAboutBlankResetWebTest))
return;
// Request additional web test specific cleanup in the renderer process:
content::WebContentsImpl* web_contents =
static_cast<WebContentsImpl*>(main_window_->web_contents());
RenderProcessHost* main_rfh_process =
web_contents->GetMainFrame()->GetProcess();
GetWebTestRenderThreadRemote(main_rfh_process)->ResetRendererAfterWebTest();
PrepareRendererForNextWebTestDone();
}
void WebTestControlHost::PrepareRendererForNextWebTestDone() {
if (leak_detector_ && main_window_) {
// When doing leak detection, we don't want to count opened windows as
// leaks, unless the test specifies that it expects to have closed them

@ -187,6 +187,7 @@ class WebTestControlHost : public WebContentsObserver,
RenderViewHost* new_host) override;
void RenderViewDeleted(RenderViewHost* render_view_host) override;
void ReadyToCommitNavigation(NavigationHandle* navigation_handle) override;
void DidFinishNavigation(NavigationHandle* navigation) override;
// RenderProcessHostObserver implementation.
void RenderProcessHostDestroyed(
@ -271,11 +272,15 @@ class WebTestControlHost : public WebContentsObserver,
void OnLeakDetectionDone(int pid,
const LeakDetector::LeakDetectionReport& report);
// At the end of the test, once browser-side cleanup is done, commence reset
// of the renderer process that will stick around.
void ResetRendererAfterWebTest();
// Callback for when the renderer completes its reset at the end of the test.
void ResetRendererAfterWebTestDone();
// In between two tests, do some cleanup. Navigate to about:blank and
// request blink to reset states.
//
// Note: If the current document had COOP:same-origin defined, the navigation
// to about:blank will have to happen in a different browsing context group.
// The process used might be a different one.
void PrepareRendererForNextWebTest();
void PrepareRendererForNextWebTestDone();
void OnPixelDumpCaptured(const SkBitmap& snapshot);
void ReportResults();
void EnqueueSurfaceCopyRequest();

@ -140,11 +140,16 @@ interface WebTestRenderThread {
// conclude the test. Used to bounce this request from another renderer.
TestFinishedFromSecondaryRenderer();
// Tells the renderer to navigate the main window's main frame to about:blank
// and reset all state in preparation for the next test. This happens before
// Reset all state in preparation for the next test. This happens before
// leak detection in order for the test harness to drop anything that tests
// passed in and which could cause a leak otherwise.
ResetRendererAfterWebTest() => ();
// The flow is:
// - browser process detects test is finished
// - browser process initiates navigation to about:blank and waits for the
// document to commit.
// - browser process calls ResetRendererAfterWebTest() for the renderer
// process that about:blank committed in.
ResetRendererAfterWebTest();
// Processes a work item. This is called for the process with the main
// window's main frame.

@ -2667,37 +2667,14 @@ void TestRunner::TestFinishedFromSecondaryRenderer() {
NotifyDone();
}
void TestRunner::ResetRendererAfterWebTest(base::OnceClosure done_callback) {
// Instead of resetting for the next test here, delay until after the
// navigation to about:blank, which is heard about in in
// `DidCommitNavigationInMainFrame()`. This ensures we reset settings that are
// set between now and the load of about:blank, and that no new changes or
// loads can be started by the renderer.
waiting_for_reset_navigation_to_about_blank_ = std::move(done_callback);
void TestRunner::ResetRendererAfterWebTest() {
WebFrameTestProxy* main_frame = FindInProcessMainWindowMainFrame();
DCHECK(main_frame);
main_frame->GetWebFrame()->ResetForTesting();
}
void TestRunner::DidCommitNavigationInMainFrame(WebFrameTestProxy* main_frame) {
// This method is just meant to catch the about:blank navigation started in
// ResetRendererAfterWebTest().
if (!waiting_for_reset_navigation_to_about_blank_)
return;
// This would mean some other navigation was already happening when the test
// ended, the about:blank should still be coming.
GURL url = main_frame->GetWebFrame()->GetDocumentLoader()->GetUrl();
if (!url.IsAboutBlank())
return;
// Perform the reset now that the main frame is on about:blank.
main_frame->Reset();
// When the about:blank navigation happens in a new process, the new
// WebFrameTestProxy is not designated to be the "MainWindowMainFrame" one
// yet. It will be tracked later after receiving the SetTestConfiguration IPC.
if (main_frame)
main_frame->Reset();
Reset();
// Ack to the browser.
std::move(waiting_for_reset_navigation_to_about_blank_).Run();
}
void TestRunner::AddMainFrame(WebFrameTestProxy* frame) {

@ -98,12 +98,8 @@ class TestRunner {
void TestFinishedFromSecondaryRenderer();
// Performs a reset at the end of a test, in order to prepare for the next
// test. This includes a navigation to about:blank, which we hear about
// through DidCommitNavigationInMainFrame().
void ResetRendererAfterWebTest(base::OnceClosure done_callback);
// Listener for navigations in order to hear about the navigation to
// about:blank done for ResetRendererAfterWebTest().
void DidCommitNavigationInMainFrame(WebFrameTestProxy* main_frame);
// test.
void ResetRendererAfterWebTest();
// Track the set of all main frames in the process, which is also the set of
// windows rooted in this process.
@ -587,12 +583,6 @@ class TestRunner {
blink::WebEffectiveConnectionType effective_connection_type_ =
blink::WebEffectiveConnectionType::kTypeUnknown;
// Set to ack callback when the browser asks the renderer to reset at the end
// of a test. Part of reset involves performing a navigation to about:blank
// and this tracks that the navigation is in progress, and is called to inform
// the browser that the reset is complete.
base::OnceClosure waiting_for_reset_navigation_to_about_blank_;
mojom::WebTestRunTestConfiguration test_config_;
base::WeakPtrFactory<TestRunner> weak_factory_{this};

@ -164,14 +164,9 @@ class TestRenderFrameObserver : public RenderFrameObserver {
test_runner_->PrintMessage(description + " - didCommitLoadForFrame\n");
}
if (render_frame()->IsMainFrame()) {
// Track main frames once they are swapped in, if they started
// provisional.
// Track main frames once they are swapped in, if they started provisional.
if (render_frame()->IsMainFrame())
test_runner_->AddMainFrame(frame_proxy());
// Looking for navigations to about:blank after a test completes.
test_runner_->DidCommitNavigationInMainFrame(frame_proxy());
}
}
void DidFinishSameDocumentNavigation() override {
@ -270,6 +265,7 @@ void WebFrameTestProxy::Reset() {
CHECK(IsMainFrame());
if (IsMainFrame()) {
GetWebFrame()->ClearActiveFindMatchForTesting();
GetWebFrame()->SetName(blink::WebString());
GetWebFrame()->ClearOpener();

@ -75,9 +75,8 @@ void WebTestRenderThreadObserver::TestFinishedFromSecondaryRenderer() {
test_runner_->TestFinishedFromSecondaryRenderer();
}
void WebTestRenderThreadObserver::ResetRendererAfterWebTest(
base::OnceClosure done_callback) {
test_runner_->ResetRendererAfterWebTest(std::move(done_callback));
void WebTestRenderThreadObserver::ResetRendererAfterWebTest() {
test_runner_->ResetRendererAfterWebTest();
}
void WebTestRenderThreadObserver::ProcessWorkItem(

@ -38,7 +38,7 @@ class WebTestRenderThreadObserver : public RenderThreadObserver,
void ReplicateWebTestRuntimeFlagsChanges(
base::Value changed_layout_test_runtime_flags) override;
void TestFinishedFromSecondaryRenderer() override;
void ResetRendererAfterWebTest(base::OnceClosure done_callback) override;
void ResetRendererAfterWebTest() override;
void ProcessWorkItem(mojom::WorkItemPtr work_item) override;
void ReplicateWorkQueueStates(base::Value work_queue_states) override;

@ -59,7 +59,6 @@ class WebSecurityOrigin {
BLINK_PLATFORM_EXPORT static WebSecurityOrigin CreateFromString(
const WebString&);
BLINK_PLATFORM_EXPORT static WebSecurityOrigin Create(const WebURL&);
BLINK_PLATFORM_EXPORT static WebSecurityOrigin CreateUniqueOpaque();
BLINK_PLATFORM_EXPORT void Reset();
BLINK_PLATFORM_EXPORT void Assign(const WebSecurityOrigin&);

@ -837,8 +837,8 @@ class WebLocalFrame : public WebFrame {
virtual void UpdateCurrentHistoryItem() = 0;
virtual PageState CurrentHistoryItemToPageState() = 0;
virtual const WebHistoryItem& GetCurrentHistoryItem() const = 0;
// Reset TextFinder state and loads about:blank.
virtual void ResetForTesting() = 0;
// Reset TextFinder state for the web test runner in between two tests.
virtual void ClearActiveFindMatchForTesting() = 0;
protected:
explicit WebLocalFrame(mojom::TreeScopeType scope,

@ -1072,19 +1072,10 @@ void WebLocalFrameImpl::ReloadImage(const WebNode& web_node) {
image_element->ForceReload();
}
void WebLocalFrameImpl::ResetForTesting() {
void WebLocalFrameImpl::ClearActiveFindMatchForTesting() {
DCHECK(GetFrame());
if (GetTextFinder())
GetTextFinder()->ClearActiveFindMatch();
ResourceRequest resource_request(url::kAboutBlankURL);
resource_request.SetMode(network::mojom::RequestMode::kNavigate);
resource_request.SetRedirectMode(network::mojom::RedirectMode::kManual);
resource_request.SetRequestContext(
mojom::blink::RequestContextType::INTERNAL);
resource_request.SetRequestorOrigin(
blink::WebSecurityOrigin::CreateUniqueOpaque());
FrameLoadRequest request(nullptr, resource_request);
GetFrame()->Loader().StartNavigation(request, WebFrameLoadType::kStandard);
}
WebDocumentLoader* WebLocalFrameImpl::GetDocumentLoader() const {

@ -133,7 +133,7 @@ class CORE_EXPORT WebLocalFrameImpl final
const override;
void SendPings(const WebURL& destination_url) override;
void StartReload(WebFrameLoadType) override;
void ResetForTesting() override;
void ClearActiveFindMatchForTesting() override;
void EnableViewSourceMode(bool enable) override;
bool IsViewSourceModeEnabled() const override;
WebDocumentLoader* GetDocumentLoader() const override;

@ -45,10 +45,6 @@ WebSecurityOrigin WebSecurityOrigin::Create(const WebURL& url) {
return WebSecurityOrigin(SecurityOrigin::Create(url));
}
WebSecurityOrigin WebSecurityOrigin::CreateUniqueOpaque() {
return WebSecurityOrigin(SecurityOrigin::CreateUniqueOpaque());
}
void WebSecurityOrigin::Reset() {
private_ = nullptr;
}