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..0cefb0c45e243ab9eae65d570053a0180531b59f 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,10 +15,14 @@
 
 namespace base {
 
-typedef HMODULE (WINAPI* LoadLibraryFunction)(const wchar_t* file_name);
+typedef HMODULE(WINAPI* AddDllDirectory)(PCWSTR NewDirectory);
+typedef HMODULE(WINAPI* LoadLibraryFunction)(const wchar_t* file_name);
+typedef HMODULE(WINAPI* LoadLibraryFunctionEx)(const wchar_t* file_name,
+                                               HANDLE hFile,
+                                               DWORD dwFlags);
 
 namespace {
-
+// This helper method uses LoadLibrary() WinAPI.
 NativeLibrary LoadNativeLibraryHelper(const FilePath& library_path,
                                       LoadLibraryFunction load_library_api,
                                       NativeLibraryLoadError* error) {
@@ -48,6 +53,26 @@ NativeLibrary LoadNativeLibraryHelper(const FilePath& library_path,
   return module;
 }
 
+// This helper method uses LoadLibraryEx() WinAPI.
+NativeLibrary LoadNativeLibraryHelperEx(const FilePath& library_path,
+                                        LoadLibraryFunctionEx load_library_api,
+                                        NativeLibraryLoadError* error) {
+  // LoadLibraryEx() opens the file off disk.
+  ThreadRestrictions::AssertIOAllowed();
+
+  // 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.
+  HMODULE module = (*load_library_api)(
+      library_path.value().c_str(), nullptr,
+      LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR | LOAD_LIBRARY_SEARCH_DEFAULT_DIRS);
+  if (!module && error) {
+    // GetLastError() needs to be called immediately after |load_library_api|.
+    error->code = GetLastError();
+  }
+
+  return module;
+}
+
 }  // namespace
 
 std::string NativeLibraryLoadError::ToString() const {
@@ -62,12 +87,73 @@ NativeLibrary LoadNativeLibraryWithOptions(const FilePath& library_path,
 }
 
 NativeLibrary LoadNativeLibraryDynamically(const FilePath& library_path) {
-  typedef HMODULE (WINAPI* LoadLibraryFunction)(const wchar_t* file_name);
+  HMODULE load_library_module = nullptr;
+
+  enum LoadLibraryResult {
+    // LoadLibraryExW API/flags are available and the call succeeds.
+    LoadLibExW_Succeed = 0,
+    // LoadLibraryExW API/flags are availabe to use but the call fails, then
+    // LoadLibraryW is used and succeeds.
+    LoadLibExW_Fail_LoadLibW_Succeed,
+    // LoadLibraryExW API/flags are availabe to use but the call fails, then
+    // LoadLibraryW is used but fails as well.
+    LoadLibExW_Fail_LoadLibW_Fail,
+    // LoadLibraryExW API/flags are unavailabe to use, then LoadLibraryW is used
+    // and succeeds.
+    LoadLibExW_Unavailable_LoadLibW_Succeed,
+    // LoadLibraryExW API/flags are unavailabe to use, then LoadLibraryW is used
+    // but fails.
+    LoadLibExW_Unavailable_LoadLibW_Fail,
+    // Add new items before this one, always keep this one at the end.
+    END

[xhwang, 2017/03/14 00:14:32]

nit: We use UPPER_CASE style for enums
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Also, the names seem a bit verbose to me. Assuming you'll explain what these are
in histograms.xml, how about something like:

SUCCEED,
FAIL_AND_SUCCEED,
FAIL_AND_FAIL,
UNAVAILABLE_AND_SUCCEED,
UNAVAILABLE_AND_FAIL,

[chengx, 2017/03/14 02:44:40]

Done.

+  };
+
+  // This variable records the library loading result.
+  LoadLibraryResult load_library_status = LoadLibraryResult::LoadLibExW_Succeed;
+
+  // 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.
+  auto add_dll_dir_func = reinterpret_cast<AddDllDirectory>(
+      GetProcAddress(GetModuleHandle(L"kernel32.dll"), "AddDllDirectory"));
+
+  if (add_dll_dir_func) {
+    LoadLibraryFunctionEx load_library =
+        reinterpret_cast<LoadLibraryFunctionEx>(
+            GetProcAddress(GetModuleHandle(L"kernel32.dll"), "LoadLibraryExW"));
+    load_library_module =
+        LoadNativeLibraryHelperEx(library_path, load_library, nullptr);

[xhwang, 2017/03/14 00:14:32]

I guess we are mixing DLL load failure with LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR not
supported failure. But we really are only interested in
LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR failure, not the former...

If LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR is not supported, what kind of error code
will we get?

[chengx, 2017/03/14 02:44:40]

I think you are right. We have LoadLibraryW and LoadLibraryExW defined in the
Winbase.h already, so there is no need to GetProcAddress for them. So there will
be no DLL load failures now.

+  }

[xhwang, 2017/03/14 00:14:32]

nit: We (media) like to return early...

So here you can do something like:

if (load_library_module) {
  ReportResultToUMA(...);
  return load_library_module;
}

So the code below doesn't need to be a big if block...

Feel free to ignore if base/ or your team has different style.

[chengx, 2017/03/14 02:44:40]

Return early SGTM.

 
-  LoadLibraryFunction load_library = reinterpret_cast<LoadLibraryFunction>(
-      GetProcAddress(GetModuleHandle(L"kernel32.dll"), "LoadLibraryW"));
+  // If LoadLibraryExW API call fails, try LoadLibraryW API.
+  if (!load_library_module) {
+    LoadLibraryFunction load_library = reinterpret_cast<LoadLibraryFunction>(
+        GetProcAddress(GetModuleHandle(L"kernel32.dll"), "LoadLibraryW"));
+    load_library_module =
+        LoadNativeLibraryHelper(library_path, load_library, nullptr);
+    // Record the library loading result.
+    if (add_dll_dir_func) {
+      if (load_library_module)
+        load_library_status =
+            LoadLibraryResult::LoadLibExW_Fail_LoadLibW_Succeed;
+      else
+        load_library_status = LoadLibraryResult::LoadLibExW_Fail_LoadLibW_Fail;
+    } else if (load_library_module)
+      load_library_status =
+          LoadLibraryResult::LoadLibExW_Unavailable_LoadLibW_Succeed;
+    else
+      load_library_status =
+          LoadLibraryResult::LoadLibExW_Unavailable_LoadLibW_Fail;
+  }
 
-  return LoadNativeLibraryHelper(library_path, load_library, NULL);
+  UMA_HISTOGRAM_ENUMERATION("LibraryLoader.NativeLibraryDynamicallyLoad",
+                            load_library_status, LoadLibraryResult::END);
+  return load_library_module;
 }
 
 // static