Use LoadLibraryExW if applicable in LoadNativeLibrary on Windows

base/native_library_win.cc

Index: base/native_library_win.cc
diff --git a/base/native_library_win.cc b/base/native_library_win.cc
index 64c7380f173247c28b78e1f339b963d7374cf83f..f160a6985e1358fb087ec64012c48050db76f498 100644
--- a/base/native_library_win.cc
+++ b/base/native_library_win.cc
@@ -7,6 +7,7 @@
 #include <windows.h>
 
 #include "base/files/file_util.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
@@ -14,16 +15,81 @@
 
 namespace base {
 
-typedef HMODULE (WINAPI* LoadLibraryFunction)(const wchar_t* file_name);
+typedef HMODULE(WINAPI* AddDllDirectory)(PCWSTR NewDirectory);
 
 namespace {
+enum LoadLibraryResult {

[Ilya Sherman, 2017/03/14 18:42:28]

nit: Please document that this is used to back an UMA histogram, and should
therefore be treated as append-only.

[chengx, 2017/03/14 23:08:30]

Done.

+  // LoadLibraryExW API/flags are available and the call succeeds.
+  SUCCEED = 0,
+  // LoadLibraryExW API/flags are availabe to use but the call fails, then
+  // LoadLibraryW is used and succeeds.
+  FAIL_AND_SUCCEED,
+  // LoadLibraryExW API/flags are availabe to use but the call fails, then
+  // LoadLibraryW is used but fails as well.
+  FAIL_AND_FAIL,
+  // LoadLibraryExW API/flags are unavailabe to use, then LoadLibraryW is used
+  // and succeeds.
+  UNAVAILABLE_AND_SUCCEED,
+  // LoadLibraryExW API/flags are unavailabe to use, then LoadLibraryW is used
+  // but fails.
+  UNAVAILABLE_AND_FAIL,
+  // Add new items before this one, always keep this one at the end.
+  END
+};
+
+// A helper method to log library loading result to UMA.
+void LogLibrarayLoadResultToUMA(LoadLibraryResult& result) {

[xhwang, 2017/03/14 21:48:25]

We pretty much never pass non-const ref in Chromium. Just pass by value?

[chengx, 2017/03/14 23:08:30]

Yep, should pass by const ref or pass by value (accepted).

+  UMA_HISTOGRAM_ENUMERATION("LibraryLoader.NativeLibraryDynamicallyLoad",
+                            result, LoadLibraryResult::END);
+}
+
+// A helper method to check if AddDllDirectory method is available, thus
+// LOAD_LIBRARY_SEARCH_* flags are available on systems.
+bool IsLoadLibSearchFlagsAvailable() {

[xhwang, 2017/03/14 21:48:25]

nit: s/Is/Are. Also since this is related to LOAD_LIBRARY_SEARCH flags. So to be
exact, probably name it to

AreLoadLibrarySearchFlagsAvailable()

or simply

AreSearchFlagsAvailable() since this whole file is in the context of
"LoadLibrary".

[chengx, 2017/03/14 23:08:30]

Renamed to AreSearchFlagsAvailable().

+  // The LOAD_LIBRARY_SEARCH_* flags are available on systems that have
+  // KB2533623 installed. To determine whether the flags are available, use
+  // GetProcAddress to get the address of the AddDllDirectory,
+  // RemoveDllDirectory, or SetDefaultDllDirectories function. If GetProcAddress
+  // succeeds, the LOAD_LIBRARY_SEARCH_* flags can be used with LoadLibraryEx.
+  // https://msdn.microsoft.com/en-us/library/windows/desktop/ms684179(v=vs.85).aspx
+  // The LOAD_LIBRARY_SEARCH_* flags are used in LoadNativeLibraryHelperEx
+  // method above.

[xhwang, 2017/03/14 21:48:25]

"above" is not accurate now

[chengx, 2017/03/14 23:08:30]

Done.

+  auto add_dll_dir_func = reinterpret_cast<AddDllDirectory>(
+      GetProcAddress(GetModuleHandle(L"kernel32.dll"), "AddDllDirectory"));
+  return !!add_dll_dir_func;
+}
 
 NativeLibrary LoadNativeLibraryHelper(const FilePath& library_path,
-                                      LoadLibraryFunction load_library_api,
                                       NativeLibraryLoadError* error) {
   // LoadLibrary() opens the file off disk.
   ThreadRestrictions::AssertIOAllowed();
 
+  HMODULE load_library_module = nullptr;

[xhwang, 2017/03/14 21:48:25]

nit: ISTM the old name |module| is concise and clear, and |load_library| here
doesn't add much value.

[chengx, 2017/03/14 23:08:30]

Done.

+
+  // This variable records the library loading result.
+  LoadLibraryResult load_library_status = LoadLibraryResult::SUCCEED;
+
+  bool is_load_lib_flags_available = IsLoadLibSearchFlagsAvailable();
+  if (is_load_lib_flags_available) {
+    // LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR flag is needed to search the library
+    // directory as the library may have dependencies on DLLs in this
+    // directory.
+    load_library_module = ::LoadLibraryExW(
+        library_path.value().c_str(), nullptr,
+        LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR | LOAD_LIBRARY_SEARCH_DEFAULT_DIRS);
+    // If LoadLibraryExW succeeds, log this metric and return.
+    if (load_library_module) {
+      LogLibrarayLoadResultToUMA(load_library_status);
+      return load_library_module;
+    }
+    // GetLastError() needs to be called immediately after
+    // LoadLibraryExW call.
+    if (error)
+      error->code = GetLastError();
+  }
+
+  // If LoadLibraryExW API/flags are unavailable or API call fails, try
+  // LoadLibraryW API.

[xhwang, 2017/03/14 21:48:25]

nit: add an empty line here since these two lines are for all the code left in
this function.

[chengx, 2017/03/14 23:08:30]

Done.

   // Switch the current directory to the library directory as the library
   // may have dependencies on DLLs in this directory.
   bool restore_directory = false;
@@ -36,18 +102,30 @@ NativeLibrary LoadNativeLibraryHelper(const FilePath& library_path,
     }
   }
 
-  HMODULE module = (*load_library_api)(library_path.value().c_str());
-  if (!module && error) {
-    // GetLastError() needs to be called immediately after |load_library_api|.
+  load_library_module = ::LoadLibraryW(library_path.value().c_str());
+
+  // GetLastError() needs to be called immediately after LoadLibraryW call.
+  if (!load_library_module && error)
     error->code = GetLastError();
-  }
 
   if (restore_directory)
     SetCurrentDirectory(current_directory);
 
-  return module;
-}
+  // Record the library loading result.
+  if (is_load_lib_flags_available) {
+    if (load_library_module)
+      load_library_status = LoadLibraryResult::FAIL_AND_SUCCEED;
+    else
+      load_library_status = LoadLibraryResult::FAIL_AND_FAIL;
+  } else if (load_library_module)

[xhwang, 2017/03/14 21:48:25]

nit: use curly here and below as well:

https://google.github.io/styleguide/cppguide.html#Conditionals

// Not allowed - curly on IF but not ELSE
if (condition) {
  foo;
} else
  bar;

[chengx, 2017/03/14 23:08:30]

Done.

+    load_library_status = LoadLibraryResult::UNAVAILABLE_AND_SUCCEED;
+  else
+    load_library_status = LoadLibraryResult::UNAVAILABLE_AND_FAIL;

[xhwang, 2017/03/14 21:48:25]

nit: Probably move these into a helper function as well to keep the main logic
clean:

LoadLibraryResult GetLoadLibraryResult(bool, bool)

[chengx, 2017/03/14 23:08:30]

Done.

+
+  LogLibrarayLoadResultToUMA(load_library_status);
 
+  return load_library_module;
+}
 }  // namespace
 
 std::string NativeLibraryLoadError::ToString() const {
@@ -58,16 +136,7 @@ std::string NativeLibraryLoadError::ToString() const {
 NativeLibrary LoadNativeLibraryWithOptions(const FilePath& library_path,
                                            const NativeLibraryOptions& options,
                                            NativeLibraryLoadError* error) {
-  return LoadNativeLibraryHelper(library_path, LoadLibraryW, error);
-}
-
-NativeLibrary LoadNativeLibraryDynamically(const FilePath& library_path) {
-  typedef HMODULE (WINAPI* LoadLibraryFunction)(const wchar_t* file_name);
-
-  LoadLibraryFunction load_library = reinterpret_cast<LoadLibraryFunction>(
-      GetProcAddress(GetModuleHandle(L"kernel32.dll"), "LoadLibraryW"));
-
-  return LoadNativeLibraryHelper(library_path, load_library, NULL);
+  return LoadNativeLibraryHelper(library_path, error);
 }
 
 // static