Revert "Restart ash chrome when it crashed in tests"

This reverts commit f3f8964.

Reason for revert: This change introduced too many ash/lacros
details to //content/. As discussed with jam@ and erikchen@,
we decided to not support auto restart ash. It doesn't happen
often enough that we have to address the issue.

Original change's description:
> Restart ash chrome when it crashed in tests
>
> Occasionally, a bad test can crash ash chrome, which then cause
> all following tests fail. With this change, all the following tests
> will not fail due to crashed ash chrome.
>
> Currently, ash chrome is started by build/lacros/test_runner.py
> before invoke test binary. It is impossible to reuse any logic there.
> So we modify browser test framework. When the framework starts Lacros
> and fails to connect to ash chrome, we expect ash chrome was crashed
> and then try to start a new ash chrome.
>
> After all test runner processes are finished, the test launcher needs
> to terminate all ash chrome processes started by test binary. Because
> all ash processes need to outlive their parent processes(test runner),
> or else ash chrome will be killed and restarted for every test.
>
> This change is also needed to support 2 requests:
> crbug.com/1401750: Run interactive_ui_tests in parallel.
> crbug.com/1368284: Support ash feature flags for Lacros browser tests.
>
> PRE_CrashAshChrome crash ash chrome intentionally. Without this change,
> all other tests report as crashed. With this change, only
> PRE_CrashAshChrome report as crashed and all following tests report as
> passed. Unfortunately I can't submit that test because PRE_CrashAshChrome is intentionally crash. But all the logic here will
> be used to support crbug.com/1368284 so we will have test coverage.
>
> Bug: 1401520
> Test: crrev.com/c/4112333 shows a demo browser test. The
> Change-Id: I6b44a8d2fd4ccfc4c972b0323fb77ae356efb936
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4113004
> Reviewed-by: Erik Chen <erikchen@chromium.org>
> Reviewed-by: Colin Blundell <blundell@chromium.org>
> Commit-Queue: Colin Blundell <blundell@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1086245}

Bug: 1401520
Change-Id: I0a632273b0abfd81c601194b894d6801ed471e64
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4261454
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Sven Zheng <svenzheng@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1106914}

build/lacros/test_runner.py

@@ -608,8 +608,6 @@ def _RunTestWithAshChrome(args, forward_args):
     if enable_mojo_crosapi:
       forward_args.append(lacros_mojo_socket_arg)
 
-    forward_args.append("--ash-chrome-path=%s" % ash_chrome_file)
-    forward_args.append("--ash-user-data-dir=%s" % tmp_ash_data_dir_name)
     test_env = os.environ.copy()
     test_env['WAYLAND_DISPLAY'] = ash_wayland_socket_name
     test_env['EGL_PLATFORM'] = 'surfaceless'

build/lacros/test_runner_test.py

@@ -80,10 +80,6 @@ def test_do_not_require_ash_chrome(self, command, mock_popen, mock_download,
   @mock.patch.object(os.path, 'exists', return_value=True)
   @mock.patch.object(os.path, 'isfile', return_value=True)
   @mock.patch.object(os.path, 'abspath', return_value='/a/b/filter')
-  @mock.patch.object(os.path, 'dirname', return_value='/some/dir')
-  @mock.patch.object(test_runner,
-                     '_GetAshChromeDirPath',
-                     return_value='/some/dir')
   @mock.patch.object(test_runner,
                      '_GetLatestVersionOfAshChrome',
                      return_value='793554')
@@ -101,7 +97,8 @@ def test_require_ash_chrome(self, command, mock_popen, mock_download, *_):
       self.assertEqual(2, mock_popen.call_count)
 
       ash_chrome_args = mock_popen.call_args_list[0][0][0]
-      self.assertTrue(ash_chrome_args[0].endswith('/some/dir/test_ash_chrome'))
+      self.assertTrue(ash_chrome_args[0].endswith(
+          'build/lacros/prebuilt_ash_chrome/793554/test_ash_chrome'))
       expected_ash_chrome_args = [
           '--user-data-dir=/tmp/ash-data',
           '--enable-wayland-server',
@@ -128,8 +125,6 @@ def test_require_ash_chrome(self, command, mock_popen, mock_download, *_):
             command,
             '--test-launcher-filter-file=/a/b/filter',
             '--lacros-mojo-socket-for-testing=/tmp/ash-data/lacros.sock',
-            '--ash-chrome-path=/some/dir/test_ash_chrome',
-            '--ash-user-data-dir=/tmp/ash-data',
         ], test_args)
       else:
         self.assertListEqual(test_args[:len(command_parts)], command_parts)

chrome/test/base/chrome_test_launcher.cc

@@ -77,13 +77,6 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #endif
 
-#if BUILDFLAG(IS_CHROMEOS_LACROS)
-#include "base/files/file_enumerator.h"
-#include "base/files/file_util.h"
-#include "base/strings/string_number_conversions.h"
-#include "content/public/test/browser_test_switches.h"
-#endif
-
 // static
 int ChromeTestSuiteRunner::RunTestSuiteInternal(ChromeTestSuite* test_suite) {
   // Browser tests are expected not to tear-down various globals.
@@ -240,35 +233,12 @@ void ChromeTestLauncherDelegate::PreSharding() {
   if (IsUserAnAdmin())
     firewall_rules_ = std::make_unique<ScopedFirewallRules>();
 #endif
-
-#if BUILDFLAG(IS_CHROMEOS_LACROS)
-  CHECK(ash_processes_dir_.CreateUniqueTempDir());
-  base::CommandLine::ForCurrentProcess()->AppendSwitchPath(
-      content::test::switches::kAshProcessesDirPath,
-      ash_processes_dir_.GetPath());
-#endif
 }
 
 void ChromeTestLauncherDelegate::OnDoneRunningTests() {
 #if BUILDFLAG(IS_WIN)
   firewall_rules_.reset();
 #endif
-
-// In test runners, they may create ash processes outlive the runner process.
-// So we need to terminate those ash processes in the test launcher.
-#if BUILDFLAG(IS_CHROMEOS_LACROS)
-  base::FileEnumerator files(ash_processes_dir_.GetPath(), false,
-                             base::FileEnumerator::FILES);
-  for (base::FilePath name = files.Next(); !name.empty(); name = files.Next()) {
-    std::string str_pid;
-    CHECK(base::ReadFileToString(name, &str_pid));
-    int pid;
-    CHECK(base::StringToInt(str_pid, &pid)) << "Cannot read pid. The content "
-                                            << "of the file is: " << str_pid;
-    base::Process process = base::Process::Open(pid);
-    process.Terminate(0, false);
-  }
-#endif
 }
 
 int LaunchChromeTests(size_t parallel_jobs,

chrome/test/base/chrome_test_launcher.h

@@ -18,10 +18,6 @@
 #include "chrome/app/chrome_main_delegate.h"
 #endif
 
-#if BUILDFLAG(IS_CHROMEOS_LACROS)
-#include "base/files/scoped_temp_dir.h"
-#endif
-
 class ChromeTestSuite;
 
 // Allows a test suite to override the TestSuite class used. By default it is an
@@ -90,10 +86,6 @@ class ChromeTestLauncherDelegate : public content::TestLauncherDelegate {
 #endif
 
   raw_ptr<ChromeTestSuiteRunner> runner_;
-
-#if BUILDFLAG(IS_CHROMEOS_LACROS)
-  base::ScopedTempDir ash_processes_dir_;
-#endif
 };
 
 // Launches Chrome browser tests. |parallel_jobs| is number of test jobs to be

content/public/test/browser_test_base.cc

@@ -123,16 +123,10 @@
 #endif
 
 #if BUILDFLAG(IS_CHROMEOS_LACROS)
-#include "base/base_switches.h"
-#include "base/files/file_path_watcher.h"
-#include "base/files/file_util.h"
 #include "base/files/scoped_file.h"
-#include "base/strings/string_util.h"
-#include "base/test/task_environment.h"
 #include "chromeos/crosapi/cpp/crosapi_constants.h"  // nogncheck
 #include "chromeos/lacros/lacros_test_helper.h"
 #include "chromeos/startup/startup_switches.h"  // nogncheck
-#include "content/public/test/browser_test_switches.h"
 #include "mojo/public/cpp/platform/socket_utils_posix.h"
 #endif
 
@@ -436,35 +430,17 @@ void BrowserTestBase::SetUp() {
       disable_crosapi_ =
           std::make_unique<chromeos::ScopedDisableCrosapiForTesting>();
     } else {
-      bool connected_to_ash = false;
-      int retry_left = 2;
-      base::ScopedFD socket_fd;
-      int flags = 0;
-      while (retry_left-- > 0 && !connected_to_ash) {
-        auto channel = mojo::NamedPlatformChannel::ConnectToServer(socket_path);
-        socket_fd = channel.TakePlatformHandle().TakeFD();
-
-        // Mark the channel as blocking.
-        flags = fcntl(socket_fd.get(), F_GETFL);
-
-        connected_to_ash = flags != -1;
-
-        if (!connected_to_ash) {
-          LOG(WARNING) << "Ash is probably not running. Perhaps it crashed?"
-                       << " Try to start ash again.";
-          StartAshChrome();
-        }
-      }
+      auto channel = mojo::NamedPlatformChannel::ConnectToServer(socket_path);
+      base::ScopedFD socket_fd = channel.TakePlatformHandle().TakeFD();
+
+      // Mark the channel as blocking.
+      int flags = fcntl(socket_fd.get(), F_GETFL);
       std::string helper_msg =
           "On bot, open CAS outputs on test result page(Milo),"
           "there is a ash_chrome.log file which contains ash log."
           "For local debugging, pass in --ash-logging-path to test runner.";
-      if (connected_to_ash) {
-        LOG(INFO) << "Connected to ash.";
-      } else {
-        LOG(FATAL) << "Ash is probably not running. Perhaps it crashed?"
-                   << helper_msg;
-      }
+      PCHECK(flags != -1) << "Ash is probably not running. Perhaps it crashed?"
+                          << helper_msg;
       fcntl(socket_fd.get(), F_SETFL, flags & ~O_NONBLOCK);
 
       uint8_t buf[32];
@@ -1165,82 +1141,4 @@ ContentMainDelegate* BrowserTestBase::GetOptionalContentMainDelegateOverride() {
   return nullptr;
 }
 
-#if BUILDFLAG(IS_CHROMEOS_LACROS)
-void BrowserTestBase::StartAshChrome() {
-  base::test::SingleThreadTaskEnvironment task_environment;
-  base::CommandLine* cmdline = base::CommandLine::ForCurrentProcess();
-
-  base::FilePath mojo_socket_file =
-      cmdline->GetSwitchValuePath("lacros-mojo-socket-for-testing");
-  if (mojo_socket_file.empty()) {
-    LOG(WARNING) << "Start ash failed because of missing "
-                 << "--lacros-mojo-socket-for-testing";
-    return;
-  }
-  // Delete the existing file because we will reuse the same file for the new
-  // ash chrome instance.
-  CHECK(base::DeleteFile(mojo_socket_file));
-  base::FilePath ash_chrome_path =
-      cmdline->GetSwitchValuePath("ash-chrome-path");
-  CHECK(!ash_chrome_path.empty());
-
-  base::CommandLine ash_cmdline(ash_chrome_path);
-  base::FilePath ash_user_data_dir =
-      cmdline->GetSwitchValuePath(content::test::switches::kAshUserDataDir);
-  CHECK(base::DeletePathRecursively(ash_user_data_dir));
-  ash_cmdline.AppendSwitchPath("user-data-dir", ash_user_data_dir);
-  ash_cmdline.AppendSwitch("enable-wayland-server");
-  ash_cmdline.AppendSwitch("no-startup-window");
-  ash_cmdline.AppendSwitch("disable-lacros-keep-alive");
-  ash_cmdline.AppendSwitch("disable-login-lacros-opening");
-
-  std::string ash_features = "LacrosSupport,LacrosPrimary,LacrosOnly";
-  ash_cmdline.AppendSwitchASCII(switches::kEnableFeatures, ash_features);
-
-  ash_cmdline.AppendSwitchPath("lacros-mojo-socket-for-testing",
-                               mojo_socket_file);
-
-  std::string wayland_socket;
-  CHECK(
-      base::Environment::Create()->GetVar("WAYLAND_DISPLAY", &wayland_socket));
-  DCHECK(wayland_socket.length() > 0);
-  ash_cmdline.AppendSwitchASCII("wayland-server-socket", wayland_socket);
-
-  const std::string ash_ready_file =
-      ash_user_data_dir.AppendASCII("ash_ready.txt").MaybeAsASCII();
-  ash_cmdline.AppendSwitchASCII(content::test::switches::kAshReadyFilePath,
-                                ash_ready_file);
-
-  base::FilePathWatcher watcher;
-  base::RunLoop run_loop;
-  CHECK(watcher.Watch(base::FilePath(ash_ready_file),
-                      base::FilePathWatcher::Type::kNonRecursive,
-                      base::BindLambdaForTesting(
-                          [&](const base::FilePath& filepath, bool error) {
-                            CHECK(!error);
-                            run_loop.Quit();
-                          })));
-  base::LaunchOptions option;
-  option.new_process_group = true;
-  base::Process process = base::LaunchProcess(ash_cmdline, option);
-  CHECK(process.IsValid());
-  run_loop.Run();
-  // When ash is ready and crosapi was enabled, we expect mojo socket is
-  // also ready.
-  CHECK(base::PathExists(mojo_socket_file));
-  base::FilePath ash_processes_dir = cmdline->GetSwitchValuePath(
-      content::test::switches::kAshProcessesDirPath);
-  CHECK(!ash_processes_dir.empty());
-  base::FilePath temp_filepath;
-  std::string str_pid = base::StringPrintf("%d", process.Pid());
-  base::File f =
-      CreateAndOpenTemporaryFileInDir(ash_processes_dir, &temp_filepath);
-  int ret_code = f.Write(0, str_pid.c_str(), str_pid.length());
-  CHECK(ret_code >= 0 && (unsigned int)ret_code == str_pid.length())
-      << "Cannot write to file " << temp_filepath.AsUTF8Unsafe()
-      << "Return code is " << ret_code;
-  f.Close();
-}
-#endif
-
 }  // namespace content

content/public/test/browser_test_base.h

@@ -102,10 +102,6 @@ class BrowserTestBase : public ::testing::Test {
   // needed when triggering the crash via SimulateNetworkServiceCrash method.
   void IgnoreNetworkServiceCrashes();
 
-#if BUILDFLAG(IS_CHROMEOS_LACROS)
-  void StartAshChrome();
-#endif
-
   // Returns the host resolver being used for the tests. Subclasses might want
   // to configure it inside tests.
   net::RuleBasedHostResolverProc* host_resolver() {

content/test/BUILD.gn

@@ -142,8 +142,6 @@ static_library("test_support") {
     "../public/test/browser_test.h",
     "../public/test/browser_test_base.cc",
     "../public/test/browser_test_base.h",
-    "../public/test/browser_test_switches.cc",
-    "../public/test/browser_test_switches.h",
     "../public/test/browser_test_utils.cc",
     "../public/test/browser_test_utils.h",
     "../public/test/browsing_data_remover_test_util.cc",