Add a --force-browser-process-termination flag to the test runner (#25512)

A major headache when working with the browser test runs in both the
single-threaded and parallel harness on the CI are stray browser
processes that stay alive across browser test runs.

This happens with Safari, and also with Firefox browser, on each of
Windows, Linux and macOS systems.

It is easy to get especially MacOS systems to hang so that one cannot
even remote-desktop in to the CI hardware anymore, since all system
resources are taken up by the runaway browser processes.

One might argue that these are just "bugs", and after we fix those one
by one, we would have a working test harness where browser processes are
cleanly shut down so that stray browser processes would not remain.

That might be the case, though we need a system that is robust to these
types of bugs.

To help this, add a failsafe mechanism in the form of a new
`--force-browser-process-termination` flag to the test runner, to help
tear down browser processes before and after a test suite run. This way
even if the harness is misbehaving, there will be a backstop that
aggressively attempts to clean up all browsers between test runs.

This way I can have the CI tasks force-shutdown all runners between
suite runs, to ensure that even if things are still not working 100%, I
will not need to go and physically keep pressing the power button on my
Mac Minis to boot them back up.

Author: juj

test/common.py

@@ -199,6 +199,14 @@ def is_firefox():
   return EMTEST_BROWSER and 'firefox' in EMTEST_BROWSER.lower()
 
 
+def get_browser_config():
+  if is_chrome():
+    return ChromeConfig()
+  elif is_firefox():
+    return FirefoxConfig()
+  return None
+
+
 def compiler_for(filename, force_c=False):
   if shared.suffix(filename) in ('.cc', '.cxx', '.cpp') and not force_c:
     return EMXX
@@ -2410,11 +2418,7 @@ def configure_test_browser():
     EMTEST_BROWSER = '"' + EMTEST_BROWSER.replace("\\", "\\\\") + '"'
 
   if EMTEST_BROWSER_AUTO_CONFIG:
-    config = None
-    if is_chrome():
-      config = ChromeConfig()
-    elif is_firefox():
-      config = FirefoxConfig()
+    config = get_browser_config()
     if config:
       EMTEST_BROWSER += ' ' + ' '.join(config.default_flags)
       if EMTEST_HEADLESS == 1:
@@ -2435,6 +2439,22 @@ def list_processes_by_name(exe_name):
   return pids
 
 
+def terminate_list_of_processes(proc_list):
+  for proc in proc_list:
+    try:
+      proc.terminate()
+      # If the browser doesn't shut down gracefully (in response to SIGTERM)
+      # after 2 seconds kill it with force (SIGKILL).
+      try:
+        proc.wait(2)
+      except (subprocess.TimeoutExpired, psutil.TimeoutExpired):
+        logger.info('Browser did not respond to `terminate`.  Using `kill`')
+        proc.kill()
+        proc.wait()
+    except (psutil.NoSuchProcess, ProcessLookupError):
+      pass
+
+
 class FileLock:
   """Implements a filesystem-based mutex, with an additional feature that it
   returns an integer counter denoting how many times the lock has been locked
@@ -2519,19 +2539,7 @@ def __init__(self, *args, **kwargs):
 
   @classmethod
   def browser_terminate(cls):
-    for proc in cls.browser_procs:
-      try:
-        proc.terminate()
-        # If the browser doesn't shut down gracefully (in response to SIGTERM)
-        # after 2 seconds kill it with force (SIGKILL).
-        try:
-          proc.wait(2)
-        except (subprocess.TimeoutExpired, psutil.TimeoutExpired):
-          logger.info('Browser did not respond to `terminate`.  Using `kill`')
-          proc.kill()
-          proc.wait()
-      except (psutil.NoSuchProcess, ProcessLookupError):
-        pass
+    terminate_list_of_processes(cls.browser_procs)
 
   @classmethod
   def browser_restart(cls):
@@ -2564,11 +2572,8 @@ def browser_open(cls, url):
       # Recreate the new data directory.
       os.mkdir(browser_data_dir)
 
-      if is_chrome():
-        config = ChromeConfig()
-      elif is_firefox():
-        config = FirefoxConfig()
-      else:
+      config = get_browser_config()
+      if not config:
         exit_with_error(f'EMTEST_BROWSER_AUTO_CONFIG only currently works with firefox or chrome. EMTEST_BROWSER was "{EMTEST_BROWSER}"')
       if WINDOWS:
         # Escape directory delimiter backslashes for shlex.split.

test/runner.py

@@ -488,6 +488,7 @@ def parse_args():
                            'Useful when combined with --failfast')
   parser.add_argument('--force64', action='store_true')
   parser.add_argument('--crossplatform-only', action='store_true')
+  parser.add_argument('--force-browser-process-termination', action='store_true', help='If true, a fail-safe method is used to ensure that all browser processes are terminated before and after the test suite run. Note that this option will terminate all browser processes, not just those launched by the harness, so will result in loss of all open browsing sessions.')
   parser.add_argument('--repeat', type=int, default=1,
                       help='Repeat each test N times (default: 1).')
   parser.add_argument('--bell', action='store_true', help='Play a sound after the test suite finishes.')
@@ -566,6 +567,18 @@ def set_env(name, option_value):
   utils.delete_file(common.flaky_tests_log_filename)
   utils.delete_file(common.browser_spawn_lock_filename)
   utils.delete_file(f'{common.browser_spawn_lock_filename}_counter')
+  if options.force_browser_process_termination or os.getenv('EMTEST_FORCE_BROWSER_PROCESS_TERMINATION'):
+    config = common.get_browser_config()
+
+    def terminate_all_browser_processes():
+      procs = common.list_processes_by_name(config.executable_name)
+      if len(procs) > 0:
+        print(f'Terminating {len(procs)} stray browser processes.')
+        common.terminate_list_of_processes(procs)
+
+    if config and hasattr(config, 'executable_name'):
+      atexit.register(terminate_all_browser_processes)
+      terminate_all_browser_processes()
 
   def prepend_default(arg):
     if arg.startswith('test_'):