Commit f2fe5a5b authored by Pavel Feldman's avatar Pavel Feldman Committed by Commit Bot

Revert "DevTools: change Target.disposeBrowserContext to force-close WebContents"

This reverts commit 191b1ead.

Reason for revert: We can't process protocol synchronously in the headless embedder.

Original change's description:
> DevTools: change Target.disposeBrowserContext to force-close WebContents
> 
> This patch changes the behavior of Target.disposeBrowserContext method
> to force-close all the belonging WebContents.
> 
> R=​dgozman
> BUG=836272
> 
> Change-Id: Iec3da859363120a22c2648b31102457aee20e6cb
> Reviewed-on: https://chromium-review.googlesource.com/1031774
> Commit-Queue: Andrey Lushnikov <lushnikov@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
> Reviewed-by: Alex Clarke <alexclarke@chromium.org>
> Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#555640}

TBR=dgozman@chromium.org,sky@chromium.org,lushnikov@chromium.org,caseq@chromium.org,alexclarke@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 836272
Change-Id: Ib5d06ad2ce6816a82c0819c693b2eb2819d3a502
Reviewed-on: https://chromium-review.googlesource.com/1045525Reviewed-by: default avatarPavel Feldman <pfeldman@chromium.org>
Commit-Queue: Pavel Feldman <pfeldman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556276}
parent 5fd349ce
......@@ -19,7 +19,6 @@
{
"domain": "Target",
"include": [ "setRemoteLocations", "createBrowserContext", "createTarget", "disposeBrowserContext" ],
"async": ["disposeBrowserContext"],
"include_events": []
},
{
......
......@@ -22,7 +22,6 @@ TargetHandler::~TargetHandler() {
if (delegate)
delegate->UpdateDeviceDiscovery();
registrations_.clear();
BrowserList::RemoveObserver(this);
}
protocol::Response TargetHandler::SetRemoteLocations(
......@@ -112,49 +111,23 @@ void TargetHandler::OnOriginalProfileDestroyed(Profile* profile) {
});
}
void TargetHandler::DisposeBrowserContext(
protocol::Response TargetHandler::DisposeBrowserContext(
const std::string& context_id,
std::unique_ptr<DisposeBrowserContextCallback> callback) {
if (pending_context_disposals_.find(context_id) !=
pending_context_disposals_.end()) {
callback->sendFailure(protocol::Response::Error(
"Disposing of browser context " + context_id + " is already pending"));
return;
}
bool* out_success) {
auto it = registrations_.find(context_id);
if (it == registrations_.end()) {
callback->sendFailure(protocol::Response::Error(
"Failed to find browser context with id " + context_id));
return;
return protocol::Response::Error("Failed to find browser context with id " +
context_id);
}
if (pending_context_disposals_.empty())
BrowserList::AddObserver(this);
pending_context_disposals_[context_id] = std::move(callback);
Profile* profile = it->second->profile();
BrowserList::CloseAllBrowsersWithIncognitoProfile(
profile, base::DoNothing(), base::DoNothing(),
true /* skip_beforeunload */);
}
void TargetHandler::OnBrowserRemoved(Browser* browser) {
std::string context_id = browser->profile()->UniqueId();
auto pending_disposal = pending_context_disposals_.find(context_id);
if (pending_disposal == pending_context_disposals_.end())
return;
for (auto* opened_browser : *BrowserList::GetInstance()) {
if (opened_browser->profile() == browser->profile())
return;
for (auto* browser : *BrowserList::GetInstance()) {
if (browser->profile() == profile &&
!browser->IsAttemptingToCloseBrowser()) {
*out_success = false;
return protocol::Response::OK();
}
}
auto it = registrations_.find(context_id);
// We cannot delete immediately here: the profile might still be referenced
// during the browser tier-down process.
base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE,
it->second.release());
registrations_.erase(it);
pending_disposal->second->sendSuccess();
pending_context_disposals_.erase(pending_disposal);
if (pending_context_disposals_.empty())
BrowserList::RemoveObserver(this);
*out_success = true;
return protocol::Response::OK();
}
......@@ -11,14 +11,11 @@
#include "chrome/browser/devtools/protocol/forward.h"
#include "chrome/browser/devtools/protocol/target.h"
#include "chrome/browser/media/router/presentation/independent_otr_profile_manager.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_list_observer.h"
#include "net/base/host_port_pair.h"
using RemoteLocations = std::set<net::HostPortPair>;
class TargetHandler : public protocol::Target::Backend,
public BrowserListObserver {
class TargetHandler : public protocol::Target::Backend {
public:
explicit TargetHandler(protocol::UberDispatcher* dispatcher);
~TargetHandler() override;
......@@ -37,22 +34,16 @@ class TargetHandler : public protocol::Target::Backend,
protocol::Maybe<std::string> browser_context_id,
protocol::Maybe<bool> enable_begin_frame_control,
std::string* out_target_id) override;
void DisposeBrowserContext(
const std::string& context_id,
std::unique_ptr<DisposeBrowserContextCallback> callback) override;
protocol::Response DisposeBrowserContext(const std::string& context_id,
bool* out_success) override;
private:
void OnOriginalProfileDestroyed(Profile* profile);
void OnBrowserRemoved(Browser* browser) override;
base::flat_map<
std::string,
std::unique_ptr<IndependentOTRProfileManager::OTRProfileRegistration>>
registrations_;
base::flat_map<std::string, std::unique_ptr<DisposeBrowserContextCallback>>
pending_context_disposals_;
RemoteLocations remote_locations_;
base::WeakPtrFactory<TargetHandler> weak_factory_;
......
......@@ -36,15 +36,6 @@ BrowserList::BrowserVector GetBrowsersToClose(Profile* profile) {
return browsers_to_close;
}
BrowserList::BrowserVector GetIncognitoBrowsersToClose(Profile* profile) {
BrowserList::BrowserVector browsers_to_close;
for (auto* browser : *BrowserList::GetInstance()) {
if (browser->profile() == profile)
browsers_to_close.push_back(browser);
}
return browsers_to_close;
}
} // namespace
// static
......@@ -162,18 +153,6 @@ void BrowserList::CloseAllBrowsersWithProfile(
skip_beforeunload);
}
// static
void BrowserList::CloseAllBrowsersWithIncognitoProfile(
Profile* profile,
const CloseCallback& on_close_success,
const CloseCallback& on_close_aborted,
bool skip_beforeunload) {
DCHECK(profile->IsOffTheRecord());
TryToCloseBrowserList(GetIncognitoBrowsersToClose(profile), on_close_success,
on_close_aborted, profile->GetPath(),
skip_beforeunload);
}
// static
void BrowserList::TryToCloseBrowserList(const BrowserVector& browsers_to_close,
const CloseCallback& on_close_success,
......
......@@ -108,14 +108,6 @@ class BrowserList {
const CloseCallback& on_close_aborted,
bool skip_beforeunload);
// Similarly to CloseAllBrowsersWithProfile, but DCHECK's that profile is
// Off-the-Record and doesn't close browsers with the original profile.
static void CloseAllBrowsersWithIncognitoProfile(
Profile* profile,
const CloseCallback& on_close_success,
const CloseCallback& on_close_aborted,
bool skip_beforeunload);
// Returns true if at least one incognito session is active across all
// desktops.
static bool IsIncognitoSessionActive();
......
......@@ -14,7 +14,6 @@
#include "build/build_config.h"
#include "chrome/browser/chrome_notification_types.h"
#include "chrome/browser/net/url_request_mock_util.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_commands.h"
#include "chrome/browser/ui/browser_list.h"
......@@ -962,35 +961,6 @@ IN_PROC_BROWSER_TEST_F(FastUnloadTest,
EXPECT_EQ(0, unload_results.get_aborts());
}
IN_PROC_BROWSER_TEST_F(FastUnloadTest,
BrowserListForceCloseWithIncognitoProfileCloseProfiles) {
Profile* otr_profile = browser()->profile()->GetOffTheRecordProfile();
Browser* otr_browser1 = CreateBrowser(otr_profile);
Browser* otr_browser2 = CreateBrowser(otr_profile);
content::WindowedNotificationObserver otr_browser1_observer(
chrome::NOTIFICATION_BROWSER_CLOSED,
content::Source<Browser>(otr_browser1));
content::WindowedNotificationObserver otr_browser2_observer(
chrome::NOTIFICATION_BROWSER_CLOSED,
content::Source<Browser>(otr_browser2));
UnloadResults unload_results;
BrowserList::CloseAllBrowsersWithIncognitoProfile(
otr_profile,
base::BindRepeating(&UnloadResults::AddSuccess,
base::Unretained(&unload_results)),
base::BindRepeating(&UnloadResults::AddAbort,
base::Unretained(&unload_results)),
true);
// Incognito browsers should be closed.
otr_browser1_observer.Wait();
otr_browser2_observer.Wait();
// Browser with original profile should not close.
EXPECT_EQ(1, browser()->tab_strip_model()->count());
EXPECT_EQ(1, unload_results.get_successes());
EXPECT_EQ(0, unload_results.get_aborts());
}
IN_PROC_BROWSER_TEST_F(FastUnloadTest,
BrowserListForceCloseAfterNormalCloseWithFastUnload) {
NavigateToDataURL(BEFORE_UNLOAD_HTML, "beforeunload");
......
......@@ -392,7 +392,8 @@ Response TargetHandler::CreateBrowserContext(std::string* out_context_id) {
return Response::Error("Not supported");
}
Response TargetHandler::DisposeBrowserContext(const std::string& context_id) {
Response TargetHandler::DisposeBrowserContext(const std::string& context_id,
bool* out_success) {
return Response::Error("Not supported");
}
......
......@@ -63,7 +63,8 @@ class TargetHandler : public DevToolsDomainHandler,
Response CloseTarget(const std::string& target_id,
bool* out_success) override;
Response CreateBrowserContext(std::string* out_context_id) override;
Response DisposeBrowserContext(const std::string& context_id) override;
Response DisposeBrowserContext(const std::string& context_id,
bool* out_success) override;
Response CreateTarget(const std::string& url,
Maybe<int> width,
Maybe<int> height,
......
......@@ -200,11 +200,24 @@ bool HeadlessDevToolsClientImpl::DispatchEvent(
NOTREACHED() << "Badly formed event parameters";
return false;
}
it->second.Run(*result_dict);
// DevTools assumes event handling is async so we must post a task here or
// we risk breaking things.
browser_main_thread_->PostTask(
FROM_HERE,
base::BindOnce(&HeadlessDevToolsClientImpl::DispatchEventTask,
weak_ptr_factory_.GetWeakPtr(),
std::move(owning_message), &it->second, result_dict));
}
return true;
}
void HeadlessDevToolsClientImpl::DispatchEventTask(
std::unique_ptr<base::Value> owning_message,
const EventHandler* event_handler,
const base::DictionaryValue* result_dict) {
event_handler->Run(*result_dict);
}
void HeadlessDevToolsClientImpl::AgentHostClosed(
content::DevToolsAgentHost* agent_host) {
DCHECK_EQ(agent_host_, agent_host);
......
......@@ -82,21 +82,16 @@ Response TargetHandler::CreateBrowserContext(std::string* out_context_id) {
return Response::OK();
}
Response TargetHandler::DisposeBrowserContext(const std::string& context_id) {
Response TargetHandler::DisposeBrowserContext(const std::string& context_id,
bool* out_success) {
HeadlessBrowserContext* context =
browser()->GetBrowserContextForId(context_id);
if (!context)
return Response::InvalidParams("browserContextId");
std::vector<HeadlessWebContents*> web_contents = context->GetAllWebContents();
while (!web_contents.empty()) {
for (auto* wc : web_contents)
wc->Close();
// Since HeadlessWebContents::Close spawns a nested run loop to await
// closing, new web_contents could be opened. We need to re-query pages and
// close them too.
web_contents = context->GetAllWebContents();
*out_success = false;
if (context && context != browser()->GetDefaultBrowserContext() &&
context->GetAllWebContents().empty()) {
*out_success = true;
context->Close();
}
return Response::OK();
}
......
......@@ -28,7 +28,8 @@ class TargetHandler : public DomainHandler, public Target::Backend {
Response CloseTarget(const std::string& target_id,
bool* out_success) override;
Response CreateBrowserContext(std::string* out_context_id) override;
Response DisposeBrowserContext(const std::string& context_id) override;
Response DisposeBrowserContext(const std::string& context_id,
bool* out_success) override;
private:
DISALLOW_COPY_AND_ASSIGN(TargetHandler);
......
......@@ -513,6 +513,7 @@ class TargetDomainCreateAndDeleteBrowserContextTest
void OnDisposeBrowserContextResult(
std::unique_ptr<target::DisposeBrowserContextResult> result) {
EXPECT_TRUE(result->GetSuccess());
FinishAsynchronousTest();
}
......@@ -522,27 +523,16 @@ class TargetDomainCreateAndDeleteBrowserContextTest
HEADLESS_ASYNC_DEVTOOLED_TEST_F(TargetDomainCreateAndDeleteBrowserContextTest);
class TargetDomainDisposeContextSucceedsIfInUse
: public target::Observer,
public HeadlessAsyncDevTooledBrowserTest {
class TargetDomainDisposeContextFailsIfInUse
: public HeadlessAsyncDevTooledBrowserTest {
void RunDevTooledTest() override {
EXPECT_TRUE(embedded_test_server()->Start());
EXPECT_EQ(1u, GetAllWebContents(browser()).size());
devtools_client_->GetTarget()->AddObserver(this);
devtools_client_->GetTarget()->SetDiscoverTargets(
target::SetDiscoverTargetsParams::Builder().SetDiscover(true).Build(),
base::BindOnce(&TargetDomainDisposeContextSucceedsIfInUse::
OnDiscoverTargetsEnabled,
base::Unretained(this)));
}
void OnDiscoverTargetsEnabled(
std::unique_ptr<target::SetDiscoverTargetsResult> result) {
EXPECT_EQ(1u, GetAllWebContents(browser()).size());
devtools_client_->GetTarget()->GetExperimental()->CreateBrowserContext(
target::CreateBrowserContextParams::Builder().Build(),
base::BindOnce(
&TargetDomainDisposeContextSucceedsIfInUse::OnContextCreated,
&TargetDomainDisposeContextFailsIfInUse::OnContextCreated,
base::Unretained(this)));
}
......@@ -556,7 +546,7 @@ class TargetDomainDisposeContextSucceedsIfInUse
.SetBrowserContextId(context_id_)
.Build(),
base::BindOnce(
&TargetDomainDisposeContextSucceedsIfInUse::OnCreateTargetResult,
&TargetDomainDisposeContextFailsIfInUse::OnCreateTargetResult,
base::Unretained(this)));
}
......@@ -564,38 +554,54 @@ class TargetDomainDisposeContextSucceedsIfInUse
std::unique_ptr<target::CreateTargetResult> result) {
page_id_ = result->GetTargetId();
destroyed_targets_.clear();
devtools_client_->GetTarget()->GetExperimental()->DisposeBrowserContext(
target::DisposeBrowserContextParams::Builder()
.SetBrowserContextId(context_id_)
.Build(),
base::BindOnce(&TargetDomainDisposeContextSucceedsIfInUse::
base::BindOnce(&TargetDomainDisposeContextFailsIfInUse::
OnDisposeBrowserContextResult,
base::Unretained(this)));
}
void OnTargetDestroyed(const target::TargetDestroyedParams& params) override {
destroyed_targets_.push_back(params.GetTargetId());
void OnDisposeBrowserContextResult(
std::unique_ptr<target::DisposeBrowserContextResult> result) {
EXPECT_FALSE(result->GetSuccess());
// Close the page and try again.
devtools_client_->GetTarget()->GetExperimental()->CloseTarget(
target::CloseTargetParams::Builder().SetTargetId(page_id_).Build(),
base::BindOnce(
&TargetDomainDisposeContextFailsIfInUse::OnCloseTargetResult,
base::Unretained(this)));
}
void OnDisposeBrowserContextResult(
void OnCloseTargetResult(std::unique_ptr<target::CloseTargetResult> result) {
EXPECT_TRUE(result->GetSuccess());
devtools_client_->GetTarget()->GetExperimental()->DisposeBrowserContext(
target::DisposeBrowserContextParams::Builder()
.SetBrowserContextId(context_id_)
.Build(),
base::BindOnce(&TargetDomainDisposeContextFailsIfInUse::
OnDisposeBrowserContextResult2,
base::Unretained(this)));
}
void OnDisposeBrowserContextResult2(
std::unique_ptr<target::DisposeBrowserContextResult> result) {
EXPECT_EQ(destroyed_targets_.size(), 1u);
EXPECT_EQ(destroyed_targets_[0], page_id_);
devtools_client_->GetTarget()->RemoveObserver(this);
EXPECT_TRUE(result->GetSuccess());
FinishAsynchronousTest();
}
private:
std::vector<std::string> destroyed_targets_;
std::string context_id_;
std::string page_id_;
};
HEADLESS_ASYNC_DEVTOOLED_TEST_F(TargetDomainDisposeContextSucceedsIfInUse);
HEADLESS_ASYNC_DEVTOOLED_TEST_F(TargetDomainDisposeContextFailsIfInUse);
class TargetDomainCreateTwoContexts : public HeadlessAsyncDevTooledBrowserTest,
public target::Observer,
public target::ExperimentalObserver,
public page::Observer {
public:
void RunDevTooledTest() override {
......@@ -606,15 +612,7 @@ class TargetDomainCreateTwoContexts : public HeadlessAsyncDevTooledBrowserTest,
devtools_client_->GetPage()->Enable(run_loop.QuitClosure());
run_loop.Run();
devtools_client_->GetTarget()->AddObserver(this);
devtools_client_->GetTarget()->SetDiscoverTargets(
target::SetDiscoverTargetsParams::Builder().SetDiscover(true).Build(),
base::BindOnce(&TargetDomainCreateTwoContexts::OnDiscoverTargetsEnabled,
base::Unretained(this)));
}
void OnDiscoverTargetsEnabled(
std::unique_ptr<target::SetDiscoverTargetsResult> result) {
devtools_client_->GetTarget()->GetExperimental()->AddObserver(this);
devtools_client_->GetTarget()->GetExperimental()->CreateBrowserContext(
target::CreateBrowserContextParams::Builder().Build(),
base::BindOnce(&TargetDomainCreateTwoContexts::OnContextOneCreated,
......@@ -811,18 +809,18 @@ class TargetDomainCreateTwoContexts : public HeadlessAsyncDevTooledBrowserTest,
EXPECT_EQ("", value_value->GetString())
<< "Page 2 should not share cookies from page one";
devtools_client_->GetTarget()->GetExperimental()->DisposeBrowserContext(
target::DisposeBrowserContextParams::Builder()
.SetBrowserContextId(context_id_one_)
devtools_client_->GetTarget()->GetExperimental()->CloseTarget(
target::CloseTargetParams::Builder()
.SetTargetId(page_id_one_)
.Build(),
base::BindOnce(&TargetDomainCreateTwoContexts::OnCloseContext,
base::BindOnce(&TargetDomainCreateTwoContexts::OnCloseTarget,
base::Unretained(this)));
devtools_client_->GetTarget()->GetExperimental()->DisposeBrowserContext(
target::DisposeBrowserContextParams::Builder()
.SetBrowserContextId(context_id_two_)
devtools_client_->GetTarget()->GetExperimental()->CloseTarget(
target::CloseTargetParams::Builder()
.SetTargetId(page_id_two_)
.Build(),
base::BindOnce(&TargetDomainCreateTwoContexts::OnCloseContext,
base::BindOnce(&TargetDomainCreateTwoContexts::OnCloseTarget,
base::Unretained(this)));
devtools_client_->GetTarget()->GetExperimental()->RemoveObserver(this);
......@@ -830,17 +828,33 @@ class TargetDomainCreateTwoContexts : public HeadlessAsyncDevTooledBrowserTest,
}
}
void OnTargetDestroyed(const target::TargetDestroyedParams& params) override {
++page_close_count_;
void OnCloseTarget(std::unique_ptr<target::CloseTargetResult> result) {
page_close_count_++;
if (page_close_count_ < 2)
return;
devtools_client_->GetTarget()->GetExperimental()->DisposeBrowserContext(
target::DisposeBrowserContextParams::Builder()
.SetBrowserContextId(context_id_one_)
.Build(),
base::BindOnce(&TargetDomainCreateTwoContexts::OnCloseContext,
base::Unretained(this)));
devtools_client_->GetTarget()->GetExperimental()->DisposeBrowserContext(
target::DisposeBrowserContextParams::Builder()
.SetBrowserContextId(context_id_two_)
.Build(),
base::BindOnce(&TargetDomainCreateTwoContexts::OnCloseContext,
base::Unretained(this)));
}
void OnCloseContext(
std::unique_ptr<target::DisposeBrowserContextResult> result) {
EXPECT_TRUE(result->GetSuccess());
if (++context_closed_count_ < 2)
return;
EXPECT_EQ(page_close_count_, 2);
devtools_client_->GetTarget()->RemoveObserver(this);
FinishAsynchronousTest();
}
......
......@@ -5537,7 +5537,7 @@ domain Target
optional integer width
# Frame height in DIP (headless chrome only).
optional integer height
# The browser context to create the page in.
# The browser context to create the page in (headless chrome only).
optional BrowserContextID browserContextId
# Whether BeginFrames for this target will be controlled via DevTools (headless chrome only,
# not supported on MacOS yet, false by default).
......@@ -5554,11 +5554,12 @@ domain Target
# Deprecated.
deprecated optional TargetID targetId
# Deletes a BrowserContext. All the belonging pages will be closed without calling their
# beforeunload hooks.
# Deletes a BrowserContext, will fail of any open page uses it.
experimental command disposeBrowserContext
parameters
BrowserContextID browserContextId
returns
boolean success
# Returns information about a target.
experimental command getTargetInfo
......
......@@ -1125,12 +1125,13 @@
this.assertEquals(await evalCode(target2, 'localStorage.getItem("page1")'), null);
this.assertEquals(await evalCode(target2, 'localStorage.getItem("page2")'), 'page2');
const removedTargets = [];
SDK.targetManager.observeTargets({targetAdded: () => {}, targetRemoved: target => removedTargets.push(target)});
await Promise.all([disposeBrowserContext(browserContextIds[0]), disposeBrowserContext(browserContextIds[1])]);
this.assertEquals(removedTargets.length, 2);
this.assertEquals(removedTargets.indexOf(target1) !== -1, true);
this.assertEquals(removedTargets.indexOf(target2) !== -1, true);
this.assertEquals(await disposeBrowserContext(browserContextIds[0]), false);
this.assertEquals(await disposeBrowserContext(browserContextIds[1]), false);
await closeTarget(target1);
await closeTarget(target2);
this.assertEquals(await disposeBrowserContext(browserContextIds[0]), true);
this.assertEquals(await disposeBrowserContext(browserContextIds[1]), true);
this.releaseControl();
......@@ -1153,9 +1154,15 @@
return target;
}
async function closeTarget(target) {
const targetAgent = SDK.targetManager.mainTarget().targetAgent();
await targetAgent.invoke_closeTarget({targetId: target.id()});
}
async function disposeBrowserContext(browserContextId) {
const targetAgent = SDK.targetManager.mainTarget().targetAgent();
await targetAgent.invoke_disposeBrowserContext({browserContextId});
const {success} = await targetAgent.invoke_disposeBrowserContext({browserContextId});
return success;
}
async function evalCode(target, code) {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment