|
|
DescriptionDon'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}
Patch Set 1 #
Total comments: 3
Patch Set 2 : change base::FileVersionInfo::CreateFileVersionInfoForModule() #Patch Set 3 : self-review #
Total comments: 11
Patch Set 4 : CR grt #6 #Patch Set 5 : self-review #
Total comments: 8
Patch Set 6 : CR grt #8 #Patch Set 7 : self-review #Patch Set 8 : self-review #
Total comments: 2
Patch Set 9 : CR grt #10 #
Total comments: 10
Patch Set 10 : CR thestig #20 #Patch Set 11 : self-review #
Total comments: 2
Patch Set 12 : nit #Patch Set 13 : rebase #
Messages
Total messages: 40 (15 generated)
fdoray@chromium.org changed reviewers: + grt@chromium.org
Can you review this CL? Thanks. https://codereview.chromium.org/2046583002/diff/1/chrome/installer/util/modul... File chrome/installer/util/module_util_win.cc (right): https://codereview.chromium.org/2046583002/diff/1/chrome/installer/util/modul... chrome/installer/util/module_util_win.cc:26: // This structure is not declared anywhere in the SDK, but is documented at I copied this from third_party/crashpad/crashpad/snapshot/win/pe_image_reader.cc. Would it be appropriate to declare in in base/win/* so that it can be reused here and in crashpad?
https://codereview.chromium.org/2046583002/diff/1/chrome/installer/util/modul... File chrome/installer/util/module_util_win.cc (right): https://codereview.chromium.org/2046583002/diff/1/chrome/installer/util/modul... chrome/installer/util/module_util_win.cc:26: // This structure is not declared anywhere in the SDK, but is documented at On 2016/06/06 14:40:58, fdoray wrote: > I copied this from > third_party/crashpad/crashpad/snapshot/win/pe_image_reader.cc. Would it be > appropriate to declare in in base/win/* so that it can be reused here and in > crashpad? Many current uses of CreateFileVersionInfoForModule and CreateFileVersionInfo look to me like they could be serviced with this impl or something like it (some require other fields from VS_FIXEDFILEINFO). Please survey them all to see how practical it is to make a single fn in base/win that would serve most/all callers. Thanks.
Description was changed from ========== Read executable version from VS_VERSION_INFO resource. When a chrome.exe process is launched, it needs to know its version to determine which DLL to load. Previously, this version was obtained with base::FileVersionInfo::CreateFileVersionInfoForModule() (involves ::GetModuleFileName, ::GetFileVersionInfo, ::VerQueryValue and grabbing the loader lock). This sometimes failed with Windows error ERROR_RESOURCE_TYPE_NOT_FOUND or ERROR_FILE_NOT_FOUND. With this CL, the version is obtained by reading the VS_VERSION_INFO resource of the current module directly. BUG=609709 ========== to ========== 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 ==========
PTAnL https://codereview.chromium.org/2046583002/diff/1/chrome/installer/util/modul... File chrome/installer/util/module_util_win.cc (right): https://codereview.chromium.org/2046583002/diff/1/chrome/installer/util/modul... chrome/installer/util/module_util_win.cc:26: // This structure is not declared anywhere in the SDK, but is documented at On 2016/06/06 15:42:56, grt (slow) wrote: > On 2016/06/06 14:40:58, fdoray wrote: > > I copied this from > > third_party/crashpad/crashpad/snapshot/win/pe_image_reader.cc. Would it be > > appropriate to declare in in base/win/* so that it can be reused here and in > > crashpad? > > Many current uses of CreateFileVersionInfoForModule and CreateFileVersionInfo > look to me like they could be serviced with this impl or something like it (some > require other fields from VS_FIXEDFILEINFO). Please survey them all to see how > practical it is to make a single fn in base/win that would serve most/all > callers. Thanks. Done. The documentation of ::VerQueryValue() [1] says that its first argument must come from ::GetFileVersionInfo. However, these pages [2] imply that it is also valid to use the VERSIONINFO resource as argument. And unit tests don't fail :) [1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms647464(v=vs.85).aspx [2] https://msdn.microsoft.com/en-us/library/windows/desktop/ms646981(v=vs.85).aspx https://msdn.microsoft.com/en-us/library/windows/desktop/aa381058(v=vs.85).aspx
cool! https://codereview.chromium.org/2046583002/diff/40001/base/file_version_info_... File base/file_version_info_win.cc (right): https://codereview.chromium.org/2046583002/diff/40001/base/file_version_info_... base/file_version_info_win.cc:38: FileVersionInfoWin::FileVersionInfoWin(std::vector<uint8_t> data, nit: move ctors down in the file so that the order of definitions here matches the order of declarations in the .h file. https://codereview.chromium.org/2046583002/diff/40001/base/file_version_info_... base/file_version_info_win.cc:41: : FileVersionInfoWin(data.data(), language, code_page) { , owned_data_(std::move(data)) { https://codereview.chromium.org/2046583002/diff/40001/base/file_version_info_... File base/file_version_info_win.h (right): https://codereview.chromium.org/2046583002/diff/40001/base/file_version_info_... base/file_version_info_win.h:63: std::vector<uint8_t> owned_data_; const https://codereview.chromium.org/2046583002/diff/40001/base/file_version_info_... File base/file_version_info_win_unittest.cc (right): https://codereview.chromium.org/2046583002/diff/40001/base/file_version_info_... base/file_version_info_win_unittest.cc:50: : library_(::LoadLibraryEx(path.value().c_str(), please add a brief comment explaining why base::ScopedNativeLibrary isn't appropriate for this https://codereview.chromium.org/2046583002/diff/40001/base/file_version_info_... base/file_version_info_win_unittest.cc:125: const wchar_t* kDLLNames[] = {L"FileVersionInfoTest1.dll", nit: const wchar_t* const
PTAnL https://codereview.chromium.org/2046583002/diff/40001/base/file_version_info_... File base/file_version_info_win.cc (right): https://codereview.chromium.org/2046583002/diff/40001/base/file_version_info_... base/file_version_info_win.cc:38: FileVersionInfoWin::FileVersionInfoWin(std::vector<uint8_t> data, On 2016/06/17 20:34:59, grt (slow) wrote: > nit: move ctors down in the file so that the order of definitions here matches > the order of declarations in the .h file. Done. https://codereview.chromium.org/2046583002/diff/40001/base/file_version_info_... base/file_version_info_win.cc:41: : FileVersionInfoWin(data.data(), language, code_page) { On 2016/06/17 20:34:59, grt (slow) wrote: > , owned_data_(std::move(data)) { Doesn't work error C3511: 'FileVersionInfoWin': a call to a delegating constructor shall be the only member-initializer Do you prefer the new version without a delegating constructor? https://codereview.chromium.org/2046583002/diff/40001/base/file_version_info_... File base/file_version_info_win.h (right): https://codereview.chromium.org/2046583002/diff/40001/base/file_version_info_... base/file_version_info_win.h:63: std::vector<uint8_t> owned_data_; On 2016/06/17 20:34:59, grt (slow) wrote: > const Doesn't work https://codereview.chromium.org/2046583002/diff/40001/base/file_version_info_... https://codereview.chromium.org/2046583002/diff/40001/base/file_version_info_... File base/file_version_info_win_unittest.cc (right): https://codereview.chromium.org/2046583002/diff/40001/base/file_version_info_... base/file_version_info_win_unittest.cc:50: : library_(::LoadLibraryEx(path.value().c_str(), On 2016/06/17 20:35:00, grt (slow) wrote: > please add a brief comment explaining why base::ScopedNativeLibrary isn't > appropriate for this ScopedNativeLibrary can be used for this. We just can't use the FilePath constructor because we need to open the library with LOAD_LIBRARY_AS_IMAGE_RESOURCE. https://codereview.chromium.org/2046583002/diff/40001/base/file_version_info_... base/file_version_info_win_unittest.cc:125: const wchar_t* kDLLNames[] = {L"FileVersionInfoTest1.dll", On 2016/06/17 20:34:59, grt (slow) wrote: > nit: const wchar_t* const Done.
https://codereview.chromium.org/2046583002/diff/40001/base/file_version_info_... File base/file_version_info_win.cc (right): https://codereview.chromium.org/2046583002/diff/40001/base/file_version_info_... base/file_version_info_win.cc:41: : FileVersionInfoWin(data.data(), language, code_page) { On 2016/06/20 14:06:41, fdoray wrote: > On 2016/06/17 20:34:59, grt (slow) wrote: > > , owned_data_(std::move(data)) { > > Doesn't work > error C3511: 'FileVersionInfoWin': a call to a delegating constructor shall be > the only member-initializer TIL... > Do you prefer the new version without a delegating constructor? I don't have a strong preference. https://codereview.chromium.org/2046583002/diff/80001/base/file_version_info_... File base/file_version_info_win.cc (right): https://codereview.chromium.org/2046583002/diff/80001/base/file_version_info_... base/file_version_info_win.cc:29: ::VerQueryValue(data, L"\\VarFileInfo\\Translation", (void**)&translate, does msdn state that lplpBuffer won't be modified on error? if not, please stick with the conservative approach. https://codereview.chromium.org/2046583002/diff/80001/base/file_version_info_... base/file_version_info_win.cc:198: owned_data_(std::move(data)), while i *think* that this works, i can't convince myself by reading the spec that the value returned by data.data() is guaranteed to be valid after the move. please swap these to so that the code doesn't depend on this: : owned_data_(std::move(data)), data_(owned_data_.data()), (swap them in the class definition as well) https://codereview.chromium.org/2046583002/diff/80001/base/file_version_info_... File base/file_version_info_win_unittest.cc (right): https://codereview.chromium.org/2046583002/diff/80001/base/file_version_info_... base/file_version_info_win_unittest.cc:54: NULL, nullptr ALL the things! https://codereview.chromium.org/2046583002/diff/80001/base/file_version_info_... base/file_version_info_win_unittest.cc:59: FileVersionInfo* Create() const { nit: return std::unique_ptr so that the ownership transfer is enforced by the compiler
PTAnL https://codereview.chromium.org/2046583002/diff/80001/base/file_version_info_... File base/file_version_info_win.cc (right): https://codereview.chromium.org/2046583002/diff/80001/base/file_version_info_... base/file_version_info_win.cc:29: ::VerQueryValue(data, L"\\VarFileInfo\\Translation", (void**)&translate, On 2016/06/20 20:05:14, grt (no reviews Jun 23-Jul 18) wrote: > does msdn state that lplpBuffer won't be modified on error? if not, please stick > with the conservative approach. Done. https://codereview.chromium.org/2046583002/diff/80001/base/file_version_info_... base/file_version_info_win.cc:198: owned_data_(std::move(data)), On 2016/06/20 20:05:14, grt (no reviews Jun 23-Jul 18) wrote: > while i *think* that this works, i can't convince myself by reading the spec > that the value returned by data.data() is guaranteed to be valid after the move. > please swap these to so that the code doesn't depend on this: > : owned_data_(std::move(data)), > data_(owned_data_.data()), > (swap them in the class definition as well) Done. https://codereview.chromium.org/2046583002/diff/80001/base/file_version_info_... File base/file_version_info_win_unittest.cc (right): https://codereview.chromium.org/2046583002/diff/80001/base/file_version_info_... base/file_version_info_win_unittest.cc:54: NULL, On 2016/06/20 20:05:14, grt (no reviews Jun 23-Jul 18) wrote: > nullptr ALL the things! Done. https://codereview.chromium.org/2046583002/diff/80001/base/file_version_info_... base/file_version_info_win_unittest.cc:59: FileVersionInfo* Create() const { On 2016/06/20 20:05:14, grt (no reviews Jun 23-Jul 18) wrote: > nit: return std::unique_ptr so that the ownership transfer is enforced by the > compiler Done.
lgtm https://codereview.chromium.org/2046583002/diff/140001/base/file_version_info... File base/file_version_info_win.cc (right): https://codereview.chromium.org/2046583002/diff/140001/base/file_version_info... base/file_version_info_win.cc:206: fixed_file_info_(GetVsFixedFileInfo(data_)) {} should this DCHECK(!owned_data_.empty());?
https://codereview.chromium.org/2046583002/diff/140001/base/file_version_info... File base/file_version_info_win.cc (right): https://codereview.chromium.org/2046583002/diff/140001/base/file_version_info... base/file_version_info_win.cc:206: fixed_file_info_(GetVsFixedFileInfo(data_)) {} On 2016/06/22 14:09:43, grt (no reviews Jun 23-Jul 11) wrote: > should this DCHECK(!owned_data_.empty());? Done.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org Link to the patchset: https://codereview.chromium.org/2046583002/#ps160001 (title: "CR grt #10")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2046583002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
fdoray@chromium.org changed reviewers: + thestig@chromium.org
Can you review this CL? Thanks.
thestig@: Can you review this CL? Thanks.
https://codereview.chromium.org/2046583002/diff/160001/base/file_version_info... File base/file_version_info_win.cc (right): https://codereview.chromium.org/2046583002/diff/160001/base/file_version_info... base/file_version_info_win.cc:203: data_(owned_data_.data()), What happens when |owned_data_| is empty? Or does that never happen? https://codereview.chromium.org/2046583002/diff/160001/base/file_version_info... File base/file_version_info_win.h (right): https://codereview.chromium.org/2046583002/diff/160001/base/file_version_info... base/file_version_info_win.h:61: FileVersionInfoWin(std::vector<uint8_t> data, WORD language, WORD code_page); std::vector<uint8_t>&& data ? https://codereview.chromium.org/2046583002/diff/160001/base/file_version_info... base/file_version_info_win.h:69: // This is a pointer into the data_ if it exists. Otherwise nullptr. |data_| https://codereview.chromium.org/2046583002/diff/160001/base/file_version_info... File base/file_version_info_win_unittest.cc (right): https://codereview.chromium.org/2046583002/diff/160001/base/file_version_info... base/file_version_info_win_unittest.cc:37: FileVersionInfoFactory(const FilePath& path) : path_(path) {} explicit, ditto below https://codereview.chromium.org/2046583002/diff/160001/base/file_version_info... base/file_version_info_win_unittest.cc:136: static_assert(arraysize(kDLLNames) == arraysize(kExpectedIsOfficialBuild), I realize this is pre-existing, but it's a bit wonky, just like having an unit test where everything is ifdef'd for OS_WIN. There's lots of examples in base/ where one creates a struct with members for the test input and result. Since we are changing this, can we do that instead?
thestig@: PTAnL https://codereview.chromium.org/2046583002/diff/160001/base/file_version_info... File base/file_version_info_win.cc (right): https://codereview.chromium.org/2046583002/diff/160001/base/file_version_info... base/file_version_info_win.cc:203: data_(owned_data_.data()), On 2016/06/23 04:33:53, Lei Zhang (Slow) wrote: > What happens when |owned_data_| is empty? Or does that never happen? Never happens. See line 76 and line 207 of this file. https://codereview.chromium.org/2046583002/diff/160001/base/file_version_info... File base/file_version_info_win.h (right): https://codereview.chromium.org/2046583002/diff/160001/base/file_version_info... base/file_version_info_win.h:61: FileVersionInfoWin(std::vector<uint8_t> data, WORD language, WORD code_page); On 2016/06/23 04:33:53, Lei Zhang wrote: > std::vector<uint8_t>&& data ? Done. https://codereview.chromium.org/2046583002/diff/160001/base/file_version_info... base/file_version_info_win.h:69: // This is a pointer into the data_ if it exists. Otherwise nullptr. On 2016/06/23 04:33:53, Lei Zhang wrote: > |data_| Done. https://codereview.chromium.org/2046583002/diff/160001/base/file_version_info... File base/file_version_info_win_unittest.cc (right): https://codereview.chromium.org/2046583002/diff/160001/base/file_version_info... base/file_version_info_win_unittest.cc:37: FileVersionInfoFactory(const FilePath& path) : path_(path) {} On 2016/06/23 04:33:53, Lei Zhang wrote: > explicit, ditto below Done. https://codereview.chromium.org/2046583002/diff/160001/base/file_version_info... base/file_version_info_win_unittest.cc:136: static_assert(arraysize(kDLLNames) == arraysize(kExpectedIsOfficialBuild), On 2016/06/23 04:33:53, Lei Zhang wrote: > I realize this is pre-existing, but it's a bit wonky, just like having an unit > test where everything is ifdef'd for OS_WIN. > > There's lots of examples in base/ where one creates a struct with members for > the test input and result. Since we are changing this, can we do that instead? Done.
lgtm https://codereview.chromium.org/2046583002/diff/200001/base/file_version_info... File base/file_version_info_win_unittest.cc (right): https://codereview.chromium.org/2046583002/diff/200001/base/file_version_info... base/file_version_info_win_unittest.cc:132: } test_items[]{ nit: space between ']' and '{', maybe |kTestItems| ?
https://codereview.chromium.org/2046583002/diff/200001/base/file_version_info... File base/file_version_info_win_unittest.cc (right): https://codereview.chromium.org/2046583002/diff/200001/base/file_version_info... base/file_version_info_win_unittest.cc:132: } test_items[]{ On 2016/06/27 20:57:22, Lei Zhang wrote: > nit: space between ']' and '{', maybe |kTestItems| ? space between ] and { : git cl format removes it |kTestItems| : done
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2046583002/#ps220001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2046583002/#ps240001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by fdoray@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/f177a678814f97f0b35bd6aa678c1cf885ad1656 Cr-Commit-Position: refs/heads/master@{#402549}
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/2102363002/ by petewil@chromium.org. The reason for reverting is: 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 . |