Make sanitizer signal handlers work again.

Make sanitizer signal handlers work again.

- Forward --disable-in-process-stack-traces to child processes.
- Move --disable-in-process-stack-traces switch to service_manager/embedder.
- Make SetupSignalHandlers in service_manager/embedder/main.cc a noop
  when building in sanitizer configuration and this switch is passed.

BUG=716235
Review-Url: https://codereview.chromium.org/2851653002
Cr-Commit-Position: refs/heads/master@{#468414}
Committed: https://chromium.googlesource.com/chromium/src/+/6a8400e566de50e371877ab02c110b8016a1d780

[Oliver Chang, 2017-04-27 22:42:03]

Description was changed from

==========
Make sanitizer signal handlers work again.

- Forward --disable-in-process-stack-traces to child processes.
- Don't call SetupSignalHandlers in service_manager/embedder/main.cc
  when building in sanitizer configuration.
==========

to

==========
Make sanitizer signal handlers work again.

- Forward --disable-in-process-stack-traces to child processes.
- Don't call SetupSignalHandlers in service_manager/embedder/main.cc
  when building in sanitizer configuration.
==========

[Oliver Chang, 2017-04-27 22:42:04]

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

[Oliver Chang, 2017-04-27 22:46:15]

Description was changed from

==========
Make sanitizer signal handlers work again.

- Forward --disable-in-process-stack-traces to child processes.
- Don't call SetupSignalHandlers in service_manager/embedder/main.cc
  when building in sanitizer configuration.
==========

to

==========
Make sanitizer signal handlers work again.

- Forward --disable-in-process-stack-traces to child processes.
- Don't call SetupSignalHandlers in service_manager/embedder/main.cc
  when building in sanitizer configuration.

BUG=716235
==========

[Oliver Chang, 2017-04-27 22:47:58]

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

[Oliver Chang, 2017-04-27 22:48:38]

PTAL

[commit-bot: I haz the power, 2017-04-27 22:48:49]

Dry run: CQ is trying da patch.

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

[jam, 2017-04-28 00:24:50]

https://codereview.chromium.org/2851653002/diff/1/services/service_manager/em...
File services/service_manager/embedder/main.cc (right):

https://codereview.chromium.org/2851653002/diff/1/services/service_manager/em...
services/service_manager/embedder/main.cc:111: #if !defined(ADDRESS_SANITIZER)
&& !defined(LEAK_SANITIZER) &&  \
can we use the same switch as in content? i..e move it from content/ to
services/service_manager/switches.h, and then have this code and content use it?

that way, knowledge about what breaks asan is on asan bot (through command line
flag) instead of here which is quite removed from it

[commit-bot: I haz the power, 2017-04-28 01:37:21]

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

[commit-bot: I haz the power, 2017-04-28 01:37:21]

Dry run: This issue passed the CQ dry run.

[Oliver Chang, 2017-04-28 16:35:46]

Patchset #2 (id:20001) has been deleted

[Oliver Chang, 2017-04-28 17:01:48]

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

[commit-bot: I haz the power, 2017-04-28 17:02:24]

Dry run: CQ is trying da patch.

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

[Oliver Chang, 2017-04-28 17:03:17]

Description was changed from

==========
Make sanitizer signal handlers work again.

- Forward --disable-in-process-stack-traces to child processes.
- Don't call SetupSignalHandlers in service_manager/embedder/main.cc
  when building in sanitizer configuration.

BUG=716235
==========

to

==========
Make sanitizer signal handlers work again.

- Forward --disable-in-process-stack-traces to child processes.
- Move --disable-in-process-stack-traces switch to service_manager/embedder.
- Make SetupSignalHandlers in service_manager/embedder/main.cc a noop
  when building in sanitizer configuration and this switch is passed.

BUG=716235
==========

[Oliver Chang, 2017-04-28 17:05:07]

https://codereview.chromium.org/2851653002/diff/1/services/service_manager/em...
File services/service_manager/embedder/main.cc (right):

https://codereview.chromium.org/2851653002/diff/1/services/service_manager/em...
services/service_manager/embedder/main.cc:111: #if !defined(ADDRESS_SANITIZER)
&& !defined(LEAK_SANITIZER) &&  \
On 2017/04/28 00:24:50, jam wrote:
> can we use the same switch as in content? i..e move it from content/ to
> services/service_manager/switches.h, and then have this code and content use
it?
> 
> that way, knowledge about what breaks asan is on asan bot (through command
line
> flag) instead of here which is quite removed from it

Done. I put it in service_manager/embedder/switches.h instead since otherwise
there's a dependency from service_manager/embedder to service_manager/ and GN
complains. I hope this is fine?

I kept the sanitizer #ifdef checks in so that in a non-sanitizer build there's
no unintended side effects from disabling in process stack traces.

[commit-bot: I haz the power, 2017-04-28 18:11:14]

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

[commit-bot: I haz the power, 2017-04-28 18:11:15]

Dry run: This issue passed the CQ dry run.

[jam, 2017-05-01 15:37:25]

lgtm with one comment

https://codereview.chromium.org/2851653002/diff/1/services/service_manager/em...
File services/service_manager/embedder/main.cc (right):

https://codereview.chromium.org/2851653002/diff/1/services/service_manager/em...
services/service_manager/embedder/main.cc:111: #if !defined(ADDRESS_SANITIZER)
&& !defined(LEAK_SANITIZER) &&  \
On 2017/04/28 17:05:06, Oliver Chang wrote:
> On 2017/04/28 00:24:50, jam wrote:
> > can we use the same switch as in content? i..e move it from content/ to
> > services/service_manager/switches.h, and then have this code and content use
> it?
> > 
> > that way, knowledge about what breaks asan is on asan bot (through command
> line
> > flag) instead of here which is quite removed from it
> 
> Done. I put it in service_manager/embedder/switches.h instead since otherwise
> there's a dependency from service_manager/embedder to service_manager/ and GN
> complains. I hope this is fine?
> 
> I kept the sanitizer #ifdef checks in so that in a non-sanitizer build there's
> no unintended side effects from disabling in process stack traces.

Even content doesn't have these checks, so I don't think we have anything to
worry about. Let's remove them, since keeping them there means that this code
has knowledge about what breaks *san which is what the switch is trying to
avoid.

[Oliver Chang, 2017-05-01 16:05:08]

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

[Oliver Chang, 2017-05-01 16:05:14]

thanks!

https://codereview.chromium.org/2851653002/diff/1/services/service_manager/em...
File services/service_manager/embedder/main.cc (right):

https://codereview.chromium.org/2851653002/diff/1/services/service_manager/em...
services/service_manager/embedder/main.cc:111: #if !defined(ADDRESS_SANITIZER)
&& !defined(LEAK_SANITIZER) &&  \
On 2017/05/01 15:37:24, jam wrote:
> On 2017/04/28 17:05:06, Oliver Chang wrote:
> > On 2017/04/28 00:24:50, jam wrote:
> > > can we use the same switch as in content? i..e move it from content/ to
> > > services/service_manager/switches.h, and then have this code and content
use
> > it?
> > > 
> > > that way, knowledge about what breaks asan is on asan bot (through command
> > line
> > > flag) instead of here which is quite removed from it
> > 
> > Done. I put it in service_manager/embedder/switches.h instead since
otherwise
> > there's a dependency from service_manager/embedder to service_manager/ and
GN
> > complains. I hope this is fine?
> > 
> > I kept the sanitizer #ifdef checks in so that in a non-sanitizer build
there's
> > no unintended side effects from disabling in process stack traces.
> 
> Even content doesn't have these checks, so I don't think we have anything to
> worry about. Let's remove them, since keeping them there means that this code
> has knowledge about what breaks *san which is what the switch is trying to
> avoid.

Sure, removed.

[commit-bot: I haz the power, 2017-05-01 16:05:29]

Dry run: CQ is trying da patch.

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

[Oliver Chang, 2017-05-01 17:16:15]

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

[commit-bot: I haz the power, 2017-05-01 17:16:41]

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, 2017-05-01 18:25:32]

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

[commit-bot: I haz the power, 2017-05-01 18:25:32]

Dry run: This issue passed the CQ dry run.

[Oliver Chang, 2017-05-01 18:29:23]

Please take a quick look at service_manager/embedder/main.cc again (PS#5). I
moved the command line init to before the SetupSignalHandlers()

On 2017/05/01 16:05:14, Oliver Chang wrote:
> thanks!
> 
>
https://codereview.chromium.org/2851653002/diff/1/services/service_manager/em...
> File services/service_manager/embedder/main.cc (right):
> 
>
https://codereview.chromium.org/2851653002/diff/1/services/service_manager/em...
> services/service_manager/embedder/main.cc:111: #if !defined(ADDRESS_SANITIZER)
> && !defined(LEAK_SANITIZER) &&  \
> On 2017/05/01 15:37:24, jam wrote:
> > On 2017/04/28 17:05:06, Oliver Chang wrote:
> > > On 2017/04/28 00:24:50, jam wrote:
> > > > can we use the same switch as in content? i..e move it from content/ to
> > > > services/service_manager/switches.h, and then have this code and content
> use
> > > it?
> > > > 
> > > > that way, knowledge about what breaks asan is on asan bot (through
command
> > > line
> > > > flag) instead of here which is quite removed from it
> > > 
> > > Done. I put it in service_manager/embedder/switches.h instead since
> otherwise
> > > there's a dependency from service_manager/embedder to service_manager/ and
> GN
> > > complains. I hope this is fine?
> > > 
> > > I kept the sanitizer #ifdef checks in so that in a non-sanitizer build
> there's
> > > no unintended side effects from disabling in process stack traces.
> > 
> > Even content doesn't have these checks, so I don't think we have anything to
> > worry about. Let's remove them, since keeping them there means that this
code
> > has knowledge about what breaks *san which is what the switch is trying to
> > avoid.
> 
> Sure, removed.

[jam, 2017-05-01 21:20:57]

lgtm

[Oliver Chang, 2017-05-01 21:21:52]

The CQ bit was checked by ochang@chromium.org

[commit-bot: I haz the power, 2017-05-01 21:22:39]

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, 2017-05-01 21:28:27]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493673712882060,
"parent_rev": "651848a51fcc175e9a42f588efc3d7d138ccaa36", "commit_rev":
"6a8400e566de50e371877ab02c110b8016a1d780"}

[commit-bot: I haz the power, 2017-05-01 21:28:39]

Description was changed from

==========
Make sanitizer signal handlers work again.

- Forward --disable-in-process-stack-traces to child processes.
- Move --disable-in-process-stack-traces switch to service_manager/embedder.
- Make SetupSignalHandlers in service_manager/embedder/main.cc a noop
  when building in sanitizer configuration and this switch is passed.

BUG=716235
==========

to

==========
Make sanitizer signal handlers work again.

- Forward --disable-in-process-stack-traces to child processes.
- Move --disable-in-process-stack-traces switch to service_manager/embedder.
- Make SetupSignalHandlers in service_manager/embedder/main.cc a noop
  when building in sanitizer configuration and this switch is passed.

BUG=716235

Review-Url: https://codereview.chromium.org/2851653002
Cr-Commit-Position: refs/heads/master@{#468414}
Committed:
https://chromium.googlesource.com/chromium/src/+/6a8400e566de50e371877ab02c11...
==========

[commit-bot: I haz the power, 2017-05-01 21:28:40]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/6a8400e566de50e371877ab02c11...