Make the SandboxSymbolizeHelper Singleton Leaky and Enable Impacted Test

Make the SandboxSymbolizeHelper Singleton Leaky and Enable Impacted Test

If the SandboxSymbolizeHelper Singleton is obtained on a non-joinable
thread, that singleton itself will assert and attempt to symbolize the
current stack, resulting in another request to get the
SandboxSymbolizeHelper Singleton, ultimately resulting in a stack
overflow.

Since the symbolizer is expected to last the entire process, it can be
leaky.

TaskSchedulerTaskTrackerTest SingletonAllowed Tests on POSIX can be run
after this fix, so it's enabled here as well.

BUG=634552
Committed: https://crrev.com/63f454db7093f1f578fc04632c5f5423eba14cd3
Cr-Commit-Position: refs/heads/master@{#410792}

[robliao, 2016-08-09 00:18:33]

Description was changed from

==========
Make the SandboxSymbolizeHelper Singleton Leaky

If the SandboxSymbolizeHelper Singleton is obtained on a non-joinable thread,
that singleton itself will assert and attempt to symbolize the current stack,
resulting in another request to get the SandboxSymbolizeHelper Singleton,
ultimately resulting in a stack overflow.

Since the symbolizer is expected to last the entire process, it can be leaky.

BUG=634552
==========

to

==========
Make the SandboxSymbolizeHelper Singleton Leaky

If the SandboxSymbolizeHelper Singleton is obtained on a non-joinable 
thread, that singleton itself will assert and attempt to symbolize the
current stack, resulting in another request to get the 
SandboxSymbolizeHelper Singleton, ultimately resulting in a stack 
overflow.

Since the symbolizer is expected to last the entire process, it can be
leaky.

BUG=634552
==========

[robliao, 2016-08-09 00:18:38]

The CQ bit was checked by robliao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 00:19:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 00:27:29]

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

[commit-bot: I haz the power, 2016-08-09 00:27:30]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[robliao, 2016-08-09 00:28:04]

The CQ bit was checked by robliao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 00:28:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[robliao, 2016-08-09 00:29:34]

robliao@chromium.org changed reviewers:
+ thestig@chromium.org

[robliao, 2016-08-09 00:29:35]

thestig: Please review this change. Thanks!

[commit-bot: I haz the power, 2016-08-09 00:36:21]

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

[commit-bot: I haz the power, 2016-08-09 00:36:21]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Lei Zhang, 2016-08-09 00:39:28]

lgtm

https://codereview.chromium.org/2221063004/diff/1/base/debug/stack_trace_posi...
File base/debug/stack_trace_posix.cc (right):

https://codereview.chromium.org/2221063004/diff/1/base/debug/stack_trace_posi...
base/debug/stack_trace_posix.cc:447: }  // namespace
BTW, this can be moved down to just abouve EnableInProcessStackDumping() to put
SandboxSymbolizeHelper in the anonymous namespace as well.

[gab, 2016-08-09 16:31:06]

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

[gab, 2016-08-09 16:31:06]

lgtm, thanks

Also remove #if !defined(OS_LINUX) in TaskSchedulerTaskTrackerTest associated
with this bug.

[robliao, 2016-08-09 18:05:02]

The CQ bit was checked by robliao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 18:05:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[robliao, 2016-08-09 18:06:52]

>Also remove #if !defined(OS_LINUX) in TaskSchedulerTaskTrackerTest associated
with this bug.

Done.
https://codereview.chromium.org/2229873002/

https://codereview.chromium.org/2221063004/diff/1/base/debug/stack_trace_posi...
File base/debug/stack_trace_posix.cc (right):

https://codereview.chromium.org/2221063004/diff/1/base/debug/stack_trace_posi...
base/debug/stack_trace_posix.cc:447: }  // namespace
On 2016/08/09 00:39:28, Lei Zhang wrote:
> BTW, this can be moved down to just abouve EnableInProcessStackDumping() to
put
> SandboxSymbolizeHelper in the anonymous namespace as well.

Done

[gab, 2016-08-09 18:07:47]

On 2016/08/09 18:06:52, robliao wrote:
> >Also remove #if !defined(OS_LINUX) in TaskSchedulerTaskTrackerTest associated
> with this bug.
> 
> Done.
> https://codereview.chromium.org/2229873002/

I think this should be done in this CL per being exactly related

[robliao, 2016-08-09 18:21:43]

On 2016/08/09 18:07:47, gab wrote:
> On 2016/08/09 18:06:52, robliao wrote:
> > >Also remove #if !defined(OS_LINUX) in TaskSchedulerTaskTrackerTest
associated
> > with this bug.
> > 
> > Done.
> > https://codereview.chromium.org/2229873002/
> 
> I think this should be done in this CL per being exactly related

My preference is to keep the patchset topical. It's not immediately obvious to a
casual reader why enabling the unit test is part of this change.
The other patchset has a dependent patchset, so the chain will be preserved
there.

[commit-bot: I haz the power, 2016-08-09 19:13:26]

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

[commit-bot: I haz the power, 2016-08-09 19:13:27]

Dry run: This issue passed the CQ dry run.

[gab, 2016-08-09 19:19:58]

On 2016/08/09 18:21:43, robliao wrote:
> On 2016/08/09 18:07:47, gab wrote:
> > On 2016/08/09 18:06:52, robliao wrote:
> > > >Also remove #if !defined(OS_LINUX) in TaskSchedulerTaskTrackerTest
> associated
> > > with this bug.
> > > 
> > > Done.
> > > https://codereview.chromium.org/2229873002/
> > 
> > I think this should be done in this CL per being exactly related
> 
> My preference is to keep the patchset topical. It's not immediately obvious to
a
> casual reader why enabling the unit test is part of this change.
> The other patchset has a dependent patchset, so the chain will be preserved
> there.

I strongly prefer to have them together. I think it's obvious why the changes go
together because the ifdef being removed has a comment referring to the bug
fixed by this CL.

I think it's important to do them at the same time because say this CL didn't
actually fix the problem, then it wouldn't pass CQ, whereas if landing
separately you would only realize when landing the next one and potentially need
a third CL...

[robliao, 2016-08-09 19:22:23]

The CQ bit was checked by robliao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 19:23:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[robliao, 2016-08-09 19:24:10]

Description was changed from

==========
Make the SandboxSymbolizeHelper Singleton Leaky

If the SandboxSymbolizeHelper Singleton is obtained on a non-joinable 
thread, that singleton itself will assert and attempt to symbolize the
current stack, resulting in another request to get the 
SandboxSymbolizeHelper Singleton, ultimately resulting in a stack 
overflow.

Since the symbolizer is expected to last the entire process, it can be
leaky.

BUG=634552
==========

to

==========
Make the SandboxSymbolizeHelper Singleton Leaky and Enable Impacted Test

If the SandboxSymbolizeHelper Singleton is obtained on a non-joinable 
thread, that singleton itself will assert and attempt to symbolize the
current stack, resulting in another request to get the 
SandboxSymbolizeHelper Singleton, ultimately resulting in a stack 
overflow.

Since the symbolizer is expected to last the entire process, it can be
leaky.

TaskSchedulerTaskTrackerTest SingletonAllowed Tests on POSIX can be run
after this fix, so it's enabled here as well.

BUG=634552
==========

[robliao, 2016-08-09 19:24:34]

On 2016/08/09 19:19:58, gab wrote:
> On 2016/08/09 18:21:43, robliao wrote:
> > On 2016/08/09 18:07:47, gab wrote:
> > > On 2016/08/09 18:06:52, robliao wrote:
> > > > >Also remove #if !defined(OS_LINUX) in TaskSchedulerTaskTrackerTest
> > associated
> > > > with this bug.
> > > > 
> > > > Done.
> > > > https://codereview.chromium.org/2229873002/
> > > 
> > > I think this should be done in this CL per being exactly related
> > 
> > My preference is to keep the patchset topical. It's not immediately obvious
to
> a
> > casual reader why enabling the unit test is part of this change.
> > The other patchset has a dependent patchset, so the chain will be preserved
> > there.
> 
> I strongly prefer to have them together. I think it's obvious why the changes
go
> together because the ifdef being removed has a comment referring to the bug
> fixed by this CL.
> 
> I think it's important to do them at the same time because say this CL didn't
> actually fix the problem, then it wouldn't pass CQ, whereas if landing
> separately you would only realize when landing the next one and potentially
need
> a third CL...

Done. Amended CL description to accommodate.

[commit-bot: I haz the power, 2016-08-09 20:10:26]

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

[commit-bot: I haz the power, 2016-08-09 20:10:27]

Dry run: This issue passed the CQ dry run.

[robliao, 2016-08-09 20:11:58]

The CQ bit was checked by robliao@chromium.org

[robliao, 2016-08-09 20:11:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2221063004/#ps40001
(title: "Add In Task Tracker Unit Test Enable")

[commit-bot: I haz the power, 2016-08-09 20:12:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 20:18:09]

Description was changed from

==========
Make the SandboxSymbolizeHelper Singleton Leaky and Enable Impacted Test

If the SandboxSymbolizeHelper Singleton is obtained on a non-joinable 
thread, that singleton itself will assert and attempt to symbolize the
current stack, resulting in another request to get the 
SandboxSymbolizeHelper Singleton, ultimately resulting in a stack 
overflow.

Since the symbolizer is expected to last the entire process, it can be
leaky.

TaskSchedulerTaskTrackerTest SingletonAllowed Tests on POSIX can be run
after this fix, so it's enabled here as well.

BUG=634552
==========

to

==========
Make the SandboxSymbolizeHelper Singleton Leaky and Enable Impacted Test

If the SandboxSymbolizeHelper Singleton is obtained on a non-joinable 
thread, that singleton itself will assert and attempt to symbolize the
current stack, resulting in another request to get the 
SandboxSymbolizeHelper Singleton, ultimately resulting in a stack 
overflow.

Since the symbolizer is expected to last the entire process, it can be
leaky.

TaskSchedulerTaskTrackerTest SingletonAllowed Tests on POSIX can be run
after this fix, so it's enabled here as well.

BUG=634552
==========

[commit-bot: I haz the power, 2016-08-09 20:18:11]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-09 20:21:44]

Description was changed from

==========
Make the SandboxSymbolizeHelper Singleton Leaky and Enable Impacted Test

If the SandboxSymbolizeHelper Singleton is obtained on a non-joinable 
thread, that singleton itself will assert and attempt to symbolize the
current stack, resulting in another request to get the 
SandboxSymbolizeHelper Singleton, ultimately resulting in a stack 
overflow.

Since the symbolizer is expected to last the entire process, it can be
leaky.

TaskSchedulerTaskTrackerTest SingletonAllowed Tests on POSIX can be run
after this fix, so it's enabled here as well.

BUG=634552
==========

to

==========
Make the SandboxSymbolizeHelper Singleton Leaky and Enable Impacted Test

If the SandboxSymbolizeHelper Singleton is obtained on a non-joinable
thread, that singleton itself will assert and attempt to symbolize the
current stack, resulting in another request to get the
SandboxSymbolizeHelper Singleton, ultimately resulting in a stack
overflow.

Since the symbolizer is expected to last the entire process, it can be
leaky.

TaskSchedulerTaskTrackerTest SingletonAllowed Tests on POSIX can be run
after this fix, so it's enabled here as well.

BUG=634552

Committed: https://crrev.com/63f454db7093f1f578fc04632c5f5423eba14cd3
Cr-Commit-Position: refs/heads/master@{#410792}
==========

[commit-bot: I haz the power, 2016-08-09 20:21:45]

Patchset 3 (id:??) landed as
https://crrev.com/63f454db7093f1f578fc04632c5f5423eba14cd3
Cr-Commit-Position: refs/heads/master@{#410792}