Fix CRITICAL_SECTION test

Fix CRITICAL_SECTION test

I thought I had confirmed that this still allocated and ignored the flag
on older OSs, but I must have not had the PLOG active yet? I'm not sure
what I did. (I might try to blame VMware as it has an annoying habit of
caching old binaries when you use it's "Shared Folders" feature to point
at the dev machine's build dir.)

I confirmed that it does work on Win8 and Win10 but doesn't on Win XP
and Win 7.

R=mark@chromium.org
BUG=crashpad:52
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/d1e49bd221d8584b77566da52ffc293203360c3d

[Mark Mentovai, 2015-10-16 03:36:44]

We also need to avoid the PLOG in crashpad_client_win.cc, or avoid calling
CrashpadInitializeCriticalSectionEx() altogether when we know it’ll fail. There
are a handful of other comments that you’ll want to revise.

https://codereview.chromium.org/1405243002/diff/40001/handler/win/crashy_test...
File handler/win/crashy_test_program.cc (right):

https://codereview.chromium.org/1405243002/diff/40001/handler/win/crashy_test...
handler/win/crashy_test_program.cc:127: if (major_version >= 6 && minor_version
>= 2)
This condition doesn’t seem quite right, it won’t match Windows 10. I think you
want (major == 6 && minor >= 2) || major > 6

[scottmg, 2015-10-16 19:25:05]

Patchset #4 (id:60001) has been deleted

[scottmg, 2015-10-16 19:25:16]

Patchset #4 (id:80001) has been deleted

[scottmg, 2015-10-16 19:28:09]

Patchset #4 (id:100001) has been deleted

[scottmg, 2015-10-16 19:29:00]

Ended up installing and configuring all those VMs after all!

Reworked to factor out the CRITICAL_SECTION allocation, and allocate with debug
info where possible (everywhere except 7 it seems.)

https://codereview.chromium.org/1405243002/diff/40001/handler/win/crashy_test...
File handler/win/crashy_test_program.cc (right):

https://codereview.chromium.org/1405243002/diff/40001/handler/win/crashy_test...
handler/win/crashy_test_program.cc:127: if (major_version >= 6 && minor_version
>= 2)
On 2015/10/16 03:36:44, Mark Mentovai wrote:
> This condition doesn’t seem quite right, it won’t match Windows 10. I think
you
> want (major == 6 && minor >= 2) || major > 6

It happens to work because we're unmanifested, but was unnecessarily confusing.
(The version apis are such a mess now.)

[Mark Mentovai, 2015-10-16 19:42:47]

https://codereview.chromium.org/1405243002/diff/120001/client/crashpad_client...
File client/crashpad_client_win.cc (right):

https://codereview.chromium.org/1405243002/diff/120001/client/crashpad_client...
client/crashpad_client_win.cc:153: PLOG(ERROR) <<
"InitializeCriticalSectionWithDebugInfoIfPossible";
I think you should move this into the function (and document that it logs). The
reason is that InitializeCriticalSectionWithDebugInfoIfPossible() can return
false without setting GetLastError(). (It shouldn’t happen, but it’s logically
possible, and that case should have its own non-P LOG().)

https://codereview.chromium.org/1405243002/diff/120001/snapshot/win/end_to_en...
File snapshot/win/end_to_end_test.py (right):

https://codereview.chromium.org/1405243002/diff/120001/snapshot/win/end_to_en...
snapshot/win/end_to_end_test.py:187: # We can't allocate CSs with .DebugInfo on
Win 7.
Spell out critical section.

https://codereview.chromium.org/1405243002/diff/120001/util/win/critical_sect...
File util/win/critical_section_with_debug_info.cc (right):

https://codereview.chromium.org/1405243002/diff/120001/util/win/critical_sect...
util/win/critical_section_with_debug_info.cc:35: bool
InitializeCriticalSectionWithDebugInfoIfPossible(
Add a test for this. All you have to do is call it and ASSERT_TRUE(), and then
delete the critical section. Make sure it’s something that would have failed on
older OS versions if the win7_or_lower conditional wasn’t there.

https://codereview.chromium.org/1405243002/diff/120001/util/win/critical_sect...
util/win/critical_section_with_debug_info.cc:45: // it on for Win 7, however the
Loader Lock does have .DebugInfo so there may
Since on Windows 7 (and older), the loader lock has a valid .DebugInfo, does
that mean that we still have full coverage of all critical sections on those old
OS versions?

[scottmg, 2015-10-16 20:04:11]

https://codereview.chromium.org/1405243002/diff/120001/client/crashpad_client...
File client/crashpad_client_win.cc (right):

https://codereview.chromium.org/1405243002/diff/120001/client/crashpad_client...
client/crashpad_client_win.cc:153: PLOG(ERROR) <<
"InitializeCriticalSectionWithDebugInfoIfPossible";
On 2015/10/16 19:42:47, Mark Mentovai wrote:
> I think you should move this into the function (and document that it logs).
The
> reason is that InitializeCriticalSectionWithDebugInfoIfPossible() can return
> false without setting GetLastError(). (It shouldn’t happen, but it’s logically
> possible, and that case should have its own non-P LOG().)

I don't think it could have ever returned false without setting GetLastError()
(either from GetProcAddress() or InitializeCriticalSectionEx()) but it makes
more sense inside anyway.

https://codereview.chromium.org/1405243002/diff/120001/snapshot/win/end_to_en...
File snapshot/win/end_to_end_test.py (right):

https://codereview.chromium.org/1405243002/diff/120001/snapshot/win/end_to_en...
snapshot/win/end_to_end_test.py:187: # We can't allocate CSs with .DebugInfo on
Win 7.
On 2015/10/16 19:42:47, Mark Mentovai wrote:
> Spell out critical section.

Done.

https://codereview.chromium.org/1405243002/diff/120001/util/win/critical_sect...
File util/win/critical_section_with_debug_info.cc (right):

https://codereview.chromium.org/1405243002/diff/120001/util/win/critical_sect...
util/win/critical_section_with_debug_info.cc:35: bool
InitializeCriticalSectionWithDebugInfoIfPossible(
On 2015/10/16 19:42:47, Mark Mentovai wrote:
> Add a test for this. All you have to do is call it and ASSERT_TRUE(), and then
> delete the critical section. Make sure it’s something that would have failed
on
> older OS versions if the win7_or_lower conditional wasn’t there.

Done.

https://codereview.chromium.org/1405243002/diff/120001/util/win/critical_sect...
util/win/critical_section_with_debug_info.cc:45: // it on for Win 7, however the
Loader Lock does have .DebugInfo so there may
On 2015/10/16 19:42:47, Mark Mentovai wrote:
> Since on Windows 7 (and older), the loader lock has a valid .DebugInfo, does
> that mean that we still have full coverage of all critical sections on those
old
> OS versions?

Yes, we should have all of them on XP and Vista. We'll only get the
OS-allocated-with -DebugInfo on 7, and some subset on 8+ (as there are multiple
lists).

[Mark Mentovai, 2015-10-16 21:47:41]

LGTM.

You're right about GetLastError(). That GetProcAddress(LoadLibrary()) construct
has become so familiar that I don't even think of it as Win32 anymore.

I forgot whether the version reported would be stuck at 10 or before 10 without
a manifest.

https://codereview.chromium.org/1405243002/diff/160001/handler/win/crashy_tes...
File handler/win/crashy_test_program.cc (right):

https://codereview.chromium.org/1405243002/diff/160001/handler/win/crashy_tes...
handler/win/crashy_test_program.cc:27: #include
"util/win/critical_section_with_debug_info.h"
Add a dependency on util in the .gyp file.

https://codereview.chromium.org/1405243002/diff/160001/util/win/critical_sect...
File util/win/critical_section_with_debug_info.h (right):

https://codereview.chromium.org/1405243002/diff/160001/util/win/critical_sect...
util/win/critical_section_with_debug_info.h:27: //! Success means that the
critical section was successfully initialized, but it
Hanging indent.

[scottmg, 2015-10-16 21:53:41]

On 2015/10/16 21:47:41, Mark Mentovai wrote:
> LGTM.
> 
> You're right about GetLastError(). That GetProcAddress(LoadLibrary())
construct
> has become so familiar that I don't even think of it as Win32 anymore.

Your mind is now in a terrible place after all these NT reviews. :)

> 
> I forgot whether the version reported would be stuck at 10 or before 10
without
> a manifest.
> 
>
https://codereview.chromium.org/1405243002/diff/160001/handler/win/crashy_tes...
> File handler/win/crashy_test_program.cc (right):
> 
>
https://codereview.chromium.org/1405243002/diff/160001/handler/win/crashy_tes...
> handler/win/crashy_test_program.cc:27: #include
> "util/win/critical_section_with_debug_info.h"
> Add a dependency on util in the .gyp file.
> 
>
https://codereview.chromium.org/1405243002/diff/160001/util/win/critical_sect...
> File util/win/critical_section_with_debug_info.h (right):
> 
>
https://codereview.chromium.org/1405243002/diff/160001/util/win/critical_sect...
> util/win/critical_section_with_debug_info.h:27: //! Success means that the
> critical section was successfully initialized, but it
> Hanging indent.

[scottmg, 2015-10-16 21:53:45]

https://codereview.chromium.org/1405243002/diff/160001/handler/win/crashy_tes...
File handler/win/crashy_test_program.cc (right):

https://codereview.chromium.org/1405243002/diff/160001/handler/win/crashy_tes...
handler/win/crashy_test_program.cc:27: #include
"util/win/critical_section_with_debug_info.h"
On 2015/10/16 21:47:41, Mark Mentovai wrote:
> Add a dependency on util in the .gyp file.

Done.

https://codereview.chromium.org/1405243002/diff/160001/util/win/critical_sect...
File util/win/critical_section_with_debug_info.h (right):

https://codereview.chromium.org/1405243002/diff/160001/util/win/critical_sect...
util/win/critical_section_with_debug_info.h:27: //! Success means that the
critical section was successfully initialized, but it
On 2015/10/16 21:47:41, Mark Mentovai wrote:
> Hanging indent.

Done.

[scottmg, 2015-10-16 21:55:20]

Committed patchset #8 (id:200001) manually as
d1e49bd221d8584b77566da52ffc293203360c3d (presubmit successful).

[Mark Mentovai, 2015-10-16 23:19:20]

scottmg wrote:
> Ended up installing and configuring all those VMs after all!

Yeah, why should I be the only one with ten VMs on a laptop?

(Only two of them are Windows. So far.)