Use PathService in installer::GetModulePath().

chrome/installer/util/module_util_win.cc

Index: chrome/installer/util/module_util_win.cc
diff --git a/chrome/installer/util/module_util_win.cc b/chrome/installer/util/module_util_win.cc
index c589263690a9c127ca795d8b75e3b449a2fdca6f..d3b78d5d9a83a097682a4e2d18a234ffb6b53267 100644
--- a/chrome/installer/util/module_util_win.cc
+++ b/chrome/installer/util/module_util_win.cc
@@ -2,14 +2,14 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include <windows.h>
-
 #include "chrome/installer/util/module_util_win.h"
 
+#include "base/base_paths.h"
 #include "base/file_version_info.h"
 #include "base/files/file.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/path_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/version.h"
 
@@ -17,13 +17,6 @@ namespace installer {
 
 namespace {
 
-// Returns the directory in which the currently running executable resides.
-base::FilePath GetExecutableDir() {
-  base::char16 path[MAX_PATH];
-  ::GetModuleFileNameW(nullptr, path, MAX_PATH);
-  return base::FilePath(path).DirName();
-}
-
 // Returns the version in the current module's version resource or the empty
 // string if none found.
 base::string16 GetCurrentModuleVersion() {
@@ -31,7 +24,7 @@ base::string16 GetCurrentModuleVersion() {
       CREATE_FILE_VERSION_INFO_FOR_CURRENT_MODULE());
   if (file_version_info.get()) {
     base::string16 version_string(file_version_info->file_version());
-    if (Version(base::UTF16ToASCII(version_string)).IsValid())
+    if (base::Version(base::UTF16ToASCII(version_string)).IsValid())
       return version_string;
   }
   return base::string16();
@@ -50,16 +43,18 @@ base::FilePath GetModulePath(base::StringPiece16 module_name,
                              base::string16* version) {
   DCHECK(version);
 
-  base::FilePath module_dir = GetExecutableDir();
+  base::FilePath module_dir;

[gab, 2015/11/17 14:45:58]

This won't store the module's dir, but the application's dir.

s/module_dir/application_dir/

[Peter Kasting, 2015/11/17 20:24:18]

This has ramifications for the function name here as well.  My suggestion of
DIR_EXE was based on what I believed the old code was doing.  However, the path
service does have separate selectors for the executable dir and the module dir. 
So perhaps either we should make the code here match the names, or else we
should change not only this variable name but the function names (etc.) as well
to make it clear this is the executable dir.

I don't know the context here well enough to judge; I assume you do.

[fdoray, 2015/11/17 22:10:37]

I changed the variable name, but not the function name. The purpose of this
function is to get the path to a module (e.g. chrome.dll or chrome_child.dll),
not an executable, so the name is correct. Currently, it is used to get the path
to chrome.dll or chrome_child.dll from chrome.exe.

We use base::DIR_EXE instead of base::DIR_MODULE to avoid having a different
logic depending on whether this is called from chrome.exe or chrome.dll. The
current implementation works both from chrome.exe and chrome.dll.

+  if (!base::PathService::Get(base::DIR_EXE, &module_dir))
+    return base::FilePath();

[gab, 2015/11/17 14:45:58]

Add NOTREACHED(), this should never happen.

[Peter Kasting, 2015/11/17 20:24:18]

If it should never happen, NOTREACHED() wouldn't be appropriate, as that would
be handling DCHECK failure (which our style guide bans); instead we'd want to do
something like:

  const bool has_path = base::PathService::Get(base::DIR_EXE, &module_dir);
  DCHECK(has_path);

  ...

The same comment applies below.

(Incidentally, if this truly can't ever fail, I'm very much in favor of adding
the DCHECKs.  In that case, though, we probably also want to go to the caller(s)
and remove code that handles failure, as well as documenting in the header that
this routine can't fail.)

[fdoray, 2015/11/17 22:10:37]

base::PathService::Get(base::DIR_EXE, &exe_dir) should never fail, so I added a
DCHECK. I cannot change the signature of base::PathService::Get because it can
fail with parameters other than base::DIR_EXE (e.g. base::DIR_APP_SHORTCUTS
prior to Win8). 

+
   base::FilePath module = module_dir.Append(module_name);
   if (ModuleCanBeRead(module))
     return module;
 
   base::string16 version_string(GetCurrentModuleVersion());
-  if (version_string.empty()) {
-    LOG(ERROR) << "No valid Chrome version found";
+  if (version_string.empty())
     return base::FilePath();

[gab, 2015/11/17 14:45:58]

Pretty sure this should also never happen, +NOTREACHED() -- i.e. let the
developer know the issue is here instead of hunting for "why chrome doesn't
launch" if this ever happens.

[fdoray, 2015/11/17 22:10:37]

It will not fail if the Chrome binary has a valid version number, so I added a
DCHECK.

-  }
+
   *version = version_string;
   return module_dir.Append(version_string).Append(module_name);
 }