[BrowserSwitcher] Launch IE asynchronously

Previously, IE would be launched directly from the UI thread. This was
not only causing jank, but also browser freezes when `DdeConnect()'
would take a long time to complete (>1min in some cases).

This CL makes `DdeConnect()' and `base::LaunchProcess()' run on a worker
thread with blocking allowed, so the browser is still responsive in the
meantime.

Bug: 1054895
Change-Id: I305063c12ebce67a9feffecfeb9f1d4c75211cf4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2082994
Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#747424}

chrome/browser/browser_switcher/alternative_browser_driver.h

@@ -7,6 +7,7 @@
 
 #include <string>
 
+#include "base/callback.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
@@ -39,11 +40,13 @@ enum class BrowserType {
 // - Launching an external process
 class AlternativeBrowserDriver {
  public:
+  using LaunchCallback = base::OnceCallback<void(bool success)>;
+
   virtual ~AlternativeBrowserDriver();
 
   // Tries to launch |browser| at the specified URL, using whatever
   // method is most appropriate.
-  virtual bool TryLaunch(const GURL& url) = 0;
+  virtual void TryLaunch(const GURL& url, LaunchCallback cb) = 0;
 
   // Returns the localized string for the name of the alternative browser, if it
   // was auto-detected. If the name couldn't be auto-detected, returns an empty
@@ -62,8 +65,12 @@ class AlternativeBrowserDriverImpl : public AlternativeBrowserDriver {
   explicit AlternativeBrowserDriverImpl(const BrowserSwitcherPrefs* prefs);
   ~AlternativeBrowserDriverImpl() override;
 
+  AlternativeBrowserDriverImpl(const AlternativeBrowserDriverImpl&) = delete;
+  AlternativeBrowserDriverImpl& operator=(const AlternativeBrowserDriverImpl&) =
+      delete;
+
   // AlternativeBrowserDriver
-  bool TryLaunch(const GURL& url) override;
+  void TryLaunch(const GURL& url, LaunchCallback cb) override;
   std::string GetBrowserName() const override;
   BrowserType GetBrowserType() const override;
 
@@ -74,14 +81,7 @@ class AlternativeBrowserDriverImpl : public AlternativeBrowserDriver {
  private:
   using StringType = base::FilePath::StringType;
 
-#if defined(OS_WIN)
-  bool TryLaunchWithDde(const GURL& url);
-  bool TryLaunchWithExec(const GURL& url);
-#endif
-
   const BrowserSwitcherPrefs* const prefs_;
-
-  DISALLOW_COPY_AND_ASSIGN(AlternativeBrowserDriverImpl);
 };
 
 }  // namespace browser_switcher

chrome/browser/browser_switcher/alternative_browser_driver_posix.cc

@@ -11,9 +11,14 @@
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/task/post_task.h"
+#include "base/task/thread_pool.h"
+#include "base/threading/scoped_blocking_call.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_switcher/browser_switcher_prefs.h"
 #include "chrome/grit/generated_resources.h"
+#include "content/public/browser/browser_task_traits.h"
+#include "content/public/browser/browser_thread.h"
 #include "url/gurl.h"
 
 #include "third_party/re2/src/re2/re2.h"
@@ -22,6 +27,8 @@ namespace browser_switcher {
 
 namespace {
 
+using LaunchCallback = AlternativeBrowserDriver::LaunchCallback;
+
 const char kUrlVarName[] = "${url}";
 
 #if defined(OS_MACOSX)
@@ -147,64 +154,14 @@ void ExpandPresetBrowsers(std::string* str) {
     *str = mapping->executable_name;
 }
 
-}  // namespace
-
-AlternativeBrowserDriver::~AlternativeBrowserDriver() {}
-
-AlternativeBrowserDriverImpl::AlternativeBrowserDriverImpl(
-    const BrowserSwitcherPrefs* prefs)
-    : prefs_(prefs) {}
-
-AlternativeBrowserDriverImpl::~AlternativeBrowserDriverImpl() {}
-
-bool AlternativeBrowserDriverImpl::TryLaunch(const GURL& url) {
-#if !defined(OS_MACOSX)
-  if (prefs_->GetAlternativeBrowserPath().empty()) {
-    LOG(ERROR) << "Alternative browser not configured. "
-               << "Aborting browser switch.";
-    return false;
-  }
-#endif
-
-  VLOG(2) << "Launching alternative browser...";
-  VLOG(2) << "  path = " << prefs_->GetAlternativeBrowserPath();
-  VLOG(2) << "  url = " << url.spec();
-
-  CHECK(url.SchemeIsHTTPOrHTTPS() || url.SchemeIsFile());
-
-  auto cmd_line = CreateCommandLine(url);
-  base::LaunchOptions options;
-  // Don't close the alternative browser when Chrome exits.
-  options.new_process_group = true;
-  if (!base::LaunchProcess(cmd_line, options).IsValid()) {
-    LOG(ERROR) << "Could not start the alternative browser!";
-    return false;
-  }
-  return true;
-}
-
-std::string AlternativeBrowserDriverImpl::GetBrowserName() const {
-  std::string path = prefs_->GetAlternativeBrowserPath();
-  const auto* mapping = FindBrowserMapping(path);
-  return mapping ? mapping->browser_name : std::string();
-}
-
-BrowserType AlternativeBrowserDriverImpl::GetBrowserType() const {
-  std::string path = prefs_->GetAlternativeBrowserPath();
-  const auto* mapping = FindBrowserMapping(path);
-  return mapping ? mapping->browser_type : BrowserType::kUnknown;
-}
-
-base::CommandLine AlternativeBrowserDriverImpl::CreateCommandLine(
-    const GURL& url) {
-  std::string path = prefs_->GetAlternativeBrowserPath();
+base::CommandLine CreateCommandLine(const GURL& url,
+                                    const std::string& original_path,
+                                    const std::vector<std::string>& params) {
+  std::string path = original_path;
   ExpandPresetBrowsers(&path);
   ExpandTilde(&path);
   ExpandEnvironmentVariables(&path);
 
-  const std::vector<std::string>& params =
-      prefs_->GetAlternativeBrowserParameters();
-
 #if defined(OS_MACOSX)
   // On MacOS, if the path doesn't start with a '/', it's probably not an
   // executable path. It is probably a name for an application, e.g. "Safari" or
@@ -240,4 +197,75 @@ base::CommandLine AlternativeBrowserDriverImpl::CreateCommandLine(
   return cmd_line;
 }
 
+void TryLaunchBlocking(GURL url,
+                       std::string path,
+                       std::vector<std::string> params,
+                       LaunchCallback cb) {
+  base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
+                                                base::BlockingType::MAY_BLOCK);
+
+  CHECK(url.SchemeIsHTTPOrHTTPS() || url.SchemeIsFile());
+
+  auto cmd_line = CreateCommandLine(url, path, params);
+  base::LaunchOptions options;
+  // Don't close the alternative browser when Chrome exits.
+  options.new_process_group = true;
+  const bool success = base::LaunchProcess(cmd_line, options).IsValid();
+  if (!success)
+    LOG(ERROR) << "Could not start the alternative browser!";
+}
+
+}  // namespace
+
+AlternativeBrowserDriver::~AlternativeBrowserDriver() = default;
+
+AlternativeBrowserDriverImpl::AlternativeBrowserDriverImpl(
+    const BrowserSwitcherPrefs* prefs)
+    : prefs_(prefs) {}
+
+AlternativeBrowserDriverImpl::~AlternativeBrowserDriverImpl() = default;
+
+void AlternativeBrowserDriverImpl::TryLaunch(const GURL& url,
+                                             LaunchCallback cb) {
+#if !defined(OS_MACOSX)
+  if (prefs_->GetAlternativeBrowserPath().empty()) {
+    LOG(ERROR) << "Alternative browser not configured. "
+               << "Aborting browser switch.";
+    std::move(cb).Run(false);
+    return;
+  }
+#endif
+
+  VLOG(2) << "Launching alternative browser...";
+  VLOG(2) << "  path = " << prefs_->GetAlternativeBrowserPath();
+  VLOG(2) << "  url = " << url.spec();
+
+  base::ThreadPool::PostTask(
+      FROM_HERE,
+      {base::MayBlock(), base::TaskPriority::USER_BLOCKING,
+       base::TaskShutdownBehavior::BLOCK_SHUTDOWN},
+      base::BindOnce(&TryLaunchBlocking, url,
+                     prefs_->GetAlternativeBrowserPath(),
+                     prefs_->GetAlternativeBrowserParameters(), std::move(cb)));
+}
+
+std::string AlternativeBrowserDriverImpl::GetBrowserName() const {
+  std::string path = prefs_->GetAlternativeBrowserPath();
+  const auto* mapping = FindBrowserMapping(path);
+  return mapping ? mapping->browser_name : std::string();
+}
+
+BrowserType AlternativeBrowserDriverImpl::GetBrowserType() const {
+  std::string path = prefs_->GetAlternativeBrowserPath();
+  const auto* mapping = FindBrowserMapping(path);
+  return mapping ? mapping->browser_type : BrowserType::kUnknown;
+}
+
+base::CommandLine AlternativeBrowserDriverImpl::CreateCommandLine(
+    const GURL& url) {
+  return browser_switcher::CreateCommandLine(
+      url, prefs_->GetAlternativeBrowserPath(),
+      prefs_->GetAlternativeBrowserParameters());
+}
+
 }  // namespace browser_switcher

chrome/browser/browser_switcher/alternative_browser_driver_win.cc

@@ -16,15 +16,22 @@
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/task/post_task.h"
+#include "base/task/thread_pool.h"
+#include "base/threading/scoped_blocking_call.h"
 #include "base/win/registry.h"
 #include "chrome/browser/browser_switcher/browser_switcher_prefs.h"
 #include "chrome/grit/generated_resources.h"
+#include "content/public/browser/browser_task_traits.h"
+#include "content/public/browser/browser_thread.h"
 #include "url/gurl.h"
 
 namespace browser_switcher {
 
 namespace {
 
+using LaunchCallback = AlternativeBrowserDriver::LaunchCallback;
+
 const wchar_t kUrlVarName[] = L"${url}";
 
 const wchar_t kIExploreKey[] =
@@ -179,41 +186,16 @@ void AppendCommandLineArguments(base::CommandLine* cmd_line,
 }
 
 bool IsInternetExplorer(base::StringPiece path) {
-  // TODO(nicolaso): Check if the path looks like the default IEXPLORE.exe path.
+  // We don't treat IExplore.exe as Internet Explorer here. This way, admins can
+  // set |AlternativeBrowserPath| to IExplore.exe to disable DDE, if it's
+  // causing issues or slowness.
   return (path.empty() || base::EqualsASCII(kIExploreKey, path));
 }
 
-}  // namespace
-
-AlternativeBrowserDriver::~AlternativeBrowserDriver() {}
-
-AlternativeBrowserDriverImpl::AlternativeBrowserDriverImpl(
-    const BrowserSwitcherPrefs* prefs)
-    : prefs_(prefs) {}
-
-AlternativeBrowserDriverImpl::~AlternativeBrowserDriverImpl() {}
-
-bool AlternativeBrowserDriverImpl::TryLaunch(const GURL& url) {
-  VLOG(2) << "Launching alternative browser...";
-  VLOG(2) << "  path = " << prefs_->GetAlternativeBrowserPath();
-  VLOG(2) << "  url = " << url.spec();
-  return (TryLaunchWithDde(url) || TryLaunchWithExec(url));
-}
-
-std::string AlternativeBrowserDriverImpl::GetBrowserName() const {
-  std::wstring path = base::UTF8ToWide(prefs_->GetAlternativeBrowserPath());
-  const auto* mapping = FindBrowserMapping(path, false);
-  return mapping ? mapping->browser_name : std::string();
-}
-
-BrowserType AlternativeBrowserDriverImpl::GetBrowserType() const {
-  std::wstring path = base::UTF8ToWide(prefs_->GetAlternativeBrowserPath());
-  const auto* mapping = FindBrowserMapping(path, true);
-  return mapping ? mapping->browser_type : BrowserType::kUnknown;
-}
-
-bool AlternativeBrowserDriverImpl::TryLaunchWithDde(const GURL& url) {
-  if (!IsInternetExplorer(prefs_->GetAlternativeBrowserPath()))
+bool TryLaunchWithDde(const GURL& url, const std::string& path) {
+  base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
+                                                base::BlockingType::WILL_BLOCK);
+  if (!IsInternetExplorer(path))
     return false;
 
   DWORD dde_instance = 0;
@@ -270,10 +252,27 @@ bool AlternativeBrowserDriverImpl::TryLaunchWithDde(const GURL& url) {
   return success;
 }
 
-bool AlternativeBrowserDriverImpl::TryLaunchWithExec(const GURL& url) {
+base::CommandLine CreateCommandLine(const GURL& url,
+                                    const std::string& utf8_path,
+                                    const std::vector<std::string>& params) {
+  std::wstring path = base::UTF8ToWide(utf8_path);
+  ExpandPresetBrowsers(&path);
+  ExpandEnvironmentVariables(&path);
+  base::CommandLine cmd_line(std::vector<std::wstring>{path});
+
+  AppendCommandLineArguments(&cmd_line, params, url);
+
+  return cmd_line;
+}
+
+bool TryLaunchWithExec(const GURL& url,
+                       const std::string& path,
+                       const std::vector<std::string>& args) {
+  base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
+                                                base::BlockingType::MAY_BLOCK);
   CHECK(url.SchemeIsHTTPOrHTTPS() || url.SchemeIsFile());
 
-  auto cmd_line = CreateCommandLine(url);
+  auto cmd_line = CreateCommandLine(url, path, args);
 
   base::LaunchOptions options;
   if (!base::LaunchProcess(cmd_line, options).IsValid()) {
@@ -284,17 +283,61 @@ bool AlternativeBrowserDriverImpl::TryLaunchWithExec(const GURL& url) {
   return true;
 }
 
-base::CommandLine AlternativeBrowserDriverImpl::CreateCommandLine(
-    const GURL& url) {
+void TryLaunchBlocking(GURL url,
+                       std::string path,
+                       std::vector<std::string> params,
+                       LaunchCallback cb) {
+  const bool success =
+      (TryLaunchWithDde(url, path) || TryLaunchWithExec(url, path, params));
+  base::PostTask(
+      FROM_HERE, {content::BrowserThread::UI},
+      base::BindOnce(
+          [](bool success, LaunchCallback cb) { std::move(cb).Run(success); },
+          success, std::move(cb)));
+}
+
+}  // namespace
+
+AlternativeBrowserDriver::~AlternativeBrowserDriver() = default;
+
+AlternativeBrowserDriverImpl::AlternativeBrowserDriverImpl(
+    const BrowserSwitcherPrefs* prefs)
+    : prefs_(prefs) {}
+
+AlternativeBrowserDriverImpl::~AlternativeBrowserDriverImpl() = default;
+
+void AlternativeBrowserDriverImpl::TryLaunch(const GURL& url,
+                                             LaunchCallback cb) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  VLOG(2) << "Launching alternative browser...";
+  VLOG(2) << "  path = " << prefs_->GetAlternativeBrowserPath();
+  VLOG(2) << "  url = " << url.spec();
+  base::ThreadPool::PostTask(
+      FROM_HERE,
+      {base::MayBlock(), base::TaskPriority::USER_BLOCKING,
+       base::TaskShutdownBehavior::BLOCK_SHUTDOWN},
+      base::BindOnce(&TryLaunchBlocking, url,
+                     prefs_->GetAlternativeBrowserPath(),
+                     prefs_->GetAlternativeBrowserParameters(), std::move(cb)));
+}
+
+std::string AlternativeBrowserDriverImpl::GetBrowserName() const {
   std::wstring path = base::UTF8ToWide(prefs_->GetAlternativeBrowserPath());
-  ExpandPresetBrowsers(&path);
-  ExpandEnvironmentVariables(&path);
-  base::CommandLine cmd_line(std::vector<std::wstring>{path});
+  const auto* mapping = FindBrowserMapping(path, false);
+  return mapping ? mapping->browser_name : std::string();
+}
 
-  AppendCommandLineArguments(&cmd_line,
-                             prefs_->GetAlternativeBrowserParameters(), url);
+BrowserType AlternativeBrowserDriverImpl::GetBrowserType() const {
+  std::wstring path = base::UTF8ToWide(prefs_->GetAlternativeBrowserPath());
+  const auto* mapping = FindBrowserMapping(path, true);
+  return mapping ? mapping->browser_type : BrowserType::kUnknown;
+}
 
-  return cmd_line;
+base::CommandLine AlternativeBrowserDriverImpl::CreateCommandLine(
+    const GURL& url) {
+  return browser_switcher::CreateCommandLine(
+      url, prefs_->GetAlternativeBrowserPath(),
+      prefs_->GetAlternativeBrowserParameters());
 }
 
 }  // namespace browser_switcher

chrome/browser/browser_switcher/mock_alternative_browser_driver.h

@@ -20,7 +20,7 @@ class MockAlternativeBrowserDriver : public AlternativeBrowserDriver {
 
   MOCK_CONST_METHOD1(ExpandEnvVars, void(std::string*));
   MOCK_CONST_METHOD1(ExpandPresetBrowsers, void(std::string*));
-  MOCK_METHOD1(TryLaunch, bool(const GURL&));
+  MOCK_METHOD2(TryLaunch, void(const GURL&, LaunchCallback cb));
   MOCK_CONST_METHOD0(GetBrowserName, std::string());
 };
 

chrome/browser/ui/webui/browser_switch/browser_switch_ui.cc

@@ -180,6 +180,10 @@ class BrowserSwitchHandler : public content::WebUIMessageHandler {
   // JavaScript promise is not resolved, because we close the tab anyways.
   void HandleLaunchAlternativeBrowserAndCloseTab(const base::ListValue* args);
 
+  void OnLaunchFinished(base::TimeTicks start,
+                        std::string callback_id,
+                        bool success);
+
   // Navigates to the New Tab Page.
   void HandleGotoNewTabPage(const base::ListValue* args);
 
@@ -237,6 +241,8 @@ class BrowserSwitchHandler : public content::WebUIMessageHandler {
       browser_switcher::BrowserSwitcherService::CallbackSubscription>
       service_subscription_;
 
+  base::WeakPtrFactory<BrowserSwitchHandler> weak_ptr_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(BrowserSwitchHandler);
 };
 
@@ -324,24 +330,31 @@ void BrowserSwitchHandler::HandleLaunchAlternativeBrowserAndCloseTab(
     return;
   }
 
-  bool success;
-  {
-    SCOPED_UMA_HISTOGRAM_TIMER("BrowserSwitcher.LaunchTime");
-    success = service->driver()->TryLaunch(url);
-    UMA_HISTOGRAM_BOOLEAN("BrowserSwitcher.LaunchSuccess", success);
-  }
+  service->driver()->TryLaunch(
+      url, base::BindOnce(&BrowserSwitchHandler::OnLaunchFinished,
+                          weak_ptr_factory_.GetWeakPtr(),
+                          base::TimeTicks::Now(), std::move(callback_id)));
+}
+
+void BrowserSwitchHandler::OnLaunchFinished(base::TimeTicks start,
+                                            std::string callback_id,
+                                            bool success) {
+  const base::TimeDelta runtime = base::TimeTicks::Now() - start;
+  UMA_HISTOGRAM_TIMES("BrowserSwitcher.LaunchTime", runtime);
+  UMA_HISTOGRAM_BOOLEAN("BrowserSwitcher.LaunchSuccess", success);
 
   if (!success) {
-    RejectJavascriptCallback(args->GetList()[0], base::Value());
+    RejectJavascriptCallback(base::Value(callback_id), base::Value());
     return;
   }
 
+  auto* service = GetBrowserSwitcherService(web_ui());
   auto* profile = Profile::FromWebUI(web_ui());
-
+  // We don't need to resolve the promise, because the tab will close (or
+  // navigate to about:newtab) anyways.
   if (service->prefs().KeepLastTab() && IsLastTab(profile)) {
     GotoNewTabPage(web_ui()->GetWebContents());
   } else {
-    // We don't need to resolve the promise, because the tab will close anyways.
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,
         base::BindOnce(&content::WebContents::ClosePage,