Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(374)

Unified Diff: base/native_library_win.cc

Issue 2744043003: Use LoadLibraryExW if applicable in LoadNativeLibrary on Windows (Closed)
Patch Set: Better variable name. Created 3 years, 9 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « base/native_library.h ('k') | tools/metrics/histograms/histograms.xml » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: base/native_library_win.cc
diff --git a/base/native_library_win.cc b/base/native_library_win.cc
index 64c7380f173247c28b78e1f339b963d7374cf83f..68ff3d1f95d53a9fd289f4fc3268591247448dcd 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,108 @@
namespace base {
-typedef HMODULE (WINAPI* LoadLibraryFunction)(const wchar_t* file_name);
+using AddDllDirectory = HMODULE (*)(PCWSTR new_directory);
namespace {
+// This enum is used to back an UMA histogram, and should therefore be treated
+// as append-only.
+enum LoadLibraryResult {
+ // 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) {
+ UMA_HISTOGRAM_ENUMERATION("LibraryLoader.LoadNativeLibraryWindows", result,
+ LoadLibraryResult::END);
+}
+
+// A helper method to check if AddDllDirectory method is available, thus
+// LOAD_LIBRARY_SEARCH_* flags are available on systems.
+bool 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 the LoadNativeLibraryHelper
+ // method.
+ auto add_dll_dir_func = reinterpret_cast<AddDllDirectory>(
+ GetProcAddress(GetModuleHandle(L"kernel32.dll"), "AddDllDirectory"));
+ return !!add_dll_dir_func;
+}
+
+// A helper method to encode the library loading result to enum
+// LoadLibraryResult.
+LoadLibraryResult GetLoadLibraryResult(bool are_search_flags_available,
+ bool has_load_library_succeeded) {
+ LoadLibraryResult result;
+ if (are_search_flags_available) {
+ if (has_load_library_succeeded)
+ result = LoadLibraryResult::FAIL_AND_SUCCEED;
+ else
+ result = LoadLibraryResult::FAIL_AND_FAIL;
+ } else if (has_load_library_succeeded) {
+ result = LoadLibraryResult::UNAVAILABLE_AND_SUCCEED;
+ } else {
+ result = LoadLibraryResult::UNAVAILABLE_AND_FAIL;
+ }
+ return result;
+}
NativeLibrary LoadNativeLibraryHelper(const FilePath& library_path,
- LoadLibraryFunction load_library_api,
NativeLibraryLoadError* error) {
// LoadLibrary() opens the file off disk.
ThreadRestrictions::AssertIOAllowed();
+ HMODULE module = nullptr;
+
+ // This variable records the library loading result.
+ LoadLibraryResult load_library_result = LoadLibraryResult::SUCCEED;
+
+ bool are_search_flags_available = AreSearchFlagsAvailable();
+ if (are_search_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.
+ 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 (module) {
+ LogLibrarayLoadResultToUMA(load_library_result);
+ return 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.
+ // TODO(chengx): Currently, if LoadLibraryExW API call fails, LoadLibraryW is
+ // still tried. We should strictly prefer the LoadLibraryExW over the
+ // LoadLibraryW if LoadLibraryW is statistically showing no extra benefits. If
+ // UMA metric shows that FAIL_AND_FAIL is the primary failure mode and/or
+ // FAIL_AND_SUCCESS is close to zero, we should remove this fallback.
+ // (http://crbug.com/701944)
+
// 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 +129,21 @@ 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|.
+ module = ::LoadLibraryW(library_path.value().c_str());
+
+ // GetLastError() needs to be called immediately after LoadLibraryW call.
+ if (!module && error)
error->code = GetLastError();
- }
if (restore_directory)
SetCurrentDirectory(current_directory);
+ // Get the library loading result and log it to UMA.
+ LogLibrarayLoadResultToUMA(
+ GetLoadLibraryResult(are_search_flags_available, !!module));
+
return module;
}
-
} // namespace
std::string NativeLibraryLoadError::ToString() const {
@@ -58,16 +154,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
« no previous file with comments | « base/native_library.h ('k') | tools/metrics/histograms/histograms.xml » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698