win: Add a VSFixedFileInfo test to check all modules

win: Add a VSFixedFileInfo test to check all modules

BUG=crashpad:78
TEST=crashpad_snapshot_test PEImageReader.VSFixedFileInfo_AllModules
R=scottmg@chromium.org

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/384fd93d6d7068ce19bced1cae9be057d23a843f

[Mark Mentovai, 2015-12-01 23:24:04]

Description was changed from

==========
win: Add a VSFixedFileInfo test to check all modules

BUG=crashpad:78
==========

to

==========
win: Add a VSFixedFileInfo test to check all modules

BUG=crashpad:78
TEST=crashpad_snapshot_test PEImageReader.VSFixedFileInfo_AllModules
==========

[Mark Mentovai, 2015-12-01 23:42:54]

mark@chromium.org changed reviewers:
+ scottmg@chromium.org

[Mark Mentovai, 2015-12-01 23:42:54]

https://codereview.chromium.org/1485333002/diff/1/snapshot/win/pe_image_reade...
File snapshot/win/pe_image_reader_test.cc (right):

https://codereview.chromium.org/1485333002/diff/1/snapshot/win/pe_image_reade...
snapshot/win/pe_image_reader_test.cc:102:
base::FilePath(module.name).BaseName(), &expected);
This was really weird. Without the BaseName(), I got a bunch of failures of the
form

[ RUN      ] PEImageReader.VSFixedFileInfo_AllModules
c:\chrome\crashpad\crashpad\snapshot\win\pe_image_reader_test.cc(112): error:
Value of: observed.dwFileVersionMS
  Actual: 655360
Expected: expected.dwFileVersionMS
Which is: 393218
Google Test trace:
c:\chrome\crashpad\crashpad\snapshot\win\pe_image_reader_test.cc(158):
C:\WINDOWS\SYSTEM32\ntdll.dll

Actual (in memory) 655360 = 0xa0000 (meaning 10.0), expected (on disk) 393218 =
0x60002 (meaning 6.2). These numbers were the same for every DLL that had this
problem. It was always the file version with a mismatch, the product versions
always matched. It was always only the most-significant DWORD that had a
problem, the least-significant ones were always 0x295a0000 (10586.0). (Yeah, I
updated.)

I thought that this might have been a WinSxS problem, where the loader was
actually loading a side-by-side assembly, but reporting the path of the base
library in c:\windows\system32. But when I do Properties/Details on
c:\windows\system32\ntdll.dll, it shows 10.0.10586.0 for the file version. Among
all of the ntdll.dlls I found, I couldn’t find one that called itself 6.2, so I
don’t really know where that version came from.

Giving just the base name to GetFileVersionInfo() makes everything match up.

If you know more about why this might have happened, it’d at least be a useful
thing to stick in the comment here, or maybe on GetModuleVersionAndType().

[Mark Mentovai, 2015-12-01 23:48:08]

…and it was a problem without BaseName() on Windows 10. Windows 7 is happy with
or without it.

[scottmg, 2015-12-01 23:57:38]

https://codereview.chromium.org/1485333002/diff/1/snapshot/win/pe_image_reade...
File snapshot/win/pe_image_reader_test.cc (right):

https://codereview.chromium.org/1485333002/diff/1/snapshot/win/pe_image_reade...
snapshot/win/pe_image_reader_test.cc:102:
base::FilePath(module.name).BaseName(), &expected);
On 2015/12/01 23:42:54, Mark Mentovai wrote:
> This was really weird. Without the BaseName(), I got a bunch of failures of
the
> form
> 
> [ RUN      ] PEImageReader.VSFixedFileInfo_AllModules
> c:\chrome\crashpad\crashpad\snapshot\win\pe_image_reader_test.cc(112): error:
> Value of: observed.dwFileVersionMS
>   Actual: 655360
> Expected: expected.dwFileVersionMS
> Which is: 393218
> Google Test trace:
> c:\chrome\crashpad\crashpad\snapshot\win\pe_image_reader_test.cc(158):
> C:\WINDOWS\SYSTEM32\ntdll.dll
> 
> Actual (in memory) 655360 = 0xa0000 (meaning 10.0), expected (on disk) 393218
=
> 0x60002 (meaning 6.2). These numbers were the same for every DLL that had this
> problem. It was always the file version with a mismatch, the product versions
> always matched. It was always only the most-significant DWORD that had a
> problem, the least-significant ones were always 0x295a0000 (10586.0). (Yeah, I
> updated.)
> 
> I thought that this might have been a WinSxS problem, where the loader was
> actually loading a side-by-side assembly, but reporting the path of the base
> library in c:\windows\system32. But when I do Properties/Details on
> c:\windows\system32\ntdll.dll, it shows 10.0.10586.0 for the file version.
Among
> all of the ntdll.dlls I found, I couldn’t find one that called itself 6.2, so
I
> don’t really know where that version came from.
> 
> Giving just the base name to GetFileVersionInfo() makes everything match up.
> 
> If you know more about why this might have happened, it’d at least be a useful
> thing to stick in the comment here, or maybe on GetModuleVersionAndType().

I don't know, but because it's reporting 8.0, I would guess that it's related to
GetVersionEx() lying. (The test binaries aren't manifested.)

e.g.
https://msdn.microsoft.com/en-us/library/windows/desktop/ms724451(v=vs.85).aspx

But then I'm not sure why BaseName() would resolve that.

[scottmg, 2015-12-01 23:58:27]

lgtm

[Mark Mentovai, 2015-12-02 16:07:06]

scottmg wrote:
> I don't know, but because it's reporting 8.0, I would guess that it's related
to
> GetVersionEx() lying. (The test binaries aren't manifested.)
> 
> e.g.
>
https://msdn.microsoft.com/en-us/library/windows/desktop/ms724451(v=vs.85).aspx

I’d believe that. The minidumps coming from the canary are still getting the
module versions from GetFileVersionInfo() with a full path, and they have the
correct 10.0 for these modules, with the handler running out of the manifested
chrome.exe.

> But then I'm not sure why BaseName() would resolve that.

Yeah, it’s really weird.

[Mark Mentovai, 2015-12-02 16:11:58]

Description was changed from

==========
win: Add a VSFixedFileInfo test to check all modules

BUG=crashpad:78
TEST=crashpad_snapshot_test PEImageReader.VSFixedFileInfo_AllModules
==========

to

==========
win: Add a VSFixedFileInfo test to check all modules

BUG=crashpad:78
TEST=crashpad_snapshot_test PEImageReader.VSFixedFileInfo_AllModules
R=scottmg@chromium.org

Committed:
https://chromium.googlesource.com/crashpad/crashpad/+/384fd93d6d7068ce19bced1...
==========

[Mark Mentovai, 2015-12-02 16:11:59]

Committed patchset #2 (id:20001) manually as
384fd93d6d7068ce19bced1cae9be057d23a843f (presubmit successful).

[Mark Mentovai, 2015-12-02 19:58:02]

Oh, XP…

[ RUN      ] PEImageReader.VSFixedFileInfo_AllModules
c:\chrome\crashpad\crashpad\snapshot\win\pe_image_reader_test.cc(109): error:
Value of: observed.dwFileVersionMS
  Actual: 393216
Expected: expected.dwFileVersionMS
Which is: 327762
Google Test trace:
c:\chrome\crashpad\crashpad\snapshot\win\pe_image_reader_test.cc(155):
C:\WINDOWS\WinSxS\x86_Microsoft.Windows.Common-Controls_6595b64144ccf1df_6.0.2600.6028_x-ww_61e65202\comctl32.dll
[  FAILED  ] PEImageReader.VSFixedFileInfo_AllModules (16 ms)

\windows\system32\comctl32.dll has file version 5.82.2900.6028 but product
version 6.0.2900.6028. The one in winsxs at the path shown in the test has
6.0.2900.6028 for both.

[Mark Mentovai, 2015-12-02 20:02:42]

XP prefers things *without* BaseName(). Oh boy.

[Mark Mentovai, 2015-12-02 22:25:58]

XP test fix: https://codereview.chromium.org/1493933002/