M103: Merge three CLs for fixing launch failures.

CLs merged in this CL are

9c26f9b: This change addresses known launch/shutdown/update-swap races.
393e286: Change IsBrowserAlreadyRunning back to using PathService.
a980af7: Change IsBrowserAlreadyRunning to detect browser in same dir.

No conflicts were found in any of these merges.

A full build and manual test for issue 1314491 was carried out.

BUG=1314491

CLs enclosed below:

This change addresses known launch/shutdown/update-swap races.

We check IsRunningOldChrome in PreEarlyInitialization to relaunch
immediately if we are running stale binaries. Calling DoUpgradeTasks at
that point is not safe as the browser process singleton is not owned.

We call DoUpgradeTasks right after an instance obtains the singleton,
which is the only safe way to perform the swap.

We no longer cache IsBrowserAlreadyRunning as doing so results in races.
The value must be checked while owning the singleton and not using a
stored value from before.

DoUpgradeTasks checks for IsBrowserAlreadyRunning to perform the swap
and also checks if we are running stale binaries. This is necessary as
an instance that previously owned the singleton could have done a swap
prior to the new one getting the singleton.

This is not the ideal solution, but it is a substantial improvement.
Ideally, we would have a cross-process mutex (like the browser
singleton) but use it all the way from PreEarlyInitialization. Doing
that is a design change which we can decide to do if the problem is
still severe after all mitigations discussed in Bug 1314491 are in
place.

Bug: 1314491
Change-Id: I7bd42550f49b2f0e58b585a08364ae1be4cec242
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3656507
Reviewed-by: Greg Thompson <grt@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Javier Flores Assad <floresa@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1006064}
(cherry picked from commit 9c26f9b)


Change IsBrowserAlreadyRunning back to using PathService.

A previous CL changed the behavior here to use
QueryFullProcessImageName instead of PathService, which uses
GetModuleFileName under the hood, which reads the value from the
Process Environment Block (PEB) of the process.

Using QueryFullProcessImageName was incorrect, as chrome might have
been renamed between it starting and the first call to
IsBrowserAlreadyRunning in PreEarlyInitialization by another chrome
that started at exactly the wrong time.

However, this bug did not actually manifest, because of a quirk in
QueryFullProcessImageName where it does not correctly function if the
file has been moved then renamed and instead returns the 'wrong' value
which is from the PEB rather than the 'right' value which would have
been wrong.

Because the setup --rename-chrome-exe command moves the file into a
scoped temp directory e.g.
%localappdata%\Google\Chrome SxS\Temp\scoped_dir4752_305957221\old_chrome.exe
it meant that, purely by a coincidence of two bugs, this code was
never actually truly faulty.

However, if QueryFullProcessImageName behavior were to ever change then
using this API would be incorrect as the Event should always be named
after the installation path, and not the scoped temp dir.

Therefore, this change reverts the change back to using PathService /
GetModuleFileName, but retains the correct behavior of both using an NT
path, which fixes the bug described in comment 24, and using the
executable parent directory and not the executable filename, which
fixes the bug described in comment 27.

BUG=1314491

Change-Id: I8ab01e56906820fdd53d1ba502df8acbf5a287c1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3655886
Reviewed-by: Greg Thompson <grt@chromium.org>
Commit-Queue: Will Harris <wfh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1005885}
(cherry picked from commit 393e286)


Change IsBrowserAlreadyRunning to detect browser in same dir.

This change makes IsBrowserAlreadyRunning detect any chrome
executable running from the same (parent) directory as being the
same running browser.

E.g. c:\chrome\new_chrome.exe will now be considered the 'same'
as c:\chrome\chrome.exe.

This means that if a shortcut points to new_chrome.exe (or
old_chrome.exe) and there is also a running chrome.exe, then
chrome will be considered already running, and rendez-vous
with the existing browser will be carried out, and no upgrade
tasks will be performed.

This is an extension to crrev.com/c/3617153.

BUG=1314491

(cherry picked from commit a980af7)

Change-Id: Ib61f17f277aa8ab47f38b8c2888a5abb7494d8a4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3645291
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Will Harris <wfh@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1003226}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3668941
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/branch-heads/5060@{#310}
Cr-Branched-From: b83393d-refs/heads/main@{#1002911}

chrome/browser/chrome_browser_main.cc

@@ -757,14 +757,14 @@ int ChromeBrowserMainParts::PreEarlyInitialization() {
   }
 
 #if BUILDFLAG(IS_WIN)
-  // On Windows, we use our startup as an opportunity to do upgrade/uninstall
-  // tasks.  Those care whether the browser is already running.  On Linux/Mac,
-  // upgrade/uninstall happen separately.
-  already_running_ = browser_util::IsBrowserAlreadyRunning();
-
-  // Do the tasks if chrome has been upgraded while it was last running.
-  if (!already_running_ &&
-      upgrade_util::DoUpgradeTasks(*base::CommandLine::ForCurrentProcess())) {
+  // If we are running stale binaries then relaunch and exit immediately.
+  if (upgrade_util::IsRunningOldChrome()) {
+    if (!upgrade_util::RelaunchChromeBrowser(
+            *base::CommandLine::ForCurrentProcess())) {
+      // The relaunch failed. Feel free to panic now.
+      NOTREACHED();
+    }
+
     // Note, cannot return RESULT_CODE_NORMAL_EXIT here as this code needs to
     // result in browser startup bailing.
     return chrome::RESULT_CODE_NORMAL_EXIT_UPGRADE_RELAUNCHED;
@@ -1361,7 +1361,7 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
   // If the command line specifies 'uninstall' then we need to work here
   // unless we detect another chrome browser running.
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kUninstall)) {
-    return DoUninstallTasks(already_running_);
+    return DoUninstallTasks(browser_util::IsBrowserAlreadyRunning());
   }
 
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kHideIcons) ||
@@ -1449,6 +1449,13 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
       return chrome::RESULT_CODE_PROFILE_IN_USE;
   }
 
+#if BUILDFLAG(IS_WIN)
+  // We must call DoUpgradeTasks now that we own the browser singleton to
+  // finish upgrade tasks (swap) and relaunch if necessary.
+  if (upgrade_util::DoUpgradeTasks(*base::CommandLine::ForCurrentProcess()))
+    return chrome::RESULT_CODE_NORMAL_EXIT_UPGRADE_RELAUNCHED;
+#endif  // BUILDFLAG(IS_WIN)
+
 #if BUILDFLAG(ENABLE_DOWNGRADE_PROCESSING)
   // Begin relaunch processing immediately if User Data migration is required
   // to handle a version downgrade.

chrome/browser/chrome_browser_main.h

@@ -219,13 +219,6 @@ class ChromeBrowserMainParts : public content::BrowserMainParts {
   // Observer that triggers `PostProfileInit()` when new user profiles are
   // created.
   std::unique_ptr<ProfileInitManager> profile_init_manager_;
-
-#if BUILDFLAG(IS_WIN)
-  // Whether or not another browser is already running. This is obtained once
-  // early during startup as each attempt to determine this might race another
-  // browser starting at the same time.
-  bool already_running_ = false;
-#endif
 };
 
 #endif  // CHROME_BROWSER_CHROME_BROWSER_MAIN_H_

chrome/browser/first_run/upgrade_util_win.cc

@@ -35,6 +35,7 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/first_run/upgrade_util.h"
 #include "chrome/browser/shell_integration.h"
+#include "chrome/browser/win/browser_util.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/install_static/install_util.h"
@@ -231,17 +232,26 @@ bool IsRunningOldChrome() {
 bool DoUpgradeTasks(const base::CommandLine& command_line) {
   TRACE_EVENT0("startup", "upgrade_util::DoUpgradeTasks");
   const auto begin_time = base::TimeTicks::Now();
-  if (!SwapNewChromeExeIfPresent() && !IsRunningOldChrome()) {
+  // If there is no other instance already running then check if there is a
+  // pending update and complete it by performing the swap and then relaunch.
+  bool did_swap = false;
+  if (!browser_util::IsBrowserAlreadyRunning())
+    did_swap = SwapNewChromeExeIfPresent();
+
+  // We don't need to relaunch if we didn't swap and we aren't running stale
+  // binaries.
+  if (!did_swap && !IsRunningOldChrome()) {
     UMA_HISTOGRAM_MEDIUM_TIMES("Startup.DoUpgradeTasks.NoRelaunch",
                                base::TimeTicks::Now() - begin_time);
     return false;
   }
+
   // At this point the chrome.exe has been swapped with the new one.
   if (RelaunchChromeBrowser(command_line)) {
     UMA_HISTOGRAM_MEDIUM_TIMES("Startup.DoUpgradeTasks.RelaunchSucceeded",
                                base::TimeTicks::Now() - begin_time);
   } else {
-    // The re-launch failed. Feel free to panic now.
+    // The relaunch failed. Feel free to panic now.
     NOTREACHED();
     // Log a metric anyways to see if this is at fault in crbug.com/1252004
     UMA_HISTOGRAM_MEDIUM_TIMES("Startup.DoUpgradeTasks.RelaunchFailed",

chrome/browser/first_run/upgrade_util_win.h

@@ -11,9 +11,16 @@ class CommandLine;
 
 namespace upgrade_util {
 
-// If the new_chrome.exe exists (placed by the installer then is swapped
-// to chrome.exe and the old chrome is renamed to old_chrome.exe. If there
-// is no new_chrome.exe or the swap fails the return is false;
+// This function is called from chrome.exe and checks if new_chrome.exe exists
+// and if so, it attempts to swap it to become chrome.exe. new_chrome.exe is
+// placed by the installer if there is an update while the browser is being
+// used. If there is no new_chrome.exe or the swap fails the function returns
+// false.
+// To peform the swap, chrome.exe is renamed to old_chrome.exe and moved to a
+// temporary folder, then new_chrome.exe is renamed to chrome.exe. The instance
+// performing the swap runs as old_chrome.exe until it exits.
+// The caller must own the browser ChromeProcessSingleton.
+// TODO: enforce via DCHECK that the caller owns the ChromeProcessSingleton.
 bool SwapNewChromeExeIfPresent();
 
 // Returns true if the currently running browser is running from old_chrome.exe.
@@ -30,6 +37,8 @@ bool IsRunningOldChrome();
 // existing browser process. If this returns true before message loop is
 // executed, simply exit the main function. If browser is already running, you
 // will need to exit it.
+// The caller must own the browser ChromeProcessSingleton.
+// TODO: enforce via DCHECK that the caller owns the ChromeProcessSingleton.
 bool DoUpgradeTasks(const base::CommandLine& command_line);
 
 }  // namespace upgrade_util

chrome/browser/win/browser_util.cc

@@ -9,62 +9,44 @@
 #include <algorithm>
 #include <string>
 
-#include "base/logging.h"
+#include "base/base_paths.h"
+#include "base/files/file_path.h"
+#include "base/path_service.h"
+#include "sandbox/win/src/win_utils.h"
 
 namespace browser_util {
 
-namespace {
-
-// Determine the NT path name for the current process. Returns an empty path if
-// a failure occurs.
-std::wstring GetCurrentProcessExecutablePath() {
-  std::wstring image_path;
-  image_path.resize(MAX_PATH);
-  DWORD path_length = image_path.size();
-  BOOL success =
-      ::QueryFullProcessImageNameW(::GetCurrentProcess(), PROCESS_NAME_NATIVE,
-                                   image_path.data(), &path_length);
-  if (!success && ::GetLastError() == ERROR_INSUFFICIENT_BUFFER) {
-    // Process name is potentially greater than MAX_PATH, try larger max size.
-    // https://docs.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation
-    image_path.resize(UNICODE_STRING_MAX_CHARS);
-    path_length = image_path.size();
-    success =
-        ::QueryFullProcessImageNameW(::GetCurrentProcess(), PROCESS_NAME_NATIVE,
-                                     image_path.data(), &path_length);
-  }
-  if (!success) {
-    PLOG_IF(ERROR, ::GetLastError() != ERROR_GEN_FAILURE)
-        << "Failed to get process image path";
-    return std::wstring();
-  }
-  image_path.resize(path_length);
-  return image_path;
-}
-
-}  // namespace
-
 bool IsBrowserAlreadyRunning() {
-  static HANDLE handle = NULL;
-
-  std::wstring nt_path_name = GetCurrentProcessExecutablePath();
-  if (nt_path_name.empty()) {
+  static HANDLE handle = nullptr;
+  base::FilePath exe_dir_path;
+  // DIR_EXE is obtained from the path of FILE_EXE and, on Windows, FILE_EXE is
+  // obtained from reading the PEB of the currently running process. This means
+  // that even if the EXE file is moved, the DIR_EXE will still reflect the
+  // original location of the EXE from when it was started. This is important as
+  // IsBrowserAlreadyRunning must detect any running browser in Chrome's install
+  // directory, and not in a temporary directory if it is subsequently renamed
+  // or moved while running.
+  if (!base::PathService::Get(base::DIR_EXE, &exe_dir_path)) {
     // If this fails, there isn't much that can be done. However, assuming that
     // browser is *not* already running is the safer action here, as it means
     // that any pending upgrade actions will occur and hopefully the issue that
     // caused this failure will be resolved by the newer version. This might
     // cause the currently running browser to be temporarily broken, but it's
-    // probably broken already if QueryFullProcessImageNameW is failing.
+    // probably broken already if this API is failing.
     return false;
   }
-
-  std::replace(nt_path_name.begin(), nt_path_name.end(), '\\', '!');
-  std::transform(nt_path_name.begin(), nt_path_name.end(), nt_path_name.begin(),
+  std::wstring nt_dir_name;
+  if (!sandbox::GetNtPathFromWin32Path(exe_dir_path.value(), &nt_dir_name)) {
+    // See above for why false is returned here.
+    return false;
+  }
+  std::replace(nt_dir_name.begin(), nt_dir_name.end(), '\\', '!');
+  std::transform(nt_dir_name.begin(), nt_dir_name.end(), nt_dir_name.begin(),
                  tolower);
-  nt_path_name = L"Global\\" + nt_path_name;
+  nt_dir_name = L"Global\\" + nt_dir_name;
   if (handle != NULL)
     ::CloseHandle(handle);
-  handle = ::CreateEventW(NULL, TRUE, TRUE, nt_path_name.c_str());
+  handle = ::CreateEventW(NULL, TRUE, TRUE, nt_dir_name.c_str());
   int error = ::GetLastError();
   return (error == ERROR_ALREADY_EXISTS || error == ERROR_ACCESS_DENIED);
 }