Disconnected DevToolsPipeHandler closes the browser

When the browser is controlled remotely over the pipe,
and the pipe is disconnected, there is no way left to
control the browser. Every pipe client tries to kill
the browser at that point. Instead, we can just shutdown
normally, leaving no core dumps.

Change-Id: Ib03a727930b00f48a6869c12c66c86072d45b5b9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2536354
Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827457}

chrome/browser/devtools/chrome_devtools_manager_delegate.cc

@@ -18,6 +18,7 @@
 #include "chrome/browser/devtools/devtools_window.h"
 #include "chrome/browser/devtools/protocol/target_handler.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
+#include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/browser/policy/developer_tools_policy_handler.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
@@ -29,6 +30,7 @@
 #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/browser_task_traits.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"
@@ -336,8 +338,15 @@ void ChromeDevToolsManagerDelegate::ResetAndroidDeviceManagerForTesting() {
   device_discovery_.reset();
 }
 
-void ChromeDevToolsManagerDelegate::BrowserCloseRequested() {
-  // Do not keep the application running anymore, we got an explicit request
-  // to close.
-  keep_alive_.reset();
+// static
+void ChromeDevToolsManagerDelegate::CloseBrowserSoon() {
+  content::GetUIThreadTaskRunner({})->PostTask(
+      FROM_HERE, base::BindOnce([]() {
+        if (GetInstance()) {
+          // Do not keep the application running anymore, we got an explicit
+          // request to close.
+          GetInstance()->keep_alive_.reset();
+        }
+        chrome::ExitIgnoreUnloadHandlers();
+      }));
 }

chrome/browser/devtools/chrome_devtools_manager_delegate.h

@@ -54,7 +54,8 @@ class ChromeDevToolsManagerDelegate : public content::DevToolsManagerDelegate {
   std::vector<content::BrowserContext*> GetBrowserContexts() override;
   content::BrowserContext* GetDefaultBrowserContext() override;
 
-  void BrowserCloseRequested();
+  // Closes browser soon, not in the current task.
+  static void CloseBrowserSoon();
 
  private:
   friend class DevToolsManagerDelegateTest;

chrome/browser/devtools/protocol/browser_handler.cc

@@ -12,7 +12,6 @@
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/devtools/chrome_devtools_manager_delegate.h"
 #include "chrome/browser/devtools/devtools_dock_tile.h"
-#include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_window.h"
@@ -113,12 +112,7 @@ Response BrowserHandler::GetWindowBounds(
 }
 
 Response BrowserHandler::Close() {
-  content::GetUIThreadTaskRunner({})->PostTask(
-      FROM_HERE, base::BindOnce([]() {
-        if (ChromeDevToolsManagerDelegate::GetInstance())
-          ChromeDevToolsManagerDelegate::GetInstance()->BrowserCloseRequested();
-        chrome::ExitIgnoreUnloadHandlers();
-      }));
+  ChromeDevToolsManagerDelegate::CloseBrowserSoon();
   return Response::Success();
 }
 

chrome/browser/devtools/remote_debugging_server.cc

@@ -12,6 +12,7 @@
 #include "base/path_service.h"
 #include "base/strings/string_number_conversions.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/devtools/chrome_devtools_manager_delegate.h"
 #include "chrome/browser/devtools/devtools_window.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
@@ -91,7 +92,8 @@ RemoteDebuggingServer::RemoteDebuggingServer() {
   const base::CommandLine& command_line =
       *base::CommandLine::ForCurrentProcess();
   if (command_line.HasSwitch(switches::kRemoteDebuggingPipe)) {
-    content::DevToolsAgentHost::StartRemoteDebuggingPipeHandler();
+    content::DevToolsAgentHost::StartRemoteDebuggingPipeHandler(
+        base::BindOnce(&ChromeDevToolsManagerDelegate::CloseBrowserSoon));
     return;
   }
 

content/browser/devtools/devtools_manager.cc

@@ -29,9 +29,11 @@ void DevToolsAgentHost::StartRemoteDebuggingServer(
 }
 
 // static
-void DevToolsAgentHost::StartRemoteDebuggingPipeHandler() {
+void DevToolsAgentHost::StartRemoteDebuggingPipeHandler(
+    base::OnceClosure on_disconnect) {
   DevToolsManager* manager = DevToolsManager::GetInstance();
-  manager->SetPipeHandler(std::make_unique<DevToolsPipeHandler>());
+  manager->SetPipeHandler(
+      std::make_unique<DevToolsPipeHandler>(std::move(on_disconnect)));
 }
 
 // static

content/browser/devtools/devtools_pipe_handler.cc

@@ -142,8 +142,12 @@ class PipeReaderBase : public PipeIOBase {
       bool had_error = size_read <= 0;
 #endif
       if (had_error) {
-        if (!shutting_down_.IsSet())
+        if (!shutting_down_.IsSet()) {
           LOG(ERROR) << "Connection terminated while reading from pipe";
+          GetUIThreadTaskRunner({})->PostTask(
+              FROM_HERE, base::BindOnce(&DevToolsPipeHandler::OnDisconnect,
+                                        devtools_handler_));
+        }
         return 0;
       }
       bytes_read += size_read;
@@ -341,8 +345,10 @@ class PipeReaderCBOR : public PipeReaderBase {
 
 // DevToolsPipeHandler ---------------------------------------------------
 
-DevToolsPipeHandler::DevToolsPipeHandler()
-    : read_fd_(kReadFD), write_fd_(kWriteFD) {
+DevToolsPipeHandler::DevToolsPipeHandler(base::OnceClosure on_disconnect)
+    : on_disconnect_(std::move(on_disconnect)),
+      read_fd_(kReadFD),
+      write_fd_(kWriteFD) {
   browser_target_ = DevToolsAgentHost::CreateForBrowser(
       nullptr, DevToolsAgentHost::CreateServerSocketCallback());
   browser_target_->AttachClient(this);
@@ -393,7 +399,10 @@ void DevToolsPipeHandler::HandleMessage(std::vector<uint8_t> message) {
     browser_target_->DispatchProtocolMessage(this, message);
 }
 
-void DevToolsPipeHandler::DetachFromTarget() {}
+void DevToolsPipeHandler::OnDisconnect() {
+  if (on_disconnect_)
+    std::move(on_disconnect_).Run();
+}
 
 void DevToolsPipeHandler::DispatchProtocolMessage(
     DevToolsAgentHost* agent_host,

content/browser/devtools/devtools_pipe_handler.h

@@ -5,6 +5,7 @@
 #ifndef CONTENT_BROWSER_DEVTOOLS_DEVTOOLS_PIPE_HANDLER_H_
 #define CONTENT_BROWSER_DEVTOOLS_DEVTOOLS_PIPE_HANDLER_H_
 
+#include "base/callback.h"
 #include "base/containers/span.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
@@ -17,11 +18,11 @@ class PipeWriterBase;
 
 class DevToolsPipeHandler : public DevToolsAgentHostClient {
  public:
-  DevToolsPipeHandler();
+  explicit DevToolsPipeHandler(base::OnceClosure on_disconnect);
   ~DevToolsPipeHandler() override;
 
   void HandleMessage(std::vector<uint8_t> message);
-  void DetachFromTarget();
+  void OnDisconnect();
 
   // DevToolsAgentHostClient overrides
   void DispatchProtocolMessage(DevToolsAgentHost* agent_host,
@@ -40,6 +41,7 @@ class DevToolsPipeHandler : public DevToolsAgentHostClient {
   };
 
   ProtocolMode mode_;
+  base::OnceClosure on_disconnect_;
 
   std::unique_ptr<PipeReaderBase> pipe_reader_;
   std::unique_ptr<PipeWriterBase> pipe_writer_;

content/public/browser/devtools_agent_host.h

@@ -113,7 +113,7 @@ class CONTENT_EXPORT DevToolsAgentHost
 
   // Starts remote debugging for browser target for the given fd=3
   // for reading and fd=4 for writing remote debugging messages.
-  static void StartRemoteDebuggingPipeHandler();
+  static void StartRemoteDebuggingPipeHandler(base::OnceClosure on_disconnect);
   static void StopRemoteDebuggingPipeHandler();
 
   // Observer is notified about changes in DevToolsAgentHosts.

content/shell/browser/shell_devtools_manager_delegate.cc

@@ -165,7 +165,7 @@ void ShellDevToolsManagerDelegate::StartHttpHandler(
   const base::CommandLine& command_line =
       *base::CommandLine::ForCurrentProcess();
   if (command_line.HasSwitch(switches::kRemoteDebuggingPipe))
-    DevToolsAgentHost::StartRemoteDebuggingPipeHandler();
+    DevToolsAgentHost::StartRemoteDebuggingPipeHandler(base::OnceClosure());
 }
 
 // static

headless/lib/browser/headless_browser_main_parts.cc

@@ -20,7 +20,7 @@ HeadlessBrowserMainParts::~HeadlessBrowserMainParts() = default;
 
 void HeadlessBrowserMainParts::PreMainMessageLoopRun() {
   if (browser_->options()->DevtoolsServerEnabled()) {
-    StartLocalDevToolsHttpHandler(browser_->options());
+    StartLocalDevToolsHttpHandler(browser_);
     devtools_http_handler_started_ = true;
   }
   browser_->PlatformInitialize();

headless/lib/browser/headless_devtools.cc

@@ -9,6 +9,8 @@
 
 #include "base/files/file_path.h"
 #include "base/memory/ptr_util.h"
+#include "content/public/browser/browser_task_traits.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/devtools_agent_host.h"
 #include "content/public/browser/devtools_socket_factory.h"
 #include "content/public/browser/navigation_entry.h"
@@ -124,11 +126,20 @@ class DummyTCPServerSocketFactory : public content::DevToolsSocketFactory {
   DISALLOW_COPY_AND_ASSIGN(DummyTCPServerSocketFactory);
 };
 #endif  // defined(OS_POSIX)
+
+void PostTaskToCloseBrowser(base::WeakPtr<HeadlessBrowserImpl> browser) {
+  content::GetUIThreadTaskRunner({})->PostTask(
+      FROM_HERE, base::BindOnce(&HeadlessBrowserImpl::Shutdown, browser));
+}
+
 }  // namespace
 
-void StartLocalDevToolsHttpHandler(HeadlessBrowser::Options* options) {
-  if (options->devtools_pipe_enabled)
-    content::DevToolsAgentHost::StartRemoteDebuggingPipeHandler();
+void StartLocalDevToolsHttpHandler(HeadlessBrowserImpl* browser) {
+  HeadlessBrowser::Options* options = browser->options();
+  if (options->devtools_pipe_enabled) {
+    content::DevToolsAgentHost::StartRemoteDebuggingPipeHandler(
+        base::BindOnce(&PostTaskToCloseBrowser, browser->GetWeakPtr()));
+  }
   if (options->devtools_endpoint.IsEmpty())
     return;
 

headless/lib/browser/headless_devtools.h

@@ -7,13 +7,13 @@
 
 #include <memory>
 
-#include "headless/public/headless_browser.h"
+#include "headless/lib/browser/headless_browser_impl.h"
 
 namespace headless {
 
 // Starts a DevTools HTTP handler on the loopback interface on the port
 // configured by HeadlessBrowser::Options.
-void StartLocalDevToolsHttpHandler(HeadlessBrowser::Options* options);
+void StartLocalDevToolsHttpHandler(HeadlessBrowserImpl* browser);
 void StopLocalDevToolsHttpHandler();
 
 }  // namespace headless