tor-browser

commit a805e95c66b946927146357b95497c912d897bc7
parent 295418f54d16cf7f8f10e5c8625496a8e22b951d
Author: Gabriele Svelto <gsvelto@mozilla.com>
Date:   Mon, 10 Nov 2025 13:38:58 +0000

Bug 1998231 - [mozcrash] Fix a race when terminating a process that had already shut down on Windows r=afranchuk

This handles the scenario when mozcrash attempts to terminate a process,
but the process has already shut down, which is a possible race within
this code. It also changes the minidumpwriter helper so that it won't
report an error when trying to capture a minidump of a process that has
already stopped running.

Differential Revision: https://phabricator.services.mozilla.com/D271387

Diffstat:
M
testing/mozbase/mozcrash/mozcrash/mozcrash.py
 | 
32
+++++++++++++++++++++-----------
M
testing/tools/minidumpwriter/minidumpwriter.cpp
 | 
8
+++++++-

2 files changed, 28 insertions(+), 12 deletions(-)
diff --git a/testing/mozbase/mozcrash/mozcrash/mozcrash.py b/testing/mozbase/mozcrash/mozcrash/mozcrash.py
@@ -799,11 +799,16 @@ if mozinfo.isWin:
         :param pid: PID of the process to terminate.
         """
         PROCESS_TERMINATE = 0x0001
+        PROCESS_QUERY_LIMITED_INFORMATION = 0x1000
         SYNCHRONIZE = 0x00100000
         WAIT_OBJECT_0 = 0x0
         WAIT_FAILED = -1
+        ERROR_ACCESS_DENIED = 5
+        STILL_ACTIVE = 259
         logger = get_logger()
-        handle = OpenProcess(PROCESS_TERMINATE | SYNCHRONIZE, 0, pid)
+        handle = OpenProcess(
+            PROCESS_TERMINATE | PROCESS_QUERY_LIMITED_INFORMATION | SYNCHRONIZE, 0, pid
+        )
         if handle:
             if kernel32.TerminateProcess(handle, 1):
                 # TerminateProcess is async; wait up to 30 seconds for process to
@@ -813,25 +818,30 @@ if mozinfo.isWin:
                 if status == WAIT_FAILED:
                     err = kernel32.GetLastError()
                     logger.warning(
-                        "kill_pid(): wait failed (%d) terminating pid %d: error %d"
-                        % (status, pid, err)
+                        f"kill_pid(): wait failed ({status}) terminating pid {pid}: error {err}"
                     )
                 elif status != WAIT_OBJECT_0:
                     logger.warning(
-                        "kill_pid(): wait failed (%d) terminating pid %d"
-                        % (status, pid)
+                        f"kill_pid(): wait failed ({status}) terminating pid {pid}"
                     )
             else:
                 err = kernel32.GetLastError()
-                logger.warning(
-                    "kill_pid(): unable to terminate pid %d: %d" % (pid, err)
-                )
+                status = ctypes.c_uint()
+                result = kernel32.GetExitCodeProcess(handle, ctypes.byref(status))
+                if (err == ERROR_ACCESS_DENIED) and result and (status != STILL_ACTIVE):
+                    pass  # Process had already terminated when we called `TerminateProcess()`
+                else:
+                    logger.warning(
+                        f"kill_pid(): unable to terminate pid {pid}: error {err}"
+                    )
+                    if not result:
+                        logger.warning(
+                            f"kill_pid(): process {pid} status is unknown: error {kernel32.GetLastError()}"
+                        )
             CloseHandle(handle)
         else:
             err = kernel32.GetLastError()
-            logger.warning(
-                "kill_pid(): unable to get handle for pid %d: %d" % (pid, err)
-            )
+            logger.warning(f"kill_pid(): unable to get handle for pid {pid}: {err}")
 
 else:
 
diff --git a/testing/tools/minidumpwriter/minidumpwriter.cpp b/testing/tools/minidumpwriter/minidumpwriter.cpp
@@ -48,7 +48,13 @@ int wmain(int argc, wchar_t** argv) {
   if (!MiniDumpWriteDump(hProcess, pid, file, MiniDumpNormal, nullptr, nullptr,
                          nullptr)) {
     fprintf(stderr, "Error 0x%lX in MiniDumpWriteDump\n", GetLastError());
-    rv = 1;
+    DWORD status = 0;
+    if (!GetExitCodeProcess(hProcess, &status) || (status == STILL_ACTIVE)) {
+      // We return an error only if the process was still running. If we failed
+      // because the process had already been terminated then don't consider it
+      // an actual error.
+      rv = 1;
+    }
   }
 
   CloseHandle(file);