Stack sampling profiler: handle unloaded and unloading modules

Stack sampling profiler: handle unloading and unloaded modules

Attempt to increment the reference count for the module at the
instruction pointer before unwinding the frame. If successful the module
is guaranteed be in memory while the stack is being processed. If not
successful, the module has been unloaded and unwinding from the frame is
not possible.

Fixes crashes attempting to unwind frames from modules that get unloaded
between copying the stack and walking the copy of the stack.

In theory it could happen that a module is unloaded then another module
is immediately loaded in a similar memory region, such that the new
module contains an instruction pointer associated with the unloaded
module. This likely would result in a crash unwinding the frame with the
instruction pointer. It's not clear that there's anything that can be
done to detect/avoid this case; the hope is that it occurs rarely.

BUG=545051
Committed: https://crrev.com/f2d564443af10160ebb9ce97e9d1f103f8544fe0
Cr-Commit-Position: refs/heads/master@{#357453}

[Mike Wittman, 2015-10-21 22:47:08]

Description was changed from

==========
Test profiler change

BUG=
==========

to

==========
Stack sampling profiler: handle unloading and unloaded modules

Attempt to increment the reference count for the module at the
instruction pointer before unwinding the frame. If successful the module
is guaranteed be in memory while the stack is being processed. If not
successful, the module has been unloaded and unwinding from the frame is
not possible.

Fixes crashes attempting to unwind frames from modules that get unloaded
between copying the stack and walking the copy of the stack.

In theory it could happen that a module is unloaded then another module
is immediately loaded in a similar memory region, such that the new
module contains an instruction pointer associated with the unloaded
module. This likely would result in a crash unwinding the frame with the
instruction pointer. It's not clear that there's anything that can be
done to detect/avoid this case; the hope is that it occurs rarely.

BUG=545051
==========

[Mike Wittman, 2015-10-21 22:47:08]

wittman@chromium.org changed reviewers:
+ cpu@chromium.org

[Mike Wittman, 2015-10-21 23:01:25]

PTAL

This is a bit long, but most of the changes are in support of tests.

[cpu_(ooo_6.6-7.5), 2015-10-22 22:16:20]

wouldn't it deadlock if somebody who is suspended has acquired the loader lock?

[Mike Wittman, 2015-10-22 22:51:33]

On 2015/10/22 22:16:20, cpu wrote:
> wouldn't it deadlock if somebody who is suspended has acquired the loader
lock?

No. The attempted increment of the module reference count occurs after
ResumeThread.

[cpu_(ooo_6.6-7.5), 2015-10-27 17:51:45]

cpu@chromium.org changed reviewers:
+ brucedawson@chromium.org

[cpu_(ooo_6.6-7.5), 2015-10-29 21:22:20]

we are getting deeper in the weeds here, it is clear that the facility that we
are trying to invent here requires an external process which is debugging
chrome, anything else is going to be hack upon hack.

My only real question is the test delegate, why is there? it seems to be active
in regular runs?

https://codereview.chromium.org/1423583002/diff/20001/base/profiler/stack_sam...
File base/profiler/stack_sampling_profiler.h (right):

https://codereview.chromium.org/1423583002/diff/20001/base/profiler/stack_sam...
base/profiler/stack_sampling_profiler.h:241: 
this looks strange here.

https://codereview.chromium.org/1423583002/diff/20001/base/profiler/win32_sta...
File base/profiler/win32_stack_frame_unwinder.cc (right):

https://codereview.chromium.org/1423583002/diff/20001/base/profiler/win32_sta...
base/profiler/win32_stack_frame_unwinder.cc:190: return false;
we should note that the module at that address could also be something else.

[Mike Wittman, 2015-10-29 23:23:28]

On 2015/10/29 21:22:20, cpu wrote:
> we are getting deeper in the weeds here, it is clear that the facility that we
> are trying to invent here requires an external process which is debugging
> chrome, anything else is going to be hack upon hack.

I believe we're close to the end of the hacks. I reviewed a large number of the
current crashes and nearly all were due to the dll unloading issue addressed by
this change. The module info in WinDbg shows which modules were previously
loaded at the address in question, and I did not see any instances where a
different module was loaded over the old module's address range.

We can also undo some of the existing hacks by adding problematic third party
modules to the browser blacklist. jschuh has recommended this as a better way to
address these issues.

I think this is the light at the end of the tunnel. Can we see this approach
through before trying to adopt a separate process/debugger API approach? Chances
are that will have its own set of difficult-to-foresee issues that would also
need to be worked through.

> My only real question is the test delegate, why is there? it seems to be
active
> in regular runs?

It's only used for tests -- the StackSamplingProfiler creation in
ChromeBrowserMainParts is unchanged and using the constructor that sets the test
delegate to null.

The test delegate is needed to provide a test seam after the stack copying and
before the stack walking, so tests can force unloading of a module on the stack
before walking it.

https://codereview.chromium.org/1423583002/diff/20001/base/profiler/stack_sam...
File base/profiler/stack_sampling_profiler.h (right):

https://codereview.chromium.org/1423583002/diff/20001/base/profiler/stack_sam...
base/profiler/stack_sampling_profiler.h:241: 
On 2015/10/29 21:22:19, cpu wrote:
> this looks strange here.

It could be avoided by passing this in Start(). But I think it gives a cleaner
interface to provide all the configuration state at construction time. Added a
comment.

https://codereview.chromium.org/1423583002/diff/20001/base/profiler/win32_sta...
File base/profiler/win32_stack_frame_unwinder.cc (right):

https://codereview.chromium.org/1423583002/diff/20001/base/profiler/win32_sta...
base/profiler/win32_stack_frame_unwinder.cc:190: return false;
On 2015/10/29 21:22:19, cpu wrote:
> we should note that the module at that address could also be something else.

Done.

[brucedawson, 2015-10-30 00:16:00]

Where does the reference count get incremented? I expected to see this in
Win32StackFrameUnwinder::TryUnwind but I couldn't see any code that did this or
a comment that said 'increment'.

https://codereview.chromium.org/1423583002/diff/40001/base/profiler/native_st...
File base/profiler/native_stack_sampler.h (right):

https://codereview.chromium.org/1423583002/diff/40001/base/profiler/native_st...
base/profiler/native_stack_sampler.h:52: // NativeStackSamplerTestDelegate
provides seams for test code to execute during
I'm not sure what you mean by 'seams' here, although the later comment makes the
purpose of this class clear. It this a technical use of 'seams' that I just
haven't encountered yet?

https://codereview.chromium.org/1423583002/diff/40001/base/profiler/native_st...
File base/profiler/native_stack_sampler_win.cc (right):

https://codereview.chromium.org/1423583002/diff/40001/base/profiler/native_st...
base/profiler/native_stack_sampler_win.cc:144: ScopedModuleHandle modules[]) {
Having array function parameters is frustrating because, of course, they come in
as pointers rather than arrays, so you have to pass in the max_stack_size value.

This may be out-of-scope for this change but there is a solution which is to use
array references, like this:

    const void* (&instruction_pointers)[max_stack_size],
    ScopedModuleHandle (&modules)[max_stack_size]

where max_stack_size is not a function parameter (that parameter goes away) but
instead is an integral constant. It reduces flexibility (unless you template on
the size) but it means you get to use ARRAYSIZE() on |instruction_pointers| and
|modules|. Calling the function is simpler because you don't have to pass in the
size, and calling the function incorrectly is impossible - a wonderful feature.

Food for thought.

https://codereview.chromium.org/1423583002/diff/40001/base/profiler/native_st...
base/profiler/native_stack_sampler_win.cc:171: // Note that the AGE field is
encoded in decimal, not hex.
Unless you are using it for symbol server purposes when it is hexadecimal.
Sigh...

https://codereview.chromium.org/1423583002/diff/40001/base/profiler/stack_sam...
File base/profiler/stack_sampling_profiler.h (right):

https://codereview.chromium.org/1423583002/diff/40001/base/profiler/stack_sam...
base/profiler/stack_sampling_profiler.h:161: // constructor is for test
purposes.
Wouldn't it be simpler to have test_delegate have a default value and just have
the one constructor?

https://codereview.chromium.org/1423583002/diff/40001/base/profiler/stack_sam...
File base/profiler/stack_sampling_profiler_unittest.cc (right):

https://codereview.chromium.org/1423583002/diff/40001/base/profiler/stack_sam...
base/profiler/stack_sampling_profiler_unittest.cc:295:
PlatformThread::YieldCurrentThread();
This maps to Sleep(0) which, even in test code, makes me twitch. Can we use
PlatformThread::Sleep(1) so that we're not busy waiting for the module to
unload?

Probably YieldCurrentThread should not exist or should not be implemented that
way.

https://codereview.chromium.org/1423583002/diff/40001/base/profiler/stack_sam...
base/profiler/stack_sampling_profiler_unittest.cc:421: // Tests the scenario
where the library is unloaded after copying the stack, but
Have you confirmed that this test reliably crashes in 64-bit builds without the
fix?

[Mike Wittman, 2015-10-30 17:08:07]

On 2015/10/30 00:16:00, brucedawson wrote:
> Where does the reference count get incremented? I expected to see this in
> Win32StackFrameUnwinder::TryUnwind but I couldn't see any code that did this
or
> a comment that said 'increment'.

That's done by GetModuleHandleEx. I've added a comment to
Win32UnwindFunctions::GetModuleForProgramCounter to call this out.

https://codereview.chromium.org/1423583002/diff/40001/base/profiler/native_st...
File base/profiler/native_stack_sampler.h (right):

https://codereview.chromium.org/1423583002/diff/40001/base/profiler/native_st...
base/profiler/native_stack_sampler.h:52: // NativeStackSamplerTestDelegate
provides seams for test code to execute during
On 2015/10/30 00:15:59, brucedawson wrote:
> I'm not sure what you mean by 'seams' here, although the later comment makes
the
> purpose of this class clear. It this a technical use of 'seams' that I just
> haven't encountered yet?

Yes, I believe it's originally from _Working Effectively with Legacy Code_. A
seam a place where the behavior of code can be altered without altering the code
itself, usually for unit testing testing purposes.

http://www.informit.com/articles/article.aspx?p=359417&seqNum=2

https://codereview.chromium.org/1423583002/diff/40001/base/profiler/native_st...
File base/profiler/native_stack_sampler_win.cc (right):

https://codereview.chromium.org/1423583002/diff/40001/base/profiler/native_st...
base/profiler/native_stack_sampler_win.cc:144: ScopedModuleHandle modules[]) {
On 2015/10/30 00:15:59, brucedawson wrote:
> Having array function parameters is frustrating because, of course, they come
in
> as pointers rather than arrays, so you have to pass in the max_stack_size
value.
> 
> This may be out-of-scope for this change but there is a solution which is to
use
> array references, like this:
> 
>     const void* (&instruction_pointers)[max_stack_size],
>     ScopedModuleHandle (&modules)[max_stack_size]
> 
> where max_stack_size is not a function parameter (that parameter goes away)
but
> instead is an integral constant. It reduces flexibility (unless you template
on
> the size) but it means you get to use ARRAYSIZE() on |instruction_pointers|
and
> |modules|. Calling the function is simpler because you don't have to pass in
the
> size, and calling the function incorrectly is impossible - a wonderful
feature.
> 
> Food for thought.

That's a nice alternative, but unfortunately it would violate the style guide
here since mutable reference parameters are prohibited. I suppose the parameters
could be passed by pointer but that might be more confusing than its worth.

In any case, these will probably get changed to vectors in a future CL since
this code no longer is restricted from doing memory allocation.

https://codereview.chromium.org/1423583002/diff/40001/base/profiler/native_st...
base/profiler/native_stack_sampler_win.cc:171: // Note that the AGE field is
encoded in decimal, not hex.
On 2015/10/30 00:15:59, brucedawson wrote:
> Unless you are using it for symbol server purposes when it is hexadecimal.
> Sigh...

Yeah, it needs to be this way to match the server-side implementation. I can't
see us interacting with symbol servers on the client, so chances for confusion
are likely pretty low.

https://codereview.chromium.org/1423583002/diff/40001/base/profiler/stack_sam...
File base/profiler/stack_sampling_profiler.h (right):

https://codereview.chromium.org/1423583002/diff/40001/base/profiler/stack_sam...
base/profiler/stack_sampling_profiler.h:161: // constructor is for test
purposes.
On 2015/10/30 00:15:59, brucedawson wrote:
> Wouldn't it be simpler to have test_delegate have a default value and just
have
> the one constructor?

I'm not sure. Although default arguments are permitted on constructors, using
separate functions is consistent with the rest of the code where they're
prohibited. This way probably looks less strange to the average reader who's
unfamiliar with the quirks of the style guide.

https://codereview.chromium.org/1423583002/diff/40001/base/profiler/stack_sam...
File base/profiler/stack_sampling_profiler_unittest.cc (right):

https://codereview.chromium.org/1423583002/diff/40001/base/profiler/stack_sam...
base/profiler/stack_sampling_profiler_unittest.cc:295:
PlatformThread::YieldCurrentThread();
On 2015/10/30 00:16:00, brucedawson wrote:
> This maps to Sleep(0) which, even in test code, makes me twitch. Can we use
> PlatformThread::Sleep(1) so that we're not busy waiting for the module to
> unload?
> 
> Probably YieldCurrentThread should not exist or should not be implemented that
> way.

Done.

https://codereview.chromium.org/1423583002/diff/40001/base/profiler/stack_sam...
base/profiler/stack_sampling_profiler_unittest.cc:421: // Tests the scenario
where the library is unloaded after copying the stack, but
On 2015/10/30 00:16:00, brucedawson wrote:
> Have you confirmed that this test reliably crashes in 64-bit builds without
the
> fix?

Yes, for |wait_until_unloaded| == true and for |wait_until_unloaded| == false if
the library was unloaded. I wrote the test before updating the code.

[brucedawson, 2015-10-30 23:40:57]

Sorry for the delayed reply.

> I've added a comment to Win32UnwindFunctions::GetModuleForProgramCounter to
call this out.

Excellent.

> mutable reference parameters are prohibited

I think we could make a strong case for this being an exception because an array
reference behaves identically (except for the advantages) to the non-reference
array. The usual reason to prohibit mutable references is because it makes the
mutability hidden, and that is not the case here. Anyway, that's a
future-thought idea anyway.

lgtm

[cpu_(ooo_6.6-7.5), 2015-11-01 22:38:20]

lgtm

[Mike Wittman, 2015-11-02 17:43:02]

The CQ bit was checked by wittman@chromium.org

[commit-bot: I haz the power, 2015-11-02 17:43:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1423583002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1423583002/60001

[commit-bot: I haz the power, 2015-11-02 17:54:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-02 17:54:17]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Mike Wittman, 2015-11-02 20:24:42]

The CQ bit was checked by wittman@chromium.org

[Mike Wittman, 2015-11-02 20:24:43]

The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org,
brucedawson@chromium.org
Link to the patchset: https://codereview.chromium.org/1423583002/#ps80001
(title: "non-Win compile fixes")

[commit-bot: I haz the power, 2015-11-02 20:25:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1423583002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1423583002/80001

[commit-bot: I haz the power, 2015-11-02 20:40:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-02 20:40:34]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Mike Wittman, 2015-11-02 21:23:36]

The CQ bit was checked by wittman@chromium.org

[Mike Wittman, 2015-11-02 21:23:36]

The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org,
brucedawson@chromium.org
Link to the patchset: https://codereview.chromium.org/1423583002/#ps120001
(title: "fix gcc compile")

[commit-bot: I haz the power, 2015-11-02 21:24:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1423583002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1423583002/120001

[commit-bot: I haz the power, 2015-11-02 22:38:47]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-02 22:39:35]

Patchset 7 (id:??) landed as
https://crrev.com/f2d564443af10160ebb9ce97e9d1f103f8544fe0
Cr-Commit-Position: refs/heads/master@{#357453}