[DevTools] Keep the browser alive when remote debugging without window

When --no-startup-window is passed in combination with remote debugging,
browser is controlled by automation software. In this case, we keep
the application running until explicit Browser.close call over protocol.
Otherwise, application just exits because no browser window is opened.

Drive-by: apparently, this introduced a first test with DevToolsPipeHandler,
which exposed a DCHECK. Shutting down the pipe handler posts a task to
delete threads which joins them and requires sync primitives task trait.

Bug: 1054831
Change-Id: I1fcfdad11ec10661a2163b18a7547668655f25bf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2068043Reviewed-by: Ben Wells <benwells@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#743967}

chrome/browser/devtools/chrome_devtools_manager_delegate.cc

@@ -23,13 +23,17 @@
 #include "chrome/browser/ui/browser_navigator.h"
 #include "chrome/browser/ui/browser_navigator_params.h"
 #include "chrome/browser/ui/tab_contents/tab_contents_iterator.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/grit/browser_resources.h"
 #include "components/guest_view/browser/guest_view_base.h"
+#include "components/keep_alive_registry/keep_alive_types.h"
+#include "components/keep_alive_registry/scoped_keep_alive.h"
 #include "content/public/browser/devtools_agent_host.h"
 #include "content/public/browser/devtools_agent_host_client_channel.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/content_switches.h"
 #include "extensions/browser/extension_host.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/process_manager.h"
@@ -88,6 +92,18 @@ ChromeDevToolsManagerDelegate* ChromeDevToolsManagerDelegate::GetInstance() {
 ChromeDevToolsManagerDelegate::ChromeDevToolsManagerDelegate() {
   DCHECK(!g_instance);
   g_instance = this;
+
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+  if (command_line->HasSwitch(switches::kNoStartupWindow) &&
+      (command_line->HasSwitch(switches::kRemoteDebuggingPipe) ||
+       command_line->HasSwitch(switches::kRemoteDebuggingPort))) {
+    // If running without a startup window with remote debugging,
+    // we are controlled entirely by the automation process.
+    // Keep the application running until explicit close through DevTools
+    // protocol.
+    keep_alive_.reset(new ScopedKeepAlive(KeepAliveOrigin::REMOTE_DEBUGGING,
+                                          KeepAliveRestartOption::DISABLED));
+  }
 }
 
 ChromeDevToolsManagerDelegate::~ChromeDevToolsManagerDelegate() {
@@ -319,3 +335,9 @@ void ChromeDevToolsManagerDelegate::ResetAndroidDeviceManagerForTesting() {
   // task using a raw pointer to the DeviceManager we just deleted.
   device_discovery_.reset();
 }
+
+void ChromeDevToolsManagerDelegate::BrowserCloseRequested() {
+  // Do not keep the application running anymore, we got an explicit request
+  // to close.
+  keep_alive_.reset();
+}

chrome/browser/devtools/chrome_devtools_manager_delegate.h

@@ -20,6 +20,7 @@
 #include "net/base/host_port_pair.h"
 
 class ChromeDevToolsSession;
+class ScopedKeepAlive;
 using RemoteLocations = std::set<net::HostPortPair>;
 
 namespace extensions {
@@ -53,6 +54,8 @@ class ChromeDevToolsManagerDelegate : public content::DevToolsManagerDelegate {
   std::vector<content::BrowserContext*> GetBrowserContexts() override;
   content::BrowserContext* GetDefaultBrowserContext() override;
 
+  void BrowserCloseRequested();
+
  private:
   friend class DevToolsManagerDelegateTest;
 
@@ -90,6 +93,7 @@ class ChromeDevToolsManagerDelegate : public content::DevToolsManagerDelegate {
   std::unique_ptr<DevToolsDeviceDiscovery> device_discovery_;
   content::DevToolsAgentHost::List remote_agent_hosts_;
   RemoteLocations remote_locations_;
+  std::unique_ptr<ScopedKeepAlive> keep_alive_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromeDevToolsManagerDelegate);
 };

chrome/browser/devtools/devtools_sanity_browsertest.cc

@@ -39,9 +39,11 @@
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/unpacked_installer.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
+#include "chrome/browser/lifetime/browser_shutdown.h"
 #include "chrome/browser/policy/chrome_browser_policy_connector.h"
 #include "chrome/browser/policy/developer_tools_policy_handler.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_list.h"
@@ -61,6 +63,8 @@
 #include "components/autofill/core/common/autofill_features.h"
 #include "components/javascript_dialogs/app_modal_dialog_controller.h"
 #include "components/javascript_dialogs/app_modal_dialog_view.h"
+#include "components/keep_alive_registry/keep_alive_registry.h"
+#include "components/keep_alive_registry/keep_alive_types.h"
 #include "components/policy/core/common/mock_configuration_policy_provider.h"
 #include "components/policy/core/common/policy_map.h"
 #include "components/policy/policy_constants.h"
@@ -2358,6 +2362,8 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessDevToolsSanityTest, InspectElement) {
 }
 
 IN_PROC_BROWSER_TEST_F(InProcessBrowserTest, BrowserCloseWithBeforeUnload) {
+  EXPECT_FALSE(KeepAliveRegistry::GetInstance()->IsOriginRegistered(
+      KeepAliveOrigin::REMOTE_DEBUGGING));
   ui_test_utils::NavigateToURL(browser(), GURL(url::kAboutBlankURL));
   WebContents* tab = browser()->tab_strip_model()->GetActiveWebContents();
   ASSERT_TRUE(content::ExecuteScript(
@@ -2370,6 +2376,30 @@ IN_PROC_BROWSER_TEST_F(InProcessBrowserTest, BrowserCloseWithBeforeUnload) {
   ui_test_utils::WaitForBrowserToClose(browser());
 }
 
+class KeepAliveDevToolsTest : public InProcessBrowserTest {
+ protected:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitchASCII(switches::kRemoteDebuggingPort, "0");
+    command_line->AppendSwitch(switches::kNoStartupWindow);
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(KeepAliveDevToolsTest, KeepsAliveUntilBrowserClose) {
+  EXPECT_FALSE(browser_shutdown::IsTryingToQuit());
+  EXPECT_TRUE(BrowserList::GetInstance()->empty());
+  EXPECT_TRUE(KeepAliveRegistry::GetInstance()->IsKeepingAlive());
+  EXPECT_TRUE(KeepAliveRegistry::GetInstance()->IsOriginRegistered(
+      KeepAliveOrigin::REMOTE_DEBUGGING));
+  chrome::NewEmptyWindow(ProfileManager::GetLastUsedProfile());
+  EXPECT_FALSE(BrowserList::GetInstance()->empty());
+  BrowserHandler handler(nullptr, std::string());
+  handler.Close();
+  ui_test_utils::WaitForBrowserToClose();
+  EXPECT_FALSE(KeepAliveRegistry::GetInstance()->IsKeepingAlive());
+  EXPECT_FALSE(KeepAliveRegistry::GetInstance()->IsOriginRegistered(
+      KeepAliveOrigin::REMOTE_DEBUGGING));
+}
+
 class DevToolsPolicyTest : public InProcessBrowserTest {
  protected:
   DevToolsPolicyTest() {

chrome/browser/devtools/protocol/browser_handler.cc

@@ -112,8 +112,12 @@ Response BrowserHandler::GetWindowBounds(
 }
 
 Response BrowserHandler::Close() {
-  base::PostTask(FROM_HERE, {content::BrowserThread::UI},
-                 base::BindOnce([]() { chrome::ExitIgnoreUnloadHandlers(); }));
+  base::PostTask(
+      FROM_HERE, {content::BrowserThread::UI}, base::BindOnce([]() {
+        if (ChromeDevToolsManagerDelegate::GetInstance())
+          ChromeDevToolsManagerDelegate::GetInstance()->BrowserCloseRequested();
+        chrome::ExitIgnoreUnloadHandlers();
+      }));
   return Response::OK();
 }
 

components/keep_alive_registry/keep_alive_types.cc

@@ -25,6 +25,8 @@ std::ostream& operator<<(std::ostream& out, const KeepAliveOrigin& origin) {
       return out << "LOGIN_DISPLAY_HOST_WEBUI";
     case KeepAliveOrigin::PIN_MIGRATION:
       return out << "PIN_MIGRATION";
+    case KeepAliveOrigin::REMOTE_DEBUGGING:
+      return out << "REMOTE_DEBUGGING";
     case KeepAliveOrigin::NOTIFICATION:
       return out << "NOTIFICATION";
     case KeepAliveOrigin::PENDING_NOTIFICATION_CLICK_EVENT:

components/keep_alive_registry/keep_alive_types.h

@@ -31,6 +31,9 @@ enum class KeepAliveOrigin {
   LOGIN_DISPLAY_HOST_WEBUI,
   PIN_MIGRATION,
 
+  // c/b/devtools
+  REMOTE_DEBUGGING,
+
   // c/b/extensions
   NATIVE_MESSAGING_HOST_ERROR_REPORT,
 

content/browser/devtools/devtools_pipe_handler.cc

@@ -295,7 +295,8 @@ void DevToolsPipeHandler::Shutdown() {
   if (!write_thread_) {
     base::PostTask(
         FROM_HERE,
-        {base::ThreadPool(), base::MayBlock(), base::TaskPriority::BEST_EFFORT},
+        {base::ThreadPool(), base::MayBlock(), base::WithBaseSyncPrimitives(),
+         base::TaskPriority::BEST_EFFORT},
         base::BindOnce([](base::Thread* rthread) { delete rthread; },
                        read_thread_.release()));
     return;
@@ -326,7 +327,8 @@ void DevToolsPipeHandler::Shutdown() {
   // Post background task that would join and destroy the threads.
   base::PostTask(
       FROM_HERE,
-      {base::ThreadPool(), base::MayBlock(), base::TaskPriority::BEST_EFFORT},
+      {base::ThreadPool(), base::MayBlock(), base::WithBaseSyncPrimitives(),
+       base::TaskPriority::BEST_EFFORT},
       base::BindOnce(
           [](base::Thread* rthread, base::Thread* wthread) {
             delete rthread;