Print stack traces in child processes when browser tests failed.

Print stack traces in browser tests when any process crashes, or an assert fires.

The functionality to do this opens up security holes. Currently this was working only for debug Linux builds. However our trybots are release builds, and we need to be able to see stack traces from processes on all platforms and not just Linux (i.e. to be able to debug the large flakiness that occurred last week). This is disabled for official builds.

Also make release (non-official) builds print the callstack on asserts, just like debug builds. This makes it easier to debug test failures on the CQ (for example, DCHECKs for non-threadsafe usage of pointers).

Add a regression test that both renderer and browser process crashes print the callstack.

BUG=517488, 358267, 521148
NOPRESUBMIT=true

Committed: https://crrev.com/8ba532e170befc312e66d032587fa2ad04bac975
Cr-Commit-Position: refs/heads/master@{#343240}

Committed: https://crrev.com/79dc59ac7602413181079ecb463873e29a1d7d0a
Cr-Commit-Position: refs/heads/master@{#343626}

[jam, 2015-08-12 00:54:13]

jam@chromium.org changed reviewers:
+ jln@chromium.org, scottmg@chromium.org

[jam, 2015-08-12 00:54:14]

scottmg: please review
jln: wanted to make sure you see this and you're ok with this. I saw your
comments on r258719.

[scottmg, 2015-08-12 02:28:15]

lgtm

https://codereview.chromium.org/1291553003/diff/1/base/process/launch_win.cc
File base/process/launch_win.cc (right):

https://codereview.chromium.org/1291553003/diff/1/base/process/launch_win.cc#...
base/process/launch_win.cc:72: // This causes NaCl tests to hang on XP for
reasons unclear, perhaps due
It would be better if you can remove this early return, otherwise won't get
stacks on XP. But I'm not sure if there will be problems with some NaCl modules
then.

https://codereview.chromium.org/1291553003/diff/1/content/renderer/renderer_m...
File content/renderer/renderer_main.cc (left):

https://codereview.chromium.org/1291553003/diff/1/content/renderer/renderer_m...
content/renderer/renderer_main.cc:192: // For convenience, we print the stack
traces for crashes.  When sandbox
is the CFI_ENFORCEMENT important? (I have no idea what that is)

[jam, 2015-08-12 03:01:33]

https://codereview.chromium.org/1291553003/diff/1/base/process/launch_win.cc
File base/process/launch_win.cc (right):

https://codereview.chromium.org/1291553003/diff/1/base/process/launch_win.cc#...
base/process/launch_win.cc:72: // This causes NaCl tests to hang on XP for
reasons unclear, perhaps due
On 2015/08/12 02:28:14, scottmg (returning - slow) wrote:
> It would be better if you can remove this early return, otherwise won't get
> stacks on XP. But I'm not sure if there will be problems with some NaCl
modules
> then.

I didn't want to deal with debugging nacl tests on xp so I left it as it was in
your cl. we have one xp bot vs many on waterfall and on trybots, so i figured
the most bang for the buck is doing those first. if we find the one xp bot has
renderer crashes then we can spend time on this.

https://codereview.chromium.org/1291553003/diff/1/content/renderer/renderer_m...
File content/renderer/renderer_main.cc (left):

https://codereview.chromium.org/1291553003/diff/1/content/renderer/renderer_m...
content/renderer/renderer_main.cc:192: // For convenience, we print the stack
traces for crashes.  When sandbox
On 2015/08/12 02:28:14, scottmg (returning - slow) wrote:
> is the CFI_ENFORCEMENT important? (I have no idea what that is)

it's some clang thing: http://clang.llvm.org/docs/ControlFlowIntegrity.html

looks like they wanted stack traces to be printed if it was defined and not
official. now it's not official, so it seemed like no loss of functionality.

[jln (very slow on Chromium), 2015-08-12 16:47:31]

lgtm, but could this be temporary until the current flakiness stops?

[jam, 2015-08-12 17:14:58]

On 2015/08/12 16:47:31, jln wrote:
> lgtm, but could this be temporary until the current flakiness stops?

thanks.

i definitely don't think this should be temporary. that just wastes more time
next time this happens.

also, i don't see a reason to have better security in release vs debug builds,
and worse debugability. that seems quite confusing.

[jam, 2015-08-13 14:35:51]

scottmg: ptal patchset 1->2. There are minor differences which now make it pass
on win dbg/rel 32/64. for sample try runs, see

https://codereview.chromium.org/1284083002/#ps100001
32 release:
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...
32 debug:
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_dbg_...
64 release:
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...

There are no 64 bit debug bots, so I verified with swarming manually.

[jam, 2015-08-13 14:41:58]

actually, for component builds, this won't work right now on swarming since for
GYP we manually list the pdb of the test binary. since we don't list the other
component dll's pdbs, symbolization doesn't work. however for GN, the pdb list
is generated automatically so it'll work.

[jam, 2015-08-13 15:44:11]

jam@chromium.org changed reviewers:
+ wfh@chromium.org

[jam, 2015-08-13 15:44:12]

+wfh for content/common/sandbox_win.cc

[scottmg, 2015-08-13 15:48:54]

lgtm

https://codereview.chromium.org/1291553003/diff/40001/base/debug/stack_trace_...
File base/debug/stack_trace_win.cc (right):

https://codereview.chromium.org/1291553003/diff/40001/base/debug/stack_trace_...
base/debug/stack_trace_win.cc:147: // Need to initialize symbols early in the
process or else
I don't think this is right here if someone calls
base::debug::StackTrace().Print() in a process that hasn't had
EnableInProcessStackDumping() called. Can you move the symbol init to a function
and then call the here and in SymbolContext() maybe?

[Will Harris, 2015-08-13 15:53:18]

On 2015/08/13 15:44:12, jam wrote:
> +wfh for content/common/sandbox_win.cc

lgtm

[jam, 2015-08-13 15:59:27]

https://codereview.chromium.org/1291553003/diff/40001/base/debug/stack_trace_...
File base/debug/stack_trace_win.cc (right):

https://codereview.chromium.org/1291553003/diff/40001/base/debug/stack_trace_...
base/debug/stack_trace_win.cc:147: // Need to initialize symbols early in the
process or else
On 2015/08/13 15:48:54, scottmg wrote:
> I don't think this is right here if someone calls
> base::debug::StackTrace().Print() in a process that hasn't had
> EnableInProcessStackDumping() called. Can you move the symbol init to a
function
> and then call the here and in SymbolContext() maybe?

Done.

[scottmg, 2015-08-13 16:37:12]

ship it!

lgtm

[jam, 2015-08-13 17:22:12]

The CQ bit was checked by jam@chromium.org

[jam, 2015-08-13 17:22:13]

The patchset sent to the CQ was uploaded after l-g-t-m from jln@chromium.org,
wfh@chromium.org, scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/1291553003/#ps80001
(title: "fix compile")

[commit-bot: I haz the power, 2015-08-13 17:22:26]

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

[commit-bot: I haz the power, 2015-08-13 18:30:30]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-08-13 18:31:15]

Patchset 5 (id:??) landed as
https://crrev.com/8ba532e170befc312e66d032587fa2ad04bac975
Cr-Commit-Position: refs/heads/master@{#343240}

[Ken Rockot(use gerrit already), 2015-08-16 17:36:22]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1299583002/ by rockot@chromium.org.

The reason for reverting is: This is unfortunately breaking Windows 10 in a
major way. I did a local bisect and narrowed it down to this CL.

BUG=521242.

[scottmg, 2015-08-16 23:12:10]

Confirmed ps7 is ok on win81 (tested debug component):
- chrome doesn't spawn any consoles by default
- chrome with --enable-logging works, and I get browser traces
- the new test in content_browsertests passes

So, lgtm.

The only thing that's not working is in chrome when I navigate to about:crash I
don't see the renderer stack. I think this probably works on test binaries
because they're console, not windowed like chrome? But we can address that
separately in a followup as it's still an improvement as-is over what we have.

[jam, 2015-08-16 23:52:55]

On 2015/08/16 23:12:10, scottmg wrote:
> Confirmed ps7 is ok on win81 (tested debug component):
> - chrome doesn't spawn any consoles by default
> - chrome with --enable-logging works, and I get browser traces
> - the new test in content_browsertests passes

Thanks, I had tested this on Win10 release and debug (should have mentioned
this!).

> 
> So, lgtm.
> 
> The only thing that's not working is in chrome when I navigate to about:crash
I
> don't see the renderer stack. I think this probably works on test binaries
> because they're console, not windowed like chrome? But we can address that
> separately in a followup as it's still an improvement as-is over what we have.

[jam, 2015-08-17 02:06:57]

The CQ bit was checked by jam@chromium.org

[jam, 2015-08-17 02:06:57]

The patchset sent to the CQ was uploaded after l-g-t-m from jln@chromium.org,
wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/1291553003/#ps120001
(title: "patch 1295823002 which fixes the console coming up on Win8+ and adds
regression tests")

[commit-bot: I haz the power, 2015-08-17 02:06:59]

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

[jam, 2015-08-17 02:08:28]

The CQ bit was unchecked by jam@chromium.org

[jam, 2015-08-17 03:37:54]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2015-08-17 03:37:57]

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

[commit-bot: I haz the power, 2015-08-17 03:38:22]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-08-17 03:38:47]

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

[Alexander Potapenko, 2015-08-17 13:02:39]

John,

ContentBrowserTest.RendererCrashCallStack started failing under TSan, can you
please take a look?
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20TSan%20Tests...

[jam, 2015-08-17 14:34:24]

On 2015/08/17 13:02:39, Alexander Potapenko wrote:
> John,
> 
> ContentBrowserTest.RendererCrashCallStack started failing under TSan, can you
> please take a look?
>
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20TSan%20Tests...

sure, sorry about that.

[scottmg, 2015-08-17 23:27:40]

On 2015/08/17 14:34:24, jam wrote:
> On 2015/08/17 13:02:39, Alexander Potapenko wrote:
> > John,
> > 
> > ContentBrowserTest.RendererCrashCallStack started failing under TSan, can
you
> > please take a look?
> >
>
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20TSan%20Tests...
> 
> sure, sorry about that.

Xref for NaCl maybe failing on startup because of this change
https://code.google.com/p/chromium/issues/detail?id=521820

[inferno, 2015-10-01 12:19:06]

This has broken ASAN on Windows, see example trace in
https://code.google.com/p/chromium/issues/detail?id=533350#c17

[Sébastien Marchand, 2016-01-06 21:42:53]

sebmarchand@chromium.org changed reviewers:
+ sebmarchand@chromium.org

[Sébastien Marchand, 2016-01-06 21:42:54]

https://codereview.chromium.org/1291553003/diff/120001/base/debug/stack_trace...
File base/debug/stack_trace_win.cc (right):

https://codereview.chromium.org/1291553003/diff/120001/base/debug/stack_trace...
base/debug/stack_trace_win.cc:57: if (!SymInitialize(GetCurrentProcess(), NULL,
TRUE)) {
AFAIK the calls to SymInitialize should be paired with a call to SymCleanup to
avoid leaking resources. Not sure if it really matters here but I wanted to make
sure that it hasn't been overlooked.

Context: I'm rolling base/ deps in the Syzygy repo, and this is breaking all the
tests that were taking care of calling SymInitialize (you can only call it once
during a process lifetime). I'm trying to fix this in our codebase (i.e. stop
calling this function directly in our tests). This will probably trigger a bunch
of AppVerifier warning because we're leaking some resources. I know that
supporting third party project isn't a goal of base, but I just wanted to make
sure that it hasn't been overlooked.

[brucedawson, 2016-04-18 23:06:59]

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

[brucedawson, 2016-04-18 23:07:00]

In addition to the quirky LoadLibrary call I've noticed some odd behavior in
64-bit Chrome builds caused by the 32-bit version of dbghelp.dll (deleted in
crrev.com/1857413002, but still sticking around for a while in build
directories). When mini_installer is built and run then
EnableProcessStackDumping tries to load dbghelp.dll and then fails because it
finds a 32-bit dbghelpDLL. This means that no renderers can be created.

This would have been noticed earlier but for some reason when I run 64-bit
Chrome from my build directory Windows loads a 64-bit dbghelp.dll from
C:\Windows\System.

I'm not sure if we need to more aggressively remove dbghelp.dll from output
directories or if they will go away on their own quickly enough.

I've worked around this locally by deleting out\Debug_gn_64\dbghelp.dll

https://codereview.chromium.org/1291553003/diff/120001/content/app/content_ma...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1291553003/diff/120001/content/app/content_ma...
content/app/content_main_runner.cc:206: LoadLibraryA("dbghelp.dll");
What is the purpose of this LoadLibrary call? The EnableInProcessStackDumping
function calls SymInitialize which loads dbghelp.dll - is this unintentionally
out-of-order or is supposed to increase the reference count?