diff --git a/chrome/browser/BUILD.gn b/chrome/browser/BUILD.gn index 7a38cfa6709d83..4883fe0665bb86 100644 --- a/chrome/browser/BUILD.gn +++ b/chrome/browser/BUILD.gn @@ -3438,10 +3438,9 @@ jumbo_split_static_library("browser") { ] } - if (is_win || (is_linux && !is_chromeos)) { + if (is_win || is_mac || is_desktop_linux) { sources += [ "browser_switcher/alternative_browser_driver.h", - "browser_switcher/alternative_browser_driver_linux.cc", "browser_switcher/alternative_browser_driver_win.cc", "browser_switcher/alternative_browser_launcher.cc", "browser_switcher/alternative_browser_launcher.h", @@ -3458,6 +3457,9 @@ jumbo_split_static_library("browser") { "browser_switcher/ieem_sitelist_parser.cc", "browser_switcher/ieem_sitelist_parser.h", ] + if (is_mac || is_desktop_linux) { + sources += [ "browser_switcher/alternative_browser_driver_posix.cc" ] + } } if (is_chromeos && use_cras) { diff --git a/chrome/browser/browser_switcher/alternative_browser_driver.h b/chrome/browser/browser_switcher/alternative_browser_driver.h index c6de679711bfa5..7662f9735be154 100644 --- a/chrome/browser/browser_switcher/alternative_browser_driver.h +++ b/chrome/browser/browser_switcher/alternative_browser_driver.h @@ -5,6 +5,7 @@ #ifndef CHROME_BROWSER_BROWSER_SWITCHER_ALTERNATIVE_BROWSER_DRIVER_H_ #define CHROME_BROWSER_BROWSER_SWITCHER_ALTERNATIVE_BROWSER_DRIVER_H_ +#include "base/command_line.h" #include "base/files/file_path.h" #include "base/macros.h" #include "base/strings/string_piece_forward.h" @@ -51,17 +52,13 @@ class AlternativeBrowserDriverImpl : public AlternativeBrowserDriver { void SetBrowserParameters(const base::ListValue* parameters) override; bool TryLaunch(const GURL& url) override; - using StringType = base::FilePath::StringType; - - // Appends the arguments in |raw_args| to |argv|, performing substitution of - // "${url}" at the same time. If none of |raw_args| contains "${url}", the URL - // is appended as the last argument. - static void AppendCommandLineArguments( - std::vector* argv, - const std::vector& raw_args, - const GURL& url); + // Create the CommandLine object that would be used to launch an external + // process. + base::CommandLine CreateCommandLine(const GURL& url); private: + using StringType = base::FilePath::StringType; + #if defined(OS_WIN) bool TryLaunchWithDde(const GURL& url); bool TryLaunchWithExec(const GURL& url); diff --git a/chrome/browser/browser_switcher/alternative_browser_driver_linux.cc b/chrome/browser/browser_switcher/alternative_browser_driver_posix.cc similarity index 61% rename from chrome/browser/browser_switcher/alternative_browser_driver_linux.cc rename to chrome/browser/browser_switcher/alternative_browser_driver_posix.cc index 5e6dded976b3b7..53531ece28b5e0 100644 --- a/chrome/browser/browser_switcher/alternative_browser_driver_linux.cc +++ b/chrome/browser/browser_switcher/alternative_browser_driver_posix.cc @@ -4,11 +4,11 @@ #include "chrome/browser/browser_switcher/alternative_browser_driver.h" -#include "base/command_line.h" #include "base/files/file_path.h" #include "base/process/launch.h" #include "base/strings/string_util.h" #include "base/strings/utf_string_conversions.h" +#include "build/build_config.h" #include "chrome/browser/browser_switcher/alternative_browser_launcher.h" #include "url/gurl.h" @@ -22,13 +22,23 @@ namespace { const char kUrlVarName[] = "${url}"; +#if defined(OS_MACOSX) +const char kChromeExecutableName[] = "Google Chrome"; +const char kFirefoxExecutableName[] = "Firefox"; +const char kOperaExecutableName[] = "Opera"; +const char kSafariExecutableName[] = "Safari"; +#else const char kChromeExecutableName[] = "google-chrome"; const char kFirefoxExecutableName[] = "firefox"; const char kOperaExecutableName[] = "opera"; +#endif const char kChromeVarName[] = "${chrome}"; const char kFirefoxVarName[] = "${firefox}"; const char kOperaVarName[] = "${opera}"; +#if defined(OS_MACOSX) +const char kSafariVarName[] = "${safari}"; +#endif const struct { const char* var_name; @@ -37,6 +47,9 @@ const struct { {kChromeVarName, kChromeExecutableName}, {kFirefoxVarName, kFirefoxExecutableName}, {kOperaVarName, kOperaExecutableName}, +#if defined(OS_MACOSX) + {kSafariVarName, kSafariExecutableName}, +#endif }; bool ExpandUrlVarName(std::string* arg, const GURL& url) { @@ -81,6 +94,33 @@ void ExpandEnvironmentVariables(std::string* arg) { std::swap(out, *arg); } +#if defined(OS_MACOSX) +bool ContainsUrlVarName(const std::vector& tokens) { + return std::any_of(tokens.begin(), tokens.end(), + [](const std::string& token) { + return token.find(kUrlVarName) != std::string::npos; + }); +} +#endif // defined(OS_MACOSX) + +// static +void AppendCommandLineArguments(base::CommandLine* cmd_line, + const std::vector& raw_args, + const GURL& url, + bool always_append_url) { + bool contains_url = false; + for (const auto& arg : raw_args) { + std::string expanded_arg = arg; + ExpandTilde(&expanded_arg); + ExpandEnvironmentVariables(&expanded_arg); + if (ExpandUrlVarName(&expanded_arg, url)) + contains_url = true; + cmd_line->AppendArg(expanded_arg); + } + if (always_append_url && !contains_url) + cmd_line->AppendArg(url.spec()); +} + } // namespace AlternativeBrowserDriver::~AlternativeBrowserDriver() {} @@ -90,6 +130,13 @@ AlternativeBrowserDriverImpl::~AlternativeBrowserDriverImpl() {} void AlternativeBrowserDriverImpl::SetBrowserPath(base::StringPiece path) { browser_path_ = path.as_string(); +#if defined(OS_MACOSX) + // Unlike most POSIX platforms, MacOS always has another browser than Chrome, + // so admins don't have to explicitly configure one. + if (browser_path_.empty()) { + browser_path_ = kSafariExecutableName; + } +#endif for (const auto& mapping : kBrowserVarMappings) { if (!browser_path_.compare(mapping.var_name)) { browser_path_ = mapping.executable_name; @@ -120,16 +167,7 @@ bool AlternativeBrowserDriverImpl::TryLaunch(const GURL& url) { CHECK(url.SchemeIsHTTPOrHTTPS() || url.SchemeIsFile()); - const int max_num_args = browser_params_.size() + 2; - std::vector argv; - argv.reserve(max_num_args); - std::string path = browser_path_; - ExpandTilde(&path); - ExpandEnvironmentVariables(&path); - argv.push_back(path); - AppendCommandLineArguments(&argv, browser_params_, url); - - base::CommandLine cmd_line = base::CommandLine(argv); + auto cmd_line = CreateCommandLine(url); base::LaunchOptions options; // Don't close the alternative browser when Chrome exits. options.new_process_group = true; @@ -140,23 +178,45 @@ bool AlternativeBrowserDriverImpl::TryLaunch(const GURL& url) { return true; } -// static -void AlternativeBrowserDriverImpl::AppendCommandLineArguments( - std::vector* argv, - const std::vector& raw_args, +base::CommandLine AlternativeBrowserDriverImpl::CreateCommandLine( const GURL& url) { - // TODO(crbug/882520): Do environment variable and tilde expansion. - bool contains_url = false; - for (const auto& arg : raw_args) { - std::string expanded_arg = arg; - ExpandTilde(&expanded_arg); - ExpandEnvironmentVariables(&expanded_arg); - if (ExpandUrlVarName(&expanded_arg, url)) - contains_url = true; - argv->push_back(expanded_arg); + std::string path = browser_path_; + ExpandTilde(&path); + ExpandEnvironmentVariables(&path); + +#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 + // "Google Chrome". Those can be launched using the `open(1)' command. + // + // It may use the following syntax (first syntax): + // open -a [--args ] + // + // Or, if |browser_params| contains "${url}" (second syntax): + // open -a --args + // + // Safari only supports the first syntax. + if (!browser_path_.empty() && browser_path_[0] != '/') { + base::CommandLine cmd_line(std::vector{"open"}); + cmd_line.AppendArg("-a"); + cmd_line.AppendArg(path); + if (!ContainsUrlVarName(browser_params_)) { + // First syntax. + cmd_line.AppendArg(url.spec()); + } + if (!browser_params_.empty()) { + // First or second syntax, depending on what is in |browser_params_|. + cmd_line.AppendArg("--args"); + AppendCommandLineArguments(&cmd_line, browser_params_, url, + /* always_append_url */ false); + } + return cmd_line; } - if (!contains_url) - argv->push_back(url.spec()); +#endif + base::CommandLine cmd_line(std::vector{path}); + AppendCommandLineArguments(&cmd_line, browser_params_, url, + /* always_append_url */ true); + return cmd_line; } } // namespace browser_switcher diff --git a/chrome/browser/browser_switcher/alternative_browser_driver_unittest.cc b/chrome/browser/browser_switcher/alternative_browser_driver_unittest.cc index 1a28507458da6a..640473cad4d115 100644 --- a/chrome/browser/browser_switcher/alternative_browser_driver_unittest.cc +++ b/chrome/browser/browser_switcher/alternative_browser_driver_unittest.cc @@ -20,17 +20,19 @@ namespace { StringType UTF8ToNative(base::StringPiece src) { #if defined(OS_WIN) return base::UTF8ToWide(src); -#elif defined(OS_LINUX) +#elif defined(OS_POSIX) return src.as_string(); +#else +#error "Invalid platform for browser_switcher" #endif } -std::vector UTF8VectorToNative( +base::ListValue UTF8VectorToListValue( const std::vector& src) { - std::vector out; - out.reserve(src.size()); - std::transform(src.begin(), src.end(), std::back_inserter(out), - &UTF8ToNative); + base::ListValue out; + out.GetList().reserve(src.size()); + for (base::StringPiece str : src) + out.GetList().push_back(base::Value(str)); return out; } @@ -38,108 +40,199 @@ std::vector UTF8VectorToNative( class AlternativeBrowserDriverTest : public testing::Test {}; -TEST_F(AlternativeBrowserDriverTest, AppendCommandLineArguments) { - std::vector argv = {UTF8ToNative("wget")}; - AlternativeBrowserDriverImpl::AppendCommandLineArguments( - &argv, UTF8VectorToNative({"-O", "-", "--"}), - GURL("https://example.com/")); +TEST_F(AlternativeBrowserDriverTest, CreateCommandLine) { + AlternativeBrowserDriverImpl driver; + driver.SetBrowserPath("/usr/bin/true"); + auto params = UTF8VectorToListValue({"a", "b", "c"}); + driver.SetBrowserParameters(¶ms); + auto cmd_line = driver.CreateCommandLine(GURL("http://example.com/")); + const base::CommandLine::StringVector& argv = cmd_line.argv(); EXPECT_EQ(5u, argv.size()); - EXPECT_EQ(UTF8ToNative("wget"), argv[0]); - EXPECT_EQ(UTF8ToNative("-O"), argv[1]); - EXPECT_EQ(UTF8ToNative("-"), argv[2]); - EXPECT_EQ(UTF8ToNative("--"), argv[3]); - EXPECT_EQ(UTF8ToNative("https://example.com/"), argv[4]); + EXPECT_EQ(UTF8ToNative("/usr/bin/true"), argv[0]); + EXPECT_EQ(UTF8ToNative("a"), argv[1]); + EXPECT_EQ(UTF8ToNative("b"), argv[2]); + EXPECT_EQ(UTF8ToNative("c"), argv[3]); + EXPECT_EQ(UTF8ToNative("http://example.com/"), argv[4]); } -TEST_F(AlternativeBrowserDriverTest, AppendCommandLineArgumentsExpandsUrl) { - std::vector argv = {UTF8ToNative("google-chrome")}; - AlternativeBrowserDriverImpl::AppendCommandLineArguments( - &argv, UTF8VectorToNative({"--app=${url}#fragment"}), - GURL("https://example.com/")); +TEST_F(AlternativeBrowserDriverTest, CreateCommandLineExpandsUrl) { + AlternativeBrowserDriverImpl driver; + driver.SetBrowserPath("/usr/bin/true"); + auto params = UTF8VectorToListValue({"--flag=${url}#fragment"}); + driver.SetBrowserParameters(¶ms); + auto cmd_line = driver.CreateCommandLine(GURL("http://example.com/")); + const base::CommandLine::StringVector& argv = cmd_line.argv(); EXPECT_EQ(2u, argv.size()); - EXPECT_EQ(UTF8ToNative("google-chrome"), argv[0]); - EXPECT_EQ(UTF8ToNative("--app=https://example.com/#fragment"), argv[1]); + EXPECT_EQ(UTF8ToNative("/usr/bin/true"), argv[0]); + EXPECT_EQ(UTF8ToNative("--flag=http://example.com/#fragment"), argv[1]); } #if defined(OS_WIN) -TEST_F(AlternativeBrowserDriverTest, AppendCommandLineArgumentsExpandsEnvVars) { - std::vector argv = {L"something.exe"}; +TEST_F(AlternativeBrowserDriverTest, CreateCommandLineExpandsEnvVars) { _putenv("A=AAA"); _putenv("B=BBB"); _putenv("CC=CCC"); _putenv("D=DDD"); - AlternativeBrowserDriverImpl::AppendCommandLineArguments( - &argv, - {L"%A%", L"%B%", L"before_%CC%_between_%D%_after", L"%NONEXISTENT%"}, - GURL("https://example.com/")); + AlternativeBrowserDriverImpl driver; + driver.SetBrowserPath("something.exe"); + auto params = UTF8VectorToListValue( + {"%A%", "%B%", "before_%CC%_between_%D%_after", "%NONEXISTENT%"}); + driver.SetBrowserParameters(¶ms); + auto cmd_line = driver.CreateCommandLine(GURL("http://example.com/")); + const base::CommandLine::StringVector& argv = cmd_line.argv(); EXPECT_EQ(6u, argv.size()); EXPECT_EQ(L"something.exe", argv[0]); EXPECT_EQ(L"AAA", argv[1]); EXPECT_EQ(L"BBB", argv[2]); EXPECT_EQ(L"before_CCC_between_DDD_after", argv[3]); EXPECT_EQ(L"%NONEXISTENT%", argv[4]); - EXPECT_EQ(L"https://example.com/", argv[5]); + EXPECT_EQ(L"http://example.com/", argv[5]); } TEST_F(AlternativeBrowserDriverTest, AppendCommandLineArgumentDoesntExpandUrlContent) { - std::vector argv = {L"something.exe"}; _putenv("A=AAA"); - AlternativeBrowserDriverImpl::AppendCommandLineArguments( - &argv, {}, GURL("https://evil.com/%A%")); - AlternativeBrowserDriverImpl::AppendCommandLineArguments( - &argv, {L"${url}"}, GURL("https://evil.com/%A%")); - EXPECT_EQ(3u, argv.size()); - EXPECT_EQ(L"something.exe", argv[0]); - EXPECT_EQ(L"https://evil.com/%A%", argv[1]); - EXPECT_EQ(L"https://evil.com/%A%", argv[2]); + AlternativeBrowserDriverImpl driver; + driver.SetBrowserPath("something.exe"); + auto cmd_line = driver.CreateCommandLine(GURL("http://evil.com/%A%")); + EXPECT_EQ(2u, cmd_line.argv().size()); + EXPECT_EQ(L"something.exe", cmd_line.argv()[0]); + EXPECT_EQ(L"http://evil.com/%A%", cmd_line.argv()[1]); + + auto params = UTF8VectorToListValue({"${url}"}); + driver.SetBrowserParameters(¶ms); + cmd_line = driver.CreateCommandLine(GURL("http://evil.com/%A%")); + EXPECT_EQ(2u, cmd_line.argv().size()); + EXPECT_EQ(L"something.exe", cmd_line.argv()[0]); + EXPECT_EQ(L"http://evil.com/%A%", cmd_line.argv()[1]); } #endif // defined(OS_WIN) +#if defined(OS_MACOSX) +TEST_F(AlternativeBrowserDriverTest, CreateCommandLineUsesOpen) { + // Use `open(1)' to launch browser paths that aren't absolute. + AlternativeBrowserDriverImpl driver; + + // Default to launching Safari. + driver.SetBrowserPath(""); + auto cmd_line = driver.CreateCommandLine(GURL("http://example.com/")); + EXPECT_EQ(4u, cmd_line.argv().size()); + EXPECT_EQ("open", cmd_line.argv()[0]); + EXPECT_EQ("-a", cmd_line.argv()[1]); + EXPECT_EQ("Safari", cmd_line.argv()[2]); + EXPECT_EQ("http://example.com/", cmd_line.argv()[3]); + + // Expand some "${...}" variables. + driver.SetBrowserPath("${safari}"); + cmd_line = driver.CreateCommandLine(GURL("http://example.com/")); + EXPECT_EQ(4u, cmd_line.argv().size()); + EXPECT_EQ("open", cmd_line.argv()[0]); + EXPECT_EQ("-a", cmd_line.argv()[1]); + EXPECT_EQ("Safari", cmd_line.argv()[2]); + EXPECT_EQ("http://example.com/", cmd_line.argv()[3]); + + // Path looks like an application name => use `open'. + driver.SetBrowserPath("Safari"); + cmd_line = driver.CreateCommandLine(GURL("http://example.com/")); + EXPECT_EQ(4u, cmd_line.argv().size()); + EXPECT_EQ("open", cmd_line.argv()[0]); + EXPECT_EQ("-a", cmd_line.argv()[1]); + EXPECT_EQ("Safari", cmd_line.argv()[2]); + EXPECT_EQ("http://example.com/", cmd_line.argv()[3]); +} + +TEST_F(AlternativeBrowserDriverTest, CreateCommandLineContainsUrl) { + // Use `open(1)' to launch browser paths that aren't absolute. + AlternativeBrowserDriverImpl driver; + + // Args come after `--args', e.g.: + // open -a Safari http://example.com/ --args abc def + driver.SetBrowserPath(""); + auto params = UTF8VectorToListValue({"abc", "def"}); + driver.SetBrowserParameters(¶ms); + auto cmd_line = driver.CreateCommandLine(GURL("http://example.com/")); + EXPECT_EQ(7u, cmd_line.argv().size()); + EXPECT_EQ("open", cmd_line.argv()[0]); + EXPECT_EQ("-a", cmd_line.argv()[1]); + EXPECT_EQ("Safari", cmd_line.argv()[2]); + EXPECT_EQ("http://example.com/", cmd_line.argv()[3]); + EXPECT_EQ("--args", cmd_line.argv()[4]); + EXPECT_EQ("abc", cmd_line.argv()[5]); + EXPECT_EQ("def", cmd_line.argv()[6]); + + // If args contain "${url}", only put the URL together with the other + // args. e.g.: + // open -a Safari --args abc http://example.com/ def + params = UTF8VectorToListValue({"abc", "${url}", "def"}); + driver.SetBrowserParameters(¶ms); + cmd_line = driver.CreateCommandLine(GURL("http://example.com/")); + EXPECT_EQ(7u, cmd_line.argv().size()); + EXPECT_EQ("open", cmd_line.argv()[0]); + EXPECT_EQ("-a", cmd_line.argv()[1]); + EXPECT_EQ("Safari", cmd_line.argv()[2]); + EXPECT_EQ("--args", cmd_line.argv()[3]); + EXPECT_EQ("abc", cmd_line.argv()[4]); + EXPECT_EQ("http://example.com/", cmd_line.argv()[5]); + EXPECT_EQ("def", cmd_line.argv()[6]); +} +#endif // defined(OS_MACOSX) + #if defined(OS_POSIX) -TEST_F(AlternativeBrowserDriverTest, AppendCommandLineArgumentsExpandsTilde) { - std::vector argv = {"some_script"}; +TEST_F(AlternativeBrowserDriverTest, CreateCommandLineExpandsTilde) { setenv("HOME", "/home/foobar", true); - AlternativeBrowserDriverImpl::AppendCommandLineArguments( - &argv, {"~/file.txt", "/tmp/backup.txt~"}, GURL("https://example.com/")); + + AlternativeBrowserDriverImpl driver; + driver.SetBrowserPath("/usr/bin/true"); + auto params = UTF8VectorToListValue({"~/file.txt", "/tmp/backup.txt~"}); + driver.SetBrowserParameters(¶ms); + auto cmd_line = driver.CreateCommandLine(GURL("http://example.com/")); + const base::CommandLine::StringVector& argv = cmd_line.argv(); EXPECT_EQ(4u, argv.size()); - EXPECT_EQ("some_script", argv[0]); - EXPECT_EQ("/home/foobar/file.txt", argv[1]); - EXPECT_EQ("/tmp/backup.txt~", argv[2]); - EXPECT_EQ("https://example.com/", argv[3]); + EXPECT_EQ(UTF8ToNative("/usr/bin/true"), argv[0]); + EXPECT_EQ(UTF8ToNative("/home/foobar/file.txt"), argv[1]); + EXPECT_EQ(UTF8ToNative("/tmp/backup.txt~"), argv[2]); + EXPECT_EQ(UTF8ToNative("http://example.com/"), argv[3]); } -TEST_F(AlternativeBrowserDriverTest, AppendCommandLineArgumentsExpandsEnvVars) { - std::vector argv = {"some_script"}; +TEST_F(AlternativeBrowserDriverTest, CreateCommandLineExpandsEnvVars) { setenv("A", "AAA", true); setenv("B", "BBB", true); setenv("CC", "CCC", true); setenv("D", "DDD", true); - AlternativeBrowserDriverImpl::AppendCommandLineArguments( - &argv, {"$A", "${B}", "before_${CC}_between_${D}_after", "$NONEXISTENT"}, - GURL("https://example.com/")); + + AlternativeBrowserDriverImpl driver; + driver.SetBrowserPath("/usr/bin/true"); + auto params = UTF8VectorToListValue( + {"$A", "${B}", "before_${CC}_between_${D}_after", "$NONEXISTENT"}); + driver.SetBrowserParameters(¶ms); + auto cmd_line = driver.CreateCommandLine(GURL("http://example.com/")); + const base::CommandLine::StringVector& argv = cmd_line.argv(); EXPECT_EQ(6u, argv.size()); - EXPECT_EQ("some_script", argv[0]); - EXPECT_EQ("AAA", argv[1]); - EXPECT_EQ("BBB", argv[2]); + EXPECT_EQ(UTF8ToNative("/usr/bin/true"), argv[0]); + EXPECT_EQ(UTF8ToNative("AAA"), argv[1]); + EXPECT_EQ(UTF8ToNative("BBB"), argv[2]); EXPECT_EQ("before_CCC_between_DDD_after", argv[3]); EXPECT_EQ("", argv[4]); - EXPECT_EQ("https://example.com/", argv[5]); + EXPECT_EQ(UTF8ToNative("http://example.com/"), argv[5]); } -TEST_F(AlternativeBrowserDriverTest, - AppendCommandLineArgumentDoesntExpandUrlContent) { - std::vector argv = {"something"}; +TEST_F(AlternativeBrowserDriverTest, CreateCommandLineDoesntExpandUrlContent) { setenv("A", "AAA", true); setenv("B", "BBB", true); - AlternativeBrowserDriverImpl::AppendCommandLineArguments( - &argv, {}, GURL("https://evil.com/$A${B}")); - AlternativeBrowserDriverImpl::AppendCommandLineArguments( - &argv, {"${url}"}, GURL("https://evil.com/$A${B}")); - EXPECT_EQ(3u, argv.size()); - EXPECT_EQ("something", argv[0]); - EXPECT_EQ("https://evil.com/$A$%7BB%7D", argv[1]); - EXPECT_EQ("https://evil.com/$A$%7BB%7D", argv[2]); + + AlternativeBrowserDriverImpl driver; + driver.SetBrowserPath("/usr/bin/true"); + auto cmd_line = driver.CreateCommandLine(GURL("http://evil.com/$A${B}")); + EXPECT_EQ(2u, cmd_line.argv().size()); + EXPECT_EQ(UTF8ToNative("/usr/bin/true"), cmd_line.argv()[0]); + EXPECT_EQ("http://evil.com/$A$%7BB%7D", cmd_line.argv()[1]); + + auto params = UTF8VectorToListValue({"${url}"}); + driver.SetBrowserParameters(¶ms); + cmd_line = driver.CreateCommandLine(GURL("http://evil.com/$A${B}")); + EXPECT_EQ(2u, cmd_line.argv().size()); + EXPECT_EQ(UTF8ToNative("/usr/bin/true"), cmd_line.argv()[0]); + EXPECT_EQ("http://evil.com/$A$%7BB%7D", cmd_line.argv()[1]); } #endif // defined(OS_POSIX) diff --git a/chrome/browser/browser_switcher/alternative_browser_driver_win.cc b/chrome/browser/browser_switcher/alternative_browser_driver_win.cc index 2f82dc827c9ba7..707385f36a3359 100644 --- a/chrome/browser/browser_switcher/alternative_browser_driver_win.cc +++ b/chrome/browser/browser_switcher/alternative_browser_driver_win.cc @@ -4,7 +4,6 @@ #include "chrome/browser/browser_switcher/alternative_browser_driver.h" -#include "base/command_line.h" #include "base/files/file_path.h" #include "base/process/launch.h" #include "base/strings/utf_string_conversions.h" @@ -113,6 +112,23 @@ void ExpandEnvironmentVariables(std::wstring* arg) { *arg = out.get(); } +void AppendCommandLineArguments(base::CommandLine* cmd_line, + const std::vector& raw_args, + const GURL& url) { + std::wstring url_spec = base::UTF8ToWide(url.spec()); + std::vector command_line; + bool contains_url = false; + for (const auto& arg : raw_args) { + std::wstring expanded_arg = arg; + ExpandEnvironmentVariables(&expanded_arg); + if (ExpandUrlVarName(&expanded_arg, url_spec)) + contains_url = true; + cmd_line->AppendArgNative(expanded_arg); + } + if (!contains_url) + cmd_line->AppendArgNative(url_spec); +} + } // namespace AlternativeBrowserDriver::~AlternativeBrowserDriver() {} @@ -217,17 +233,8 @@ bool AlternativeBrowserDriverImpl::TryLaunchWithExec(const GURL& url) { CHECK(url.SchemeIsHTTPOrHTTPS() || url.SchemeIsFile()); - // We know that there will be at most browser_params_.size() arguments, plus - // one for the executable, and possibly one for the URL. - const int max_num_args = browser_params_.size() + 2; - std::vector argv; - argv.reserve(max_num_args); - std::wstring path = browser_path_; - ExpandEnvironmentVariables(&path); - argv.push_back(path); - AppendCommandLineArguments(&argv, browser_params_, url); + auto cmd_line = CreateCommandLine(url); - base::CommandLine cmd_line = base::CommandLine(argv); base::LaunchOptions options; if (!base::LaunchProcess(cmd_line, options).IsValid()) { LOG(ERROR) << "Could not start the alternative browser! Error: " @@ -237,23 +244,13 @@ bool AlternativeBrowserDriverImpl::TryLaunchWithExec(const GURL& url) { return true; } -void AlternativeBrowserDriverImpl::AppendCommandLineArguments( - std::vector* argv, - const std::vector& raw_args, +base::CommandLine AlternativeBrowserDriverImpl::CreateCommandLine( const GURL& url) { - // TODO(crbug/882520): Do environment variable expansion. - std::wstring url_spec = base::UTF8ToWide(url.spec()); - std::vector command_line; - bool contains_url = false; - for (const auto& arg : raw_args) { - std::wstring expanded_arg = arg; - ExpandEnvironmentVariables(&expanded_arg); - if (ExpandUrlVarName(&expanded_arg, url_spec)) - contains_url = true; - argv->push_back(expanded_arg); - } - if (!contains_url) - argv->push_back(url_spec); + std::wstring path = browser_path_; + ExpandEnvironmentVariables(&path); + base::CommandLine cmd_line(std::vector{path}); + AppendCommandLineArguments(&cmd_line, browser_params_, url); + return cmd_line; } } // namespace browser_switcher diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc index 6cd351fbe6a770..0e67cdbc8ecedb 100644 --- a/chrome/browser/chrome_content_browser_client.cc +++ b/chrome/browser/chrome_content_browser_client.cc @@ -412,11 +412,8 @@ #include "chrome/browser/webshare/share_service_impl.h" #endif -#if defined(OS_WIN) || (defined(OS_LINUX) && !defined(OS_CHROMEOS)) -#include "chrome/browser/browser_switcher/browser_switcher_navigation_throttle.h" -#endif - -#if defined(OS_WIN) || (defined(OS_LINUX) && !defined(OS_CHROMEOS)) +#if defined(OS_WIN) || defined(OS_MACOSX) || \ + (defined(OS_LINUX) && !defined(OS_CHROMEOS)) #include "chrome/browser/browser_switcher/browser_switcher_navigation_throttle.h" #endif @@ -4173,7 +4170,8 @@ ChromeContentBrowserClient::CreateThrottlesForNavigation( if (previews_lite_page_throttle) throttles.push_back(std::move(previews_lite_page_throttle)); -#if defined(OS_WIN) || (defined(OS_LINUX) && !defined(OS_CHROMEOS)) +#if defined(OS_WIN) || defined(OS_MACOSX) || \ + (defined(OS_LINUX) && !defined(OS_CHROMEOS)) std::unique_ptr browser_switcher_throttle = browser_switcher::BrowserSwitcherNavigationThrottle :: MaybeCreateThrottleFor(handle); diff --git a/chrome/browser/prefs/browser_prefs.cc b/chrome/browser/prefs/browser_prefs.cc index d9fb637fbc1fb4..be3fc5c8aacde0 100644 --- a/chrome/browser/prefs/browser_prefs.cc +++ b/chrome/browser/prefs/browser_prefs.cc @@ -311,7 +311,8 @@ #include "chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h" #endif -#if defined(OS_WIN) || (defined(OS_LINUX) && !defined(OS_CHROMEOS)) +#if defined(OS_WIN) || defined(OS_MACOSX) || \ + (defined(OS_LINUX) && !defined(OS_CHROMEOS)) #include "chrome/browser/browser_switcher/browser_switcher_prefs.h" #endif @@ -743,7 +744,8 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) { safe_browsing::PostCleanupSettingsResetter::RegisterProfilePrefs(registry); #endif -#if defined(OS_WIN) || (defined(OS_LINUX) && !defined(OS_CHROMEOS)) +#if defined(OS_WIN) || defined(OS_MACOSX) || \ + (defined(OS_LINUX) && !defined(OS_CHROMEOS)) browser_switcher::prefs::RegisterProfilePrefs(registry); #endif diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn index 15668dec9c57e9..b204fcfe9326e7 100644 --- a/chrome/test/BUILD.gn +++ b/chrome/test/BUILD.gn @@ -2032,10 +2032,6 @@ test("browser_tests") { "../browser/renderer_context_menu/spelling_options_submenu_observer_browsertest.cc", ] } - if (is_win || (is_linux && !is_chromeos)) { - sources += - [ "../browser/browser_switcher/ieem_sitelist_parser_browsertest.cc" ] - } if (!is_posix || is_chromeos) { sources -= [ "../common/time_format_browsertest.cc" ] } @@ -2043,6 +2039,7 @@ test("browser_tests") { if (is_mac || is_win || (is_linux && !is_chromeos)) { sources += [ # Tests for non mobile and non CrOS (includes Linux, Win, Mac). + "../browser/browser_switcher/ieem_sitelist_parser_browsertest.cc", "../browser/extensions/api/image_writer_private/image_writer_utility_client_browsertest.cc", "../browser/profiles/profile_statistics_browsertest.cc", ] @@ -3346,13 +3343,8 @@ test("unit_tests") { } if (is_win || (is_linux && !is_chromeos)) { - sources += [ - "../browser/browser_switcher/alternative_browser_driver_unittest.cc", - "../browser/browser_switcher/alternative_browser_launcher_unittest.cc", - "../browser/browser_switcher/browser_switcher_navigation_throttle_unittest.cc", - "../browser/browser_switcher/browser_switcher_sitelist_unittest.cc", - "../browser/ui/views/quit_instruction_bubble_controller_unittest.cc", - ] + sources += + [ "../browser/ui/views/quit_instruction_bubble_controller_unittest.cc" ] } if (enable_native_notifications) { @@ -4440,7 +4432,13 @@ test("unit_tests") { } if (is_win || is_mac || (is_linux && !is_chromeos)) { - sources += [ "../browser/password_manager/password_store_signin_notifier_impl_unittest.cc" ] + sources += [ + "../browser/browser_switcher/alternative_browser_driver_unittest.cc", + "../browser/browser_switcher/alternative_browser_launcher_unittest.cc", + "../browser/browser_switcher/browser_switcher_navigation_throttle_unittest.cc", + "../browser/browser_switcher/browser_switcher_sitelist_unittest.cc", + "../browser/password_manager/password_store_signin_notifier_impl_unittest.cc", + ] } if (enable_widevine && is_win) {