win: Fix use of THREAD_ALL_ACCESS on XP

win: Fix use of THREAD_ALL_ACCESS on XP

OpenThread(THREAD_ALL_ACCESS, ...) fails on XP with the uplevel value of
THREAD_ALL_ACCESS, so use the XP value.

Similar to the PROCESS_ALL_ACCESS in
https://codereview.chromium.org/1337133002/ but I mistakenly only
grepped for PROCESS_ALL_ACCESS at that point.

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

[scottmg, 2015-09-11 22:51:01]

https://codereview.chromium.org/1337653005/diff/20001/util/win/xp_compat.h
File util/win/xp_compat.h (right):

https://codereview.chromium.org/1337653005/diff/20001/util/win/xp_compat.h#ne...
util/win/xp_compat.h:35: kXPThreadAllAccess = STANDARD_RIGHTS_REQUIRED |
SYNCHRONIZE | 0x3FF,
I could have left this in the test .cc, but it seemed odd to have one here and
not the other (it may yet be needed in the snapshot code too, currently failing
on XP for reasons unclear).

[Mark Mentovai, 2015-09-11 23:03:33]

LGTM

https://codereview.chromium.org/1337653005/diff/20001/util/win/scoped_process...
File util/win/scoped_process_suspend_test.cc (right):

https://codereview.chromium.org/1337653005/diff/20001/util/win/scoped_process...
util/win/scoped_process_suspend_test.cc:50: EXPECT_TRUE(thread.is_valid());
ASSERT_TRUE()? Otherwise everything that follows is suspect too.

All of these things that are ASSERTing or EXPECTing on a win32 call can stream
ErrorMessage from test/errors.h:
  ASSERT_TRUE(thread.is_valid()) << ErrorMessage("OpenThread");

https://codereview.chromium.org/1337653005/diff/20001/util/win/scoped_process...
util/win/scoped_process_suspend_test.cc:54: ResumeThread(thread.get());
Let’s harden this a bit more by EXPECT_TRUE()ing this guy’s result too. Maybe
the Thread32First() above, also.

https://codereview.chromium.org/1337653005/diff/20001/util/win/xp_compat.h
File util/win/xp_compat.h (right):

https://codereview.chromium.org/1337653005/diff/20001/util/win/xp_compat.h#ne...
util/win/xp_compat.h:35: kXPThreadAllAccess = STANDARD_RIGHTS_REQUIRED |
SYNCHRONIZE | 0x3FF,
scottmg wrote:
> I could have left this in the test .cc, but it seemed odd to have one here and
> not the other (it may yet be needed in the snapshot code too, currently
failing
> on XP for reasons unclear).

Yeah, I agree that she belongs here with her sister.

In this case, kXPThreadAllAccess misses THREAD_RESUME. Is that OK? Like the one
above, I assume it is because that’s implied by THREAD_SUSPEND_RESUME.

[scottmg, 2015-09-11 23:32:52]

https://codereview.chromium.org/1337653005/diff/20001/util/win/scoped_process...
File util/win/scoped_process_suspend_test.cc (right):

https://codereview.chromium.org/1337653005/diff/20001/util/win/scoped_process...
util/win/scoped_process_suspend_test.cc:50: EXPECT_TRUE(thread.is_valid());
On 2015/09/11 23:03:33, Mark Mentovai - August is over wrote:
> ASSERT_TRUE()? Otherwise everything that follows is suspect too.
> 
> All of these things that are ASSERTing or EXPECTing on a win32 call can stream
> ErrorMessage from test/errors.h:
>   ASSERT_TRUE(thread.is_valid()) << ErrorMessage("OpenThread");

Yeah, I just hate ASSERT because there's no "return false;" version. Am I using
it wrong?

Added ErrorMessage()s.

https://codereview.chromium.org/1337653005/diff/20001/util/win/scoped_process...
util/win/scoped_process_suspend_test.cc:54: ResumeThread(thread.get());
On 2015/09/11 23:03:33, Mark Mentovai - August is over wrote:
> Let’s harden this a bit more by EXPECT_TRUE()ing this guy’s result too. Maybe

Done (_NE -1).

> the Thread32First() above, also.

I think return false; is sufficient since it only makes sense to use this
function in an EXPECT_TRUE.

https://codereview.chromium.org/1337653005/diff/20001/util/win/xp_compat.h
File util/win/xp_compat.h (right):

https://codereview.chromium.org/1337653005/diff/20001/util/win/xp_compat.h#ne...
util/win/xp_compat.h:35: kXPThreadAllAccess = STANDARD_RIGHTS_REQUIRED |
SYNCHRONIZE | 0x3FF,
On 2015/09/11 23:03:33, Mark Mentovai - August is over wrote:
> scottmg wrote:
> > I could have left this in the test .cc, but it seemed odd to have one here
and
> > not the other (it may yet be needed in the snapshot code too, currently
> failing
> > on XP for reasons unclear).
> 
> Yeah, I agree that she belongs here with her sister.
> 
> In this case, kXPThreadAllAccess misses THREAD_RESUME. Is that OK? Like the
one
> above, I assume it is because that’s implied by THREAD_SUSPEND_RESUME.

Yes, I think so.

[Mark Mentovai, 2015-09-11 23:36:28]

LGTM

https://codereview.chromium.org/1337653005/diff/20001/util/win/scoped_process...
File util/win/scoped_process_suspend_test.cc (right):

https://codereview.chromium.org/1337653005/diff/20001/util/win/scoped_process...
util/win/scoped_process_suspend_test.cc:50: EXPECT_TRUE(thread.is_valid());
scottmg wrote:
> On 2015/09/11 23:03:33, Mark Mentovai - August is over wrote:
> > ASSERT_TRUE()? Otherwise everything that follows is suspect too.
> > 
> > All of these things that are ASSERTing or EXPECTing on a win32 call can
stream
> > ErrorMessage from test/errors.h:
> >   ASSERT_TRUE(thread.is_valid()) << ErrorMessage("OpenThread");
> 
> Yeah, I just hate ASSERT because there's no "return false;" version. Am I
using
> it wrong?

No, that’s right, forgot this was a bool-returning function.

https://codereview.chromium.org/1337653005/diff/20001/util/win/scoped_process...
util/win/scoped_process_suspend_test.cc:54: ResumeThread(thread.get());
scottmg wrote:
> I think return false; is sufficient since it only makes sense to use this
> function in an EXPECT_TRUE.

Yeah, but then from the caller’s perspective it looks like the suspend count was
wrong, when in reality we couldn’t even enumerate threads, and you’d waste
minutes debugging the wrong thing when trying to figure it out. For that reason,
it does make sense to log a special failure for Thread32First() and even
CreateToolhelp32Snapshot().

[scottmg, 2015-09-11 23:43:46]

https://codereview.chromium.org/1337653005/diff/20001/util/win/scoped_process...
File util/win/scoped_process_suspend_test.cc (right):

https://codereview.chromium.org/1337653005/diff/20001/util/win/scoped_process...
util/win/scoped_process_suspend_test.cc:54: ResumeThread(thread.get());
On 2015/09/11 23:36:28, Mark Mentovai - August is over wrote:
> scottmg wrote:
> > I think return false; is sufficient since it only makes sense to use this
> > function in an EXPECT_TRUE.
> 
> Yeah, but then from the caller’s perspective it looks like the suspend count
was
> wrong, when in reality we couldn’t even enumerate threads, and you’d waste
> minutes debugging the wrong thing when trying to figure it out. For that
reason,
> it does make sense to log a special failure for Thread32First() and even
> CreateToolhelp32Snapshot().

Done.

[scottmg, 2015-09-11 23:45:50]

Committed patchset #6 (id:80002) manually as
c3cc1d19c1f0ec0d78fa26d8604d8b598f014d9b (presubmit successful).