win: Fix incorrect thread suspend count due to ScopedProcessSuspend

win: Fix incorrect thread suspend count due to ScopedProcessSuspend

After https://codereview.chromium.org/1303173011/, the thread suspend
count would be one too large because the count is adjusted when the
process is suspended. Counteract this by passing in whether the
process is suspended or not so that the thread's suspension count
can be adjusted.

Add a test to sanity-check thread suspend count.

R=mark@chromium.org

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/d7f90b45b6318eb3e54002aa52bfd579e5346852

[Mark Mentovai, 2015-09-09 17:45:36]

A static map, a mutex to protect it…hmm. I think that seems a little intense for
what we’re trying to achieve.

I was thinking that the easy way out would be to construct the ProcessReader
with an extra argument that said “I’ve already suspended it X times.” A cleaner
design would move the Nt[Un]SuspendProcess() wrappers into ProcessReader so that
we can track the suspend count right there. Do either of those speak to you?

[scottmg, 2015-09-09 18:06:31]

On 2015/09/09 17:45:36, Mark Mentovai - August is over wrote:
> A static map, a mutex to protect it…hmm. I think that seems a little intense
for
> what we’re trying to achieve.

Agreed, it's too heavy.

> I was thinking that the easy way out would be to construct the ProcessReader
> with an extra argument that said “I’ve already suspended it X times.” A
cleaner
> design would move the Nt[Un]SuspendProcess() wrappers into ProcessReader so
that
> we can track the suspend count right there. Do either of those speak to you?

I don't like any of the solutions very much. I think the process should be
suspended as high/early as possible to minimize possibility of retrieving
confusing information.

I think it doesn't help that much to move Suspend/Resume into ProcessReader
since then it's either incorrect with multiple ProcessReader objects for the
same process, or we're back to global shared data.

Maybe the best thing to do is simply to have ProcessReader assume/assert that
the process is suspended (once) while it's active, which we'll then do in
generate_dump or the top level exception handler. Other solutions are more
complicated but are still inaccurate anyway, so maybe just accounting for the
local change in suspension count is the best we can do.

[Mark Mentovai, 2015-09-09 18:09:11]

The assertion would be OK, except it breaks the generate_dump --no-suspend
thing, which is mostly useful for edge cases and other testing, but we do have
tests where we need a ProcessReader for the self-process.

[scottmg, 2015-09-09 18:43:36]

On 2015/09/09 18:09:11, Mark Mentovai - August is over wrote:
> The assertion would be OK, except it breaks the generate_dump --no-suspend
> thing, which is mostly useful for edge cases and other testing, but we do have
> tests where we need a ProcessReader for the self-process.

That's true. OK, went with passing in the "best guess" suspension state of the
process to snapshot/reader.

[Mark Mentovai, 2015-09-09 18:57:27]

https://codereview.chromium.org/1326443007/diff/20001/snapshot/win/process_re...
File snapshot/win/process_reader_win.cc (right):

https://codereview.chromium.org/1326443007/diff/20001/snapshot/win/process_re...
snapshot/win/process_reader_win.cc:133: thread->suspend_count = 0;
LOG(ERROR) or DCHECK() if the suspension state is kSuspended here too.

https://codereview.chromium.org/1326443007/diff/20001/snapshot/win/process_re...
snapshot/win/process_reader_win.cc:143: LOG(ERROR) << "suspend count was 0, but
process should be suspended";
Upgrade (sidegrade) to a DCHECK()? This indicates a gross logic error.

https://codereview.chromium.org/1326443007/diff/20001/snapshot/win/process_re...
File snapshot/win/process_reader_win.h (right):

https://codereview.chromium.org/1326443007/diff/20001/snapshot/win/process_re...
snapshot/win/process_reader_win.h:31: //! \brief The process is suspended.
Reverse these so that “running” = “false” (not suspended). Your ternaries will
optimize away nicely that way.

https://codereview.chromium.org/1326443007/diff/20001/snapshot/win/process_re...
snapshot/win/process_reader_win.h:114: ProcessSuspensionState process_state_;
process_state_ sounds like it can mean a few other thigns. suspension_state_ or
process_suspension_state_?

https://codereview.chromium.org/1326443007/diff/20001/util/win/exception_hand...
File util/win/exception_handler_server.cc (right):

https://codereview.chromium.org/1326443007/diff/20001/util/win/exception_hand...
util/win/exception_handler_server.cc:412: ScopedProcessSuspend
suspend(client->process());
I think this worked better in CrashReportExceptionHandler. I don’t think it
should be up to the base to decide whether suspension is appropriate or not.
That’s a decision for the delegate.

[scottmg, 2015-09-09 19:13:25]

https://codereview.chromium.org/1326443007/diff/20001/snapshot/win/process_re...
File snapshot/win/process_reader_win.cc (right):

https://codereview.chromium.org/1326443007/diff/20001/snapshot/win/process_re...
snapshot/win/process_reader_win.cc:133: thread->suspend_count = 0;
On 2015/09/09 18:57:27, Mark Mentovai - August is over wrote:
> LOG(ERROR) or DCHECK() if the suspension state is kSuspended here too.

Done.

https://codereview.chromium.org/1326443007/diff/20001/snapshot/win/process_re...
snapshot/win/process_reader_win.cc:143: LOG(ERROR) << "suspend count was 0, but
process should be suspended";
On 2015/09/09 18:57:27, Mark Mentovai - August is over wrote:
> Upgrade (sidegrade) to a DCHECK()? This indicates a gross logic error.

Done.

https://codereview.chromium.org/1326443007/diff/20001/snapshot/win/process_re...
File snapshot/win/process_reader_win.h (right):

https://codereview.chromium.org/1326443007/diff/20001/snapshot/win/process_re...
snapshot/win/process_reader_win.h:31: //! \brief The process is suspended.
On 2015/09/09 18:57:27, Mark Mentovai - August is over wrote:
> Reverse these so that “running” = “false”&nbsp;(not suspended). Your ternaries
will
> optimize away nicely that way.

Done.

https://codereview.chromium.org/1326443007/diff/20001/snapshot/win/process_re...
snapshot/win/process_reader_win.h:114: ProcessSuspensionState process_state_;
On 2015/09/09 18:57:27, Mark Mentovai - August is over wrote:
> process_state_ sounds like it can mean a few other thigns. suspension_state_
or
> process_suspension_state_?

Done.

https://codereview.chromium.org/1326443007/diff/20001/util/win/exception_hand...
File util/win/exception_handler_server.cc (right):

https://codereview.chromium.org/1326443007/diff/20001/util/win/exception_hand...
util/win/exception_handler_server.cc:412: ScopedProcessSuspend
suspend(client->process());
On 2015/09/09 18:57:27, Mark Mentovai - August is over wrote:
> I think this worked better in CrashReportExceptionHandler. I don’t think it
> should be up to the base to decide whether suspension is appropriate or not.
> That’s a decision for the delegate.

OK, done.

https://codereview.chromium.org/1326443007/diff/40001/snapshot/win/process_re...
File snapshot/win/process_reader_win.cc (right):

https://codereview.chromium.org/1326443007/diff/40001/snapshot/win/process_re...
snapshot/win/process_reader_win.cc:133: DCHECK(suspension_state ==
ProcessSuspensionState::kRunning);
dcheck_eq fails in logging.h because of enum class not being int convertible.

[Mark Mentovai, 2015-09-09 19:22:11]

LGTM

https://codereview.chromium.org/1326443007/diff/40001/snapshot/win/process_re...
File snapshot/win/process_reader_win.cc (right):

https://codereview.chromium.org/1326443007/diff/40001/snapshot/win/process_re...
snapshot/win/process_reader_win.cc:133: DCHECK(suspension_state ==
ProcessSuspensionState::kRunning);
scottmg wrote:
> dcheck_eq fails in logging.h because of enum class not being int convertible.

That’s OK, DCHECK() is good.

https://codereview.chromium.org/1326443007/diff/40001/snapshot/win/process_re...
File snapshot/win/process_reader_win_test.cc (right):

https://codereview.chromium.org/1326443007/diff/40001/snapshot/win/process_re...
snapshot/win/process_reader_win_test.cc:143: EXPECT_EQ(0u,
thread.suspend_count);
In order to have assurance that this is testing anything, first do:

  ASSERT_FALSE(process_reader.Threads().empty());

(possibly saving Threads() in a local so you don’t need to call it twice.)

[scottmg, 2015-09-09 19:28:00]

https://codereview.chromium.org/1326443007/diff/40001/snapshot/win/process_re...
File snapshot/win/process_reader_win_test.cc (right):

https://codereview.chromium.org/1326443007/diff/40001/snapshot/win/process_re...
snapshot/win/process_reader_win_test.cc:143: EXPECT_EQ(0u,
thread.suspend_count);
On 2015/09/09 19:22:11, Mark Mentovai - August is over wrote:
> In order to have assurance that this is testing anything, first do:
> 
>   ASSERT_FALSE(process_reader.Threads().empty());
> 
> (possibly saving Threads() in a local so you don’t need to call it twice.)

Done.

[scottmg, 2015-09-09 19:29:36]

Committed patchset #4 (id:60001) manually as
d7f90b45b6318eb3e54002aa52bfd579e5346852 (presubmit successful).

[Mark Mentovai, 2015-09-09 19:29:50]

LGTM