Skip to content

Commit

Permalink
Don't block on bowsingContext.close
Browse files Browse the repository at this point in the history 
ChromeDriver session used to block while handling browsingContext.close
command. This was needed to close the browser if the last window was
closed.
This commit implements a different mechanism to determine if the browser
needs to be closed. This allows handling the beforeunload prompt because
the browsingContext.close command no longer blocks.

Bug: 42323838, 379049702
Change-Id: I6bb4a72837a20861d576457e2a14e7c0efbd33a5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6080431
Auto-Submit: Vladimir Nechaev <[email protected]>
Commit-Queue: Vladimir Nechaev <[email protected]>
Reviewed-by: Alex N. Jose <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1394766}
  • Loading branch information
@nechaev-chromium
nechaev-chromium authored and Chromium LUCI CQ committed Dec 11, 2024
1 parent 078289a commit d619531
Show file tree
Hide file tree
Showing 23 changed files with 367 additions and 276 deletions.
9 changes: 6 additions & 3 deletions chrome/chrome.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,11 @@

#include "base/values.h"

struct BrowserInfo;
class ChromeDesktopImpl;
class DevToolsClient;
class Status;
class WebView;
struct BrowserInfo;

class Chrome {
public:
Expand Down Expand Up @@ -46,8 +47,7 @@ class Chrome {
// Return number of opened tabs without updating the internal maps
// it's needed for BiDi to prevent trying to attach to sessions when
// classic commands are not used.
virtual Status GetWebViewCount(size_t* web_view_count,
bool w3c_compliant) = 0;
virtual int GetWebViewCount() const = 0;

// Return the id of the first WebView that is a page.
virtual Status GetWebViewIdForFirstTab(std::string* web_view_id,
Expand Down Expand Up @@ -120,6 +120,9 @@ class Chrome {

// Quits Chrome.
virtual Status Quit() = 0;

// Get the browser wide client.
virtual DevToolsClient* Client() const = 0;
};

#endif // CHROME_TEST_CHROMEDRIVER_CHROME_CHROME_H_
13 changes: 2 additions & 11 deletions chrome/chrome_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,17 +115,8 @@ Status ChromeImpl::GetWebViewIdForFirstTab(std::string* web_view_id,
return Status(kUnknownError, "unable to discover open window in chrome");
}

Status ChromeImpl::GetWebViewCount(size_t* web_view_count, bool w3c_compliant) {
WebViewsInfo views_info;
Status status = target_utils::GetTopLevelViewsInfo(
*devtools_websocket_client_, nullptr, views_info);
if (status.IsError()) {
return status;
}

*web_view_count = views_info.GetSize();

return Status(kOk);
int ChromeImpl::GetWebViewCount() const {
return web_views_.size();
}

Status ChromeImpl::GetTopLevelWebViewIds(std::list<std::string>* web_view_ids,
Expand Down
4 changes: 2 additions & 2 deletions chrome/chrome_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ class ChromeImpl : public Chrome {
Status GetAsDesktop(ChromeDesktopImpl** desktop) override;
const BrowserInfo* GetBrowserInfo() const override;
bool HasCrashedWebView() override;
Status GetWebViewCount(size_t* web_view_count, bool w3c_compliant) override;
int GetWebViewCount() const override;
Status GetWebViewIdForFirstTab(std::string* web_view_id,
bool w3c_complaint) override;
Status GetTopLevelWebViewIds(std::list<std::string>* web_view_ids,
Expand Down Expand Up @@ -93,7 +93,7 @@ class ChromeImpl : public Chrome {
bool HasTouchScreen() const override;
std::string page_load_strategy() const override;
Status Quit() override;
DevToolsClient* Client() const;
DevToolsClient* Client() const override;

protected:
ChromeImpl(BrowserInfo browser_info,
Expand Down
18 changes: 2 additions & 16 deletions chrome/devtools_client_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -265,17 +265,10 @@ DevToolsClientImpl::DevToolsClientImpl(const std::string& id,
is_tab_(is_tab) {}

DevToolsClientImpl::~DevToolsClientImpl() {
if (IsNull()) {
if (parent_ == nullptr) {
return;
}
if (parent_ != nullptr) {
parent_->UnregisterSessionHandler(session_id_);
} else {
// Resetting the callback is redundant as we assume
// that .dtor won't start a nested message loop.
// Doing this just in case.
socket_->SetNotificationCallback(base::RepeatingClosure());
}
parent_->UnregisterSessionHandler(session_id_);
}

void DevToolsClientImpl::SetParserFuncForTesting(
Expand Down Expand Up @@ -511,13 +504,6 @@ Status DevToolsClientImpl::SetSocket(std::unique_ptr<SyncWebSocket> socket) {
}
socket_ = std::move(socket);
socket_->SetId(id_);
// If error happens during proactive event consumption we ignore it
// as there is no active user request where the error might be returned.
// Unretained 'this' won't cause any problems as we reset the callback in the
// .dtor.
socket_->SetNotificationCallback(base::BindRepeating(
base::IgnoreResult(&DevToolsClientImpl::HandleReceivedEvents),
base::Unretained(this)));

return OnConnected();
}
Expand Down
8 changes: 6 additions & 2 deletions chrome/stub_chrome.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ bool StubChrome::HasCrashedWebView() {
return false;
}

Status StubChrome::GetWebViewCount(size_t* web_view_count, bool w3c_compliant) {
return Status(kOk);
int StubChrome::GetWebViewCount() const {
return 1;
}

Status StubChrome::GetWebViewIdForFirstTab(std::string* web_view_id,
Expand Down Expand Up @@ -113,3 +113,7 @@ std::string StubChrome::page_load_strategy() const {
Status StubChrome::Quit() {
return Status(kOk);
}

DevToolsClient* StubChrome::Client() const {
return nullptr;
}
3 changes: 2 additions & 1 deletion chrome/stub_chrome.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class StubChrome : public Chrome {
Status GetAsDesktop(ChromeDesktopImpl** desktop) override;
const BrowserInfo* GetBrowserInfo() const override;
bool HasCrashedWebView() override;
Status GetWebViewCount(size_t* web_view_count, bool w3c_compliant) override;
int GetWebViewCount() const override;
Status GetWebViewIdForFirstTab(std::string* web_view_id,
bool w3c_compliant) override;
Status GetTopLevelWebViewIds(std::list<std::string>* tab_view_ids,
Expand Down Expand Up @@ -54,6 +54,7 @@ class StubChrome : public Chrome {
bool HasTouchScreen() const override;
std::string page_load_strategy() const override;
Status Quit() override;
DevToolsClient* Client() const override;

private:
BrowserInfo browser_info_;
Expand Down
24 changes: 18 additions & 6 deletions chrome_launcher.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,7 @@ Status LaunchRemoteChromeSession(
const Capabilities& capabilities,
std::vector<std::unique_ptr<DevToolsEventListener>>
devtools_event_listeners,
base::RepeatingClosure on_socket_message,
std::unique_ptr<Chrome>& chrome) {
Status status(kOk);
std::unique_ptr<DevToolsHttpClient> devtools_http_client;
Expand All @@ -435,6 +436,7 @@ Status LaunchRemoteChromeSession(

std::unique_ptr<DevToolsClient> devtools_websocket_client;
std::unique_ptr<SyncWebSocket> socket = socket_factory.Run();
socket->SetNotificationCallback(std::move(on_socket_message));
BrowserInfo browser_info = *devtools_http_client->browser_info();
if (browser_info.web_socket_url.empty()) {
browser_info.web_socket_url =
Expand All @@ -460,6 +462,7 @@ Status LaunchDesktopChrome(network::mojom::URLLoaderFactory* factory,
const Capabilities& capabilities,
std::vector<std::unique_ptr<DevToolsEventListener>>
devtools_event_listeners,
base::RepeatingClosure on_socket_message,
bool w3c_compliant,
std::unique_ptr<Chrome>& chrome) {
base::CommandLine command(base::CommandLine::NO_PROGRAM);
Expand Down Expand Up @@ -656,6 +659,7 @@ Status LaunchDesktopChrome(network::mojom::URLLoaderFactory* factory,
}
if (ready_to_connect) {
socket = socket_factory.Run();
socket->SetNotificationCallback(std::move(on_socket_message));
browser_info = *(devtools_http_client->browser_info());
if (browser_info.web_socket_url.empty()) {
browser_info.web_socket_url =
Expand All @@ -677,6 +681,7 @@ Status LaunchDesktopChrome(network::mojom::URLLoaderFactory* factory,
if (status.IsOk()) {
socket = pipe_builder.TakeSocket();
DCHECK(socket);
socket->SetNotificationCallback(std::move(on_socket_message));
status = CreateBrowserwideDevToolsClientAndConnect(
std::move(socket), devtools_event_listeners,
browser_info.web_socket_url, !capabilities.web_socket_url,
Expand Down Expand Up @@ -793,6 +798,7 @@ Status LaunchAndroidChrome(network::mojom::URLLoaderFactory* factory,
std::vector<std::unique_ptr<DevToolsEventListener>>
devtools_event_listeners,
DeviceManager& device_manager,
base::RepeatingClosure on_socket_message,
std::unique_ptr<Chrome>& chrome) {
Status status(kOk);
std::unique_ptr<Device> device;
Expand Down Expand Up @@ -840,6 +846,7 @@ Status LaunchAndroidChrome(network::mojom::URLLoaderFactory* factory,

std::unique_ptr<DevToolsClient> devtools_websocket_client;
std::unique_ptr<SyncWebSocket> socket = socket_factory.Run();
socket->SetNotificationCallback(std::move(on_socket_message));
BrowserInfo browser_info = *devtools_http_client->browser_info();
if (browser_info.web_socket_url.empty()) {
browser_info.web_socket_url =
Expand All @@ -863,6 +870,7 @@ Status LaunchReplayChrome(network::mojom::URLLoaderFactory* factory,
const Capabilities& capabilities,
std::vector<std::unique_ptr<DevToolsEventListener>>
devtools_event_listeners,
base::RepeatingClosure on_socket_message,
bool w3c_compliant,
std::unique_ptr<Chrome>& chrome) {
base::CommandLine command(base::CommandLine::NO_PROGRAM);
Expand Down Expand Up @@ -898,6 +906,7 @@ Status LaunchReplayChrome(network::mojom::URLLoaderFactory* factory,
base::FilePath log_path(log_path_str);
std::unique_ptr<SyncWebSocket> socket =
std::make_unique<LogReplaySocket>(log_path);
socket->SetNotificationCallback(std::move(on_socket_message));
std::unique_ptr<DevToolsClient> devtools_websocket_client;
BrowserInfo browser_info = *devtools_http_client->browser_info();
if (browser_info.web_socket_url.empty()) {
Expand Down Expand Up @@ -960,28 +969,31 @@ Status LaunchChrome(network::mojom::URLLoaderFactory* factory,
const Capabilities& capabilities,
std::vector<std::unique_ptr<DevToolsEventListener>>
devtools_event_listeners,
base::RepeatingClosure on_socket_message,
bool w3c_compliant,
std::unique_ptr<Chrome>& chrome) {
if (capabilities.IsRemoteBrowser()) {
// TODO(johnchen): Clean up naming for ChromeDriver sessions created
// by connecting to an already-running Chrome at a given debuggerAddress.
return LaunchRemoteChromeSession(factory, socket_factory, capabilities,
std::move(devtools_event_listeners),
chrome);
std::move(on_socket_message), chrome);
}
const base::CommandLine* cmd_line = base::CommandLine::ForCurrentProcess();
if (capabilities.IsAndroid()) {
return LaunchAndroidChrome(factory, socket_factory, capabilities,
std::move(devtools_event_listeners),
device_manager, chrome);
device_manager, std::move(on_socket_message),
chrome);
} else if (cmd_line->HasSwitch("devtools-replay")) {
return LaunchReplayChrome(factory, capabilities,
std::move(devtools_event_listeners),
w3c_compliant, chrome);
return LaunchReplayChrome(
factory, capabilities, std::move(devtools_event_listeners),
std::move(on_socket_message), w3c_compliant, chrome);
} else {
return LaunchDesktopChrome(factory, socket_factory, capabilities,
std::move(devtools_event_listeners),
w3c_compliant, chrome);
std::move(on_socket_message), w3c_compliant,
chrome);
}
}

Expand Down
1 change: 1 addition & 0 deletions chrome_launcher.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ Status LaunchChrome(network::mojom::URLLoaderFactory* factory,
const Capabilities& capabilities,
std::vector<std::unique_ptr<DevToolsEventListener>>
devtools_event_listeners,
base::RepeatingClosure on_socket_message,
bool w3c_compliant,
std::unique_ptr<Chrome>& chrome);

Expand Down
Loading

0 comments on commit d619531

Please sign in to comment.