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

Issue 1493933002: win: Placate more OS versions in the PEImageReader test (Closed)

Created:
5 years ago by Mark Mentovai
Modified:
5 years ago
Reviewers:
scottmg
CC:
crashpad-dev_chromium.org
Base URL:
https://chromium.googlesource.com/crashpad/crashpad@master
Target Ref:
refs/heads/master
Project:
crashpad
Visibility:
Public.

Description

win: Placate more OS versions in the PEImageReader test The BaseName() was added because system DLLs were being reported by GetFileVersionInfo()/VerQueryValue() as having major file versions of 6.2 instead of 10.0 on Windows 10 when accessed by full path, but not by BaseName(). The PEImageReader gets the correct version from the in-memory images, 10.0. This trick didn't work on Windows XP, where two copies of comctl32.dll were found loaded into the process, one with a major file version of 5.82 and the other with 6.0. Giving GetFileVersionInfo() the BaseName() would result in it returning information from one of these, which would cause the version to not match when the PEImageReader was looking at the other. All of these GetFileVersionInfo() quirks make me glad that we're not using it anymore (outside of the test). Because of the version numbers involved (NT 6.2 = Windows 8, where GetVersion()/GetVersionEx() start behaving differently for non-manifested applications) and the fact that GetFileVersionInfo() and VerQueryValue() seem to report 10.0 even with full paths on Windows 10 in applications manifested to run on that OS, the BaseName() thing is restricted to Windows 8 and higher. TEST=crashpad_snapshot_test PEImageReader.VSFixedFileInfo_AllModules BUG=crashpad:78 R=scottmg@chromium.org Committed: https://chromium.googlesource.com/crashpad/crashpad/+/47fbac4ae10514cda2c8d5bd86bd1f690cdd1d5e

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+16 lines, -5 lines) Patch
M snapshot/win/pe_image_reader_test.cc View 1 chunk +16 lines, -5 lines 0 comments Download

Messages

Total messages: 5 (2 generated)
Mark Mentovai
I tested on XP, 7, and 10, and Scott tested on Vista. It seems like ...
5 years ago (2015-12-02 22:25:18 UTC) #2
scottmg
lgtm
5 years ago (2015-12-02 22:30:18 UTC) #3
Mark Mentovai
5 years ago (2015-12-02 22:48:25 UTC) #5
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as
47fbac4ae10514cda2c8d5bd86bd1f690cdd1d5e (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698