Revert of Don't use ::GetFileVersionInfo() in CreateFileVersionInfoForModule()...

Revert of Don't use ::GetFileVersionInfo() in CreateFileVersionInfoForModule() (patchset #13 id:240001 of https://codereview.chromium.org/2046583002/ )

Reason for revert:
Sheriff reverting on suspicion of causing the Win x64 build to fail:
https://build.chromium.org/p/chromium/builders/Win%20x64/builds/1995

From the log:
FAILED: obj/base/base.file_version_info_win.obj
ninja -t msvc -e environment.x64 -- C:\b\build\slave\cache\cipd\goma/gomacc "C:\b\depot_tools\win_toolchain\vs_files\95ddda401ec5678f15eeed01d2bee08fcbc5ee97\VC\bin\amd64\cl.exe" /nologo /showIncludes /FC @obj\base\base.file_version_info_win.obj.rsp /c ..\..\base\file_version_info_win.cc /Foobj\base\base.file_version_info_win.obj /Fdobj\base\base.cc.pdb
c:\b\build\slave\win_x64\build\src\base\file_version_info_win.cc(81): error C2220: warning treated as error - no 'object' file generated
c:\b\build\slave\win_x64\build\src\base\file_version_info_win.cc(81): warning C4267: 'argument': conversion from 'size_t' to 'DWORD', possible loss of data
[8341/32887] CXX obj\base\threading\base.thread_collision_warner.obj

Original issue's description:
> Don't use ::GetFileVersionInfo() in CreateFileVersionInfoForModule()
>
> Currently, base::FileVersionInfo::CreateFileVersionInfoForModule()
> calls ::GetModuleFileName and ::GetFileVersionInfo, grabs the loader
> lock and potentially touches the disk to obtain the VS_VERSION_INFO
> of the module. This is gratuitous for a module that is already loaded.
>
> With this CL, base::FileVersionInfo::CreateFileVersionInfoForModule()
> uses base::win::GetResourceFromModule() to get the VS_VERSION_INFO
> resource from memory.
>
> BUG=609709
>
> Committed: https://crrev.com/f177a678814f97f0b35bd6aa678c1cf885ad1656
> Cr-Commit-Position: refs/heads/master@{#402549}

TBR=grt@chromium.org,thestig@chromium.org,fdoray@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=609709

Review-Url: https://codereview.chromium.org/2102363002
Cr-Commit-Position: refs/heads/master@{#402616}

base/BUILD.gn

@@ -1752,7 +1752,7 @@ test("base_unittests") {
     "deferred_sequenced_task_runner_unittest.cc",
     "environment_unittest.cc",
     "feature_list_unittest.cc",
-    "file_version_info_win_unittest.cc",
+    "file_version_info_unittest.cc",
     "files/dir_reader_posix_unittest.cc",
     "files/file_locking_unittest.cc",
     "files/file_path_unittest.cc",
@@ -2058,6 +2058,8 @@ test("base_unittests") {
   }
 
   if (is_linux) {
+    sources -= [ "file_version_info_unittest.cc" ]
+
     if (is_desktop_linux) {
       sources += [ "nix/xdg_util_unittest.cc" ]
     }

base/base.gyp

@@ -407,7 +407,7 @@
         'deferred_sequenced_task_runner_unittest.cc',
         'environment_unittest.cc',
         'feature_list_unittest.cc',
-        'file_version_info_win_unittest.cc',
+        'file_version_info_unittest.cc',
         'files/dir_reader_posix_unittest.cc',
         'files/file_locking_unittest.cc',
         'files/file_path_unittest.cc',
@@ -684,6 +684,9 @@
           'defines': [
             'USE_SYMBOLIZE',
           ],
+          'sources!': [
+            'file_version_info_unittest.cc',
+          ],
           'conditions': [
             [ 'desktop_linux==1', {
               'sources': [

base/file_version_info_win_unittest.cc → base/file_version_info_unittest.cc

@@ -2,26 +2,27 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "base/file_version_info_win.h"
-
-#include <windows.h>
+#include "base/file_version_info.h"
 
 #include <stddef.h>
 
 #include <memory>
 
-#include "base/file_version_info.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
-#include "base/memory/ptr_util.h"
 #include "base/path_service.h"
-#include "base/scoped_native_library.h"
+#include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+#if defined(OS_WIN)
+#include "base/file_version_info_win.h"
+#endif
+
 using base::FilePath;
 
 namespace {
 
+#if defined(OS_WIN)
 FilePath GetTestDataPath() {
   FilePath path;
   PathService::Get(base::DIR_SOURCE_ROOT, &path);
@@ -31,54 +32,12 @@ FilePath GetTestDataPath() {
   path = path.AppendASCII("file_version_info_unittest");
   return path;
 }
-
-class FileVersionInfoFactory {
- public:
-  explicit FileVersionInfoFactory(const FilePath& path) : path_(path) {}
-
-  std::unique_ptr<FileVersionInfo> Create() const {
-    return base::WrapUnique(FileVersionInfo::CreateFileVersionInfo(path_));
-  }
-
- private:
-  const FilePath path_;
-
-  DISALLOW_COPY_AND_ASSIGN(FileVersionInfoFactory);
-};
-
-class FileVersionInfoForModuleFactory {
- public:
-  explicit FileVersionInfoForModuleFactory(const FilePath& path)
-      // Load the library with LOAD_LIBRARY_AS_IMAGE_RESOURCE since it shouldn't
-      // be executed.
-      : library_(::LoadLibraryEx(path.value().c_str(),
-                                 nullptr,
-                                 LOAD_LIBRARY_AS_IMAGE_RESOURCE)) {
-    EXPECT_TRUE(library_.is_valid());
-  }
-
-  std::unique_ptr<FileVersionInfo> Create() const {
-    return base::WrapUnique(
-        FileVersionInfo::CreateFileVersionInfoForModule(library_.get()));
-  }
-
- private:
-  const base::ScopedNativeLibrary library_;
-
-  DISALLOW_COPY_AND_ASSIGN(FileVersionInfoForModuleFactory);
-};
-
-template <typename T>
-class FileVersionInfoTest : public testing::Test {};
-
-using FileVersionInfoFactories =
-    ::testing::Types<FileVersionInfoFactory, FileVersionInfoForModuleFactory>;
+#endif
 
 }  // namespace
 
-TYPED_TEST_CASE(FileVersionInfoTest, FileVersionInfoFactories);
-
-TYPED_TEST(FileVersionInfoTest, HardCodedProperties) {
+#if defined(OS_WIN)
+TEST(FileVersionInfoTest, HardCodedProperties) {
   const wchar_t kDLLName[] = {L"FileVersionInfoTest1.dll"};
 
   const wchar_t* const kExpectedValues[15] = {
@@ -103,9 +62,8 @@ TYPED_TEST(FileVersionInfoTest, HardCodedProperties) {
   FilePath dll_path = GetTestDataPath();
   dll_path = dll_path.Append(kDLLName);
 
-  TypeParam factory(dll_path);
-  std::unique_ptr<FileVersionInfo> version_info(factory.Create());
-  ASSERT_TRUE(version_info);
+  std::unique_ptr<FileVersionInfo> version_info(
+      FileVersionInfo::CreateFileVersionInfo(dll_path));
 
   int j = 0;
   EXPECT_EQ(kExpectedValues[j++], version_info->company_name());
@@ -124,52 +82,62 @@ TYPED_TEST(FileVersionInfoTest, HardCodedProperties) {
   EXPECT_EQ(kExpectedValues[j++], version_info->legal_trademarks());
   EXPECT_EQ(kExpectedValues[j++], version_info->last_change());
 }
+#endif
 
-TYPED_TEST(FileVersionInfoTest, IsOfficialBuild) {
-  constexpr struct {
-    const wchar_t* const dll_name;
-    const bool is_official_build;
-  } kTestItems[]{
-      {L"FileVersionInfoTest1.dll", true}, {L"FileVersionInfoTest2.dll", false},
+#if defined(OS_WIN)
+TEST(FileVersionInfoTest, IsOfficialBuild) {
+  const wchar_t* kDLLNames[] = {
+    L"FileVersionInfoTest1.dll",
+    L"FileVersionInfoTest2.dll"
   };
 
-  for (const auto& test_item : kTestItems) {
-    const FilePath dll_path = GetTestDataPath().Append(test_item.dll_name);
+  const bool kExpected[] = {
+    true,
+    false,
+  };
+
+  // Test consistency check.
+  ASSERT_EQ(arraysize(kDLLNames), arraysize(kExpected));
+
+  for (size_t i = 0; i < arraysize(kDLLNames); ++i) {
+    FilePath dll_path = GetTestDataPath();
+    dll_path = dll_path.Append(kDLLNames[i]);
 
-    TypeParam factory(dll_path);
-    std::unique_ptr<FileVersionInfo> version_info(factory.Create());
-    ASSERT_TRUE(version_info);
+    std::unique_ptr<FileVersionInfo> version_info(
+        FileVersionInfo::CreateFileVersionInfo(dll_path));
 
-    EXPECT_EQ(test_item.is_official_build, version_info->is_official_build());
+    EXPECT_EQ(kExpected[i], version_info->is_official_build());
   }
 }
+#endif
 
-TYPED_TEST(FileVersionInfoTest, CustomProperties) {
+#if defined(OS_WIN)
+TEST(FileVersionInfoTest, CustomProperties) {
   FilePath dll_path = GetTestDataPath();
   dll_path = dll_path.AppendASCII("FileVersionInfoTest1.dll");
 
-  TypeParam factory(dll_path);
-  std::unique_ptr<FileVersionInfo> version_info(factory.Create());
-  ASSERT_TRUE(version_info);
+  std::unique_ptr<FileVersionInfo> version_info(
+      FileVersionInfo::CreateFileVersionInfo(dll_path));
 
   // Test few existing properties.
   std::wstring str;
   FileVersionInfoWin* version_info_win =
       static_cast<FileVersionInfoWin*>(version_info.get());
-  EXPECT_TRUE(version_info_win->GetValue(L"Custom prop 1", &str));
+  EXPECT_TRUE(version_info_win->GetValue(L"Custom prop 1",  &str));
   EXPECT_EQ(L"Un", str);
   EXPECT_EQ(L"Un", version_info_win->GetStringValue(L"Custom prop 1"));
 
-  EXPECT_TRUE(version_info_win->GetValue(L"Custom prop 2", &str));
+  EXPECT_TRUE(version_info_win->GetValue(L"Custom prop 2",  &str));
   EXPECT_EQ(L"Deux", str);
   EXPECT_EQ(L"Deux", version_info_win->GetStringValue(L"Custom prop 2"));
 
-  EXPECT_TRUE(version_info_win->GetValue(L"Custom prop 3", &str));
+  EXPECT_TRUE(version_info_win->GetValue(L"Custom prop 3",  &str));
   EXPECT_EQ(L"1600 Amphitheatre Parkway Mountain View, CA 94043", str);
   EXPECT_EQ(L"1600 Amphitheatre Parkway Mountain View, CA 94043",
             version_info_win->GetStringValue(L"Custom prop 3"));
 
   // Test an non-existing property.
-  EXPECT_FALSE(version_info_win->GetValue(L"Unknown property", &str));
+  EXPECT_FALSE(version_info_win->GetValue(L"Unknown property",  &str));
   EXPECT_EQ(L"", version_info_win->GetStringValue(L"Unknown property"));
 }
+#endif

base/file_version_info_win.cc

@@ -6,63 +6,48 @@
 
 #include <windows.h>
 #include <stddef.h>
+#include <stdint.h>
 
+#include "base/file_version_info.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
+#include "base/macros.h"
 #include "base/threading/thread_restrictions.h"
-#include "base/win/resource_util.h"
 
 using base::FilePath;
 
-namespace {
-
-struct LanguageAndCodePage {
-  WORD language;
-  WORD code_page;
-};
-
-// Returns the \\VarFileInfo\\Translation value extracted from the
-// VS_VERSION_INFO resource in |data|.
-LanguageAndCodePage* GetTranslate(const void* data) {
-  LanguageAndCodePage* translate = nullptr;
-  UINT length;
-  if (::VerQueryValue(data, L"\\VarFileInfo\\Translation",
-                      reinterpret_cast<void**>(&translate), &length)) {
-    return translate;
-  }
-  return nullptr;
+FileVersionInfoWin::FileVersionInfoWin(void* data,
+                                       WORD language,
+                                       WORD code_page)
+    : language_(language), code_page_(code_page) {
+  base::ThreadRestrictions::AssertIOAllowed();
+  data_.reset((char*) data);
+  fixed_file_info_ = NULL;
+  UINT size;
+  ::VerQueryValue(data_.get(), L"\\", (LPVOID*)&fixed_file_info_, &size);
 }
 
-VS_FIXEDFILEINFO* GetVsFixedFileInfo(const void* data) {
-  VS_FIXEDFILEINFO* fixed_file_info = nullptr;
-  UINT length;
-  if (::VerQueryValue(data, L"\\", reinterpret_cast<void**>(&fixed_file_info),
-                      &length)) {
-    return fixed_file_info;
-  }
-  return nullptr;
+FileVersionInfoWin::~FileVersionInfoWin() {
+  DCHECK(data_.get());
 }
 
-}  // namespace
-
-FileVersionInfoWin::~FileVersionInfoWin() = default;
+typedef struct {
+  WORD language;
+  WORD code_page;
+} LanguageAndCodePage;
 
 // static
 FileVersionInfo* FileVersionInfo::CreateFileVersionInfoForModule(
     HMODULE module) {
-  void* data;
-  size_t version_info_length;
-  const bool has_version_resource = base::win::GetResourceFromModule(
-      module, VS_VERSION_INFO, RT_VERSION, &data, &version_info_length);
-  if (!has_version_resource)
-    return nullptr;
-
-  const LanguageAndCodePage* translate = GetTranslate(data);
-  if (!translate)
-    return nullptr;
+  // Note that the use of MAX_PATH is basically in line with what we do for
+  // all registered paths (PathProviderWin).
+  wchar_t system_buffer[MAX_PATH];
+  system_buffer[0] = 0;
+  if (!GetModuleFileName(module, system_buffer, MAX_PATH))
+    return NULL;
 
-  return new FileVersionInfoWin(data, translate->language,
-                                translate->code_page);
+  FilePath app_path(system_buffer);
+  return CreateFileVersionInfo(app_path);
 }
 
 // static
@@ -74,19 +59,30 @@ FileVersionInfo* FileVersionInfo::CreateFileVersionInfo(
   const wchar_t* path = file_path.value().c_str();
   DWORD length = ::GetFileVersionInfoSize(path, &dummy);
   if (length == 0)
-    return nullptr;
+    return NULL;
 
-  std::vector<uint8_t> data(length, 0);
+  void* data = calloc(length, 1);
+  if (!data)
+    return NULL;
 
-  if (!::GetFileVersionInfo(path, dummy, data.size(), data.data()))
-    return nullptr;
+  if (!::GetFileVersionInfo(path, dummy, length, data)) {
+    free(data);
+    return NULL;
+  }
+
+  LanguageAndCodePage* translate = NULL;
+  uint32_t page_count;
+  BOOL query_result = VerQueryValue(data, L"\\VarFileInfo\\Translation",
+                                   (void**) &translate, &page_count);
 
-  const LanguageAndCodePage* translate = GetTranslate(data.data());
-  if (!translate)
-    return nullptr;
+  if (query_result && translate) {
+    return new FileVersionInfoWin(data, translate->language,
+                                  translate->code_page);
 
-  return new FileVersionInfoWin(std::move(data), translate->language,
-                                translate->code_page);
+  } else {
+    free(data);
+    return NULL;
+  }
 }
 
 base::string16 FileVersionInfoWin::company_name() {
@@ -179,7 +175,7 @@ bool FileVersionInfoWin::GetValue(const wchar_t* name,
                  L"\\StringFileInfo\\%04x%04x\\%ls", language, code_page, name);
     LPVOID value = NULL;
     uint32_t size;
-    BOOL r = ::VerQueryValue(data_, sub_block, &value, &size);
+    BOOL r = ::VerQueryValue(data_.get(), sub_block, &value, &size);
     if (r && value) {
       value_str->assign(static_cast<wchar_t*>(value));
       return true;
@@ -195,24 +191,3 @@ std::wstring FileVersionInfoWin::GetStringValue(const wchar_t* name) {
   else
     return L"";
 }
-
-FileVersionInfoWin::FileVersionInfoWin(std::vector<uint8_t>&& data,
-                                       WORD language,
-                                       WORD code_page)
-    : owned_data_(std::move(data)),
-      data_(owned_data_.data()),
-      language_(language),
-      code_page_(code_page),
-      fixed_file_info_(GetVsFixedFileInfo(data_)) {
-  DCHECK(!owned_data_.empty());
-}
-
-FileVersionInfoWin::FileVersionInfoWin(void* data,
-                                       WORD language,
-                                       WORD code_page)
-    : data_(data),
-      language_(language),
-      code_page_(code_page),
-      fixed_file_info_(GetVsFixedFileInfo(data)) {
-  DCHECK(data_);
-}

base/file_version_info_win.h

@@ -5,23 +5,20 @@
 #ifndef BASE_FILE_VERSION_INFO_WIN_H_
 #define BASE_FILE_VERSION_INFO_WIN_H_
 
-#include <windows.h>
-
-#include <stdint.h>
-
 #include <memory>
 #include <string>
-#include <vector>
 
 #include "base/base_export.h"
 #include "base/file_version_info.h"
 #include "base/macros.h"
+#include "base/memory/free_deleter.h"
 
 struct tagVS_FIXEDFILEINFO;
 typedef tagVS_FIXEDFILEINFO VS_FIXEDFILEINFO;
 
 class BASE_EXPORT FileVersionInfoWin : public FileVersionInfo {
  public:
+  FileVersionInfoWin(void* data, WORD language, WORD code_page);
   ~FileVersionInfoWin() override;
 
   // Accessors to the different version properties.
@@ -51,25 +48,14 @@ class BASE_EXPORT FileVersionInfoWin : public FileVersionInfo {
   std::wstring GetStringValue(const wchar_t* name);
 
   // Get the fixed file info if it exists. Otherwise NULL
-  const VS_FIXEDFILEINFO* fixed_file_info() const { return fixed_file_info_; }
+  VS_FIXEDFILEINFO* fixed_file_info() { return fixed_file_info_; }
 
  private:
-  friend FileVersionInfo;
-
-  // |data| is a VS_VERSION_INFO resource. |language| and |code_page| are
-  // extracted from the \VarFileInfo\Translation value of |data|.
-  FileVersionInfoWin(std::vector<uint8_t>&& data,
-                     WORD language,
-                     WORD code_page);
-  FileVersionInfoWin(void* data, WORD language, WORD code_page);
-
-  const std::vector<uint8_t> owned_data_;
-  const void* const data_;
-  const WORD language_;
-  const WORD code_page_;
-
-  // This is a pointer into |data_| if it exists. Otherwise nullptr.
-  const VS_FIXEDFILEINFO* const fixed_file_info_;
+  std::unique_ptr<char, base::FreeDeleter> data_;
+  WORD language_;
+  WORD code_page_;
+  // This is a pointer into the data_ if it exists. Otherwise NULL.
+  VS_FIXEDFILEINFO* fixed_file_info_;
 
   DISALLOW_COPY_AND_ASSIGN(FileVersionInfoWin);
 };

chrome/browser/win/enumerate_modules_model.cc

@@ -15,7 +15,7 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/environment.h"
-#include "base/file_version_info.h"
+#include "base/file_version_info_win.h"
 #include "base/files/file_path.h"
 #include "base/i18n/case_conversion.h"
 #include "base/macros.h"
@@ -630,10 +630,16 @@ void ModuleEnumerator::PopulateModuleInformation(Module* module) {
   module->recommended_action = NONE;
   std::unique_ptr<FileVersionInfo> version_info(
       FileVersionInfo::CreateFileVersionInfo(base::FilePath(module->location)));
-  if (version_info) {
-    module->description = version_info->file_description();
-    module->version = version_info->file_version();
-    module->product_name = version_info->product_name();
+  if (version_info.get()) {
+    FileVersionInfoWin* version_info_win =
+        static_cast<FileVersionInfoWin*>(version_info.get());
+
+    VS_FIXEDFILEINFO* fixed_file_info = version_info_win->fixed_file_info();
+    if (fixed_file_info) {
+      module->description = version_info_win->file_description();
+      module->version = version_info_win->file_version();
+      module->product_name = version_info_win->product_name();
+    }
   }
 }