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..4171a2a5ea9de2afe6132e8c19c8a3bef13bd64a 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* 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);

[xhwang, 2017/03/13 20:47:09]

nit: line 19 has a space after HMODULE. Is the new format recommended by
clang-format? If so, fix 19 as well?

[chengx, 2017/03/13 22:20:22]

Sorry, it is typo. Now it is fixed.

 
 namespace {
-
+// This helper method uses LoadLibrary() WinAPI.
 NativeLibrary LoadNativeLibraryHelper(const FilePath& library_path,
                                       LoadLibraryFunction load_library_api,
                                       NativeLibraryLoadError* error) {
@@ -48,6 +53,24 @@ 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();
+
+  HMODULE module = (*load_library_api)(
+      library_path.value().c_str(), NULL,

[xhwang, 2017/03/13 20:47:08]

here and everywhere, s/NULL/nullptr?

[chengx, 2017/03/13 22:20:22]

Done.

+      LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR | LOAD_LIBRARY_SEARCH_DEFAULT_DIRS);

[xhwang, 2017/03/13 20:47:09]

nit: Add a comment why we need LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR, similar to the
comments on line 32-33.

[chengx, 2017/03/13 22:20:22]

Done.

+  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 +85,57 @@ 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 = NULL;
+
+  enum LoadLibraryResult {

[xhwang, 2017/03/13 20:47:09]

nit: enum class?

[xhwang, 2017/03/13 20:47:09]

Add a comment that combinations of these are used to report UMA, so any update
should involve histogram update.

[chengx, 2017/03/13 22:20:22]

Both should work. Using enum directly rather than enum class avoid applying
static_cast load_library_status to int in UMA_HISTOGRAM_ENUMERATION call, IMHO. 

It seems enum and enum class are both heavily used in the code base.

[chengx, 2017/03/13 22:20:22]

The enum is updated, so maybe this comment is no longer needed?

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

We do recommend enum class in new code.

How about typed enum class like this?

enum class Foo : uint32_t { ... }

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

I understand enum class if preferred over enum. However, if I use "enum class
Foo" or even "enum class Foo : uint32_t", it doesn't allow me to static_cast to
uint32_t, which is needed by UMA_HISTOGRAM_ENUMERATION. It may be a compiler
issue that I have not figured out yet. I do have a search of Chromium code base,
and seems that all (I have not found a counter example yet) the enums that are
used for UMA_HISTOGRAM_ENUMERATION are simply "enum" rather than "enum class".

+    LoadLibExW_EXIST = 0,
+    LoadLibExW_NONE = 1 << 0,
+    LoadLibExW_FAIL = 1 << 1,
+    LoadLibW_FAIL = 1 << 2,

[xhwang, 2017/03/13 20:47:09]

nit: You are using 3 bit positions (4 enum values) to represent 5 possible
states, which seems to be a bit overkill and confusing. Also, it's not obvious
how to map the valid combinations into the entries in LoadLibraryResultCategory
in the histogram.xml.

Can we just have 5 enums here (same as what you have in
LoadLibraryResultCategory), and report them directly in the impl?

[chengx, 2017/03/13 22:20:22]

Agreed and fixed. I was addicted to playing with the bits thing too much.

+    // This value is beyond the sum of all bit fields above and
+    // should remain last (shifted by one more than the last value)
+    END = 1 << 3
+  };
+
+  // This variable records the library loading result.
+  uint32_t load_library_status = LoadLibraryResult::LoadLibExW_EXIST;
+
+  // 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

[xhwang, 2017/03/13 20:47:09]

The link seems to be broken for me...

[chengx, 2017/03/13 22:20:22]

Did you copy the "aspx" in the following line?

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

I did and I checked... but now I tried it again and it's working. Sorry about
the noise.

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

Cool!

+  // 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"));

[xhwang, 2017/03/13 20:47:08]

nit: Can we move this into a helper function that returns a boolean? That'll
help keep this function clean.

[chengx, 2017/03/13 22:20:22]

Now since enum is updated, I need this add_dll_dir_func variable to help assign
value to load_library_status. If moving this to a helper function which returns
a boolean, this method needs to be called twice though.

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

You can always store !!add_dll_dir_func into a boolean?

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

accepted.

+
+  if (add_dll_dir_func) {
+    LoadLibraryFunctionEx load_library =
+        reinterpret_cast<LoadLibraryFunctionEx>(
+            GetProcAddress(GetModuleHandle(L"kernel32.dll"), "LoadLibraryExW"));

[xhwang, 2017/03/13 20:47:09]

nit for discussion: I don't know what's the value of doing this here instead of
doing it in LoadNativeLibraryHelper, which seems more natural to me.

[chengx, 2017/03/13 22:20:22]

If removing LoadNativeLibraryDynamically and then packing everything into
LoadNativeLibraryWithOptions, I think doing this in LoadNativeLibraryHelper is
obviously better. However, it seems that LoadNativeLibraryWithOptions wants to
use LoadLibraryW () via the import table directly, so I was hesitated to modify
LoadNativeLibraryHelper whose 2nd parameter is the function pointer. Again, this
goes back to the question if we want to make LoadNativeLibraryWithOptions call
APIs dynamically.

+    load_library_module =
+        LoadNativeLibraryHelperEx(library_path, load_library, NULL);

[xhwang, 2017/03/13 20:47:09]

See above that we need to update LoadNativeLibraryWithOptions, where the |error|
will be non-null. That's actually how I found that the error code is 126.

[chengx, 2017/03/13 22:20:22]

Acknowledged.

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

Alright, LoadNativeLibraryWithOptions updated.

+    if (!load_library_module)
+      load_library_status |= LoadLibraryResult::LoadLibExW_FAIL;
+  } else {
+    load_library_status |= LoadLibraryResult::LoadLibExW_NONE;
+  }
 
-  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"));

[xhwang, 2017/03/13 20:47:08]

nit: ditto about the possibility of moving this into LoadNativeLibraryHelper.

[chengx, 2017/03/13 22:20:22]

Acknowledged.

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

Yep, moved into LoadNativeLibraryHelper in the new patch set.

+    load_library_module =
+        LoadNativeLibraryHelper(library_path, load_library, NULL);
+    if (!load_library_module)
+      load_library_status |= LoadLibraryResult::LoadLibW_FAIL;
+  }
 
-  return LoadNativeLibraryHelper(library_path, load_library, NULL);
+  UMA_HISTOGRAM_ENUMERATION("LibraryLoader.NativeLibraryDynamicallyLoad",
+                            load_library_status, LoadLibraryResult::END);
+  return load_library_module;
 }
 
 // static