[SyzyAsan] Enable Crashpad reporter as a 50/50 experiment.

[SyzyAsan] Enable Crashpad reporter as a 50/50 experiment.

BUG=

Committed: https://github.com/google/syzygy/commit/858cdc69baa55a673fcad7ff294c864891d89128

[chrisha, 2016-05-18 14:03:06]

chrisha@chromium.org changed reviewers:
+ siggi@chromium.org

[chrisha, 2016-05-18 14:03:07]

PTAL?

[Sébastien Marchand, 2016-05-18 14:16:16]

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

[Sébastien Marchand, 2016-05-18 14:16:17]

You also need to update the unittests:
https://github.com/google/syzygy/blob/master/syzygy/agent/asan/runtime_unitte...

[Sigurður Ásgeirsson, 2016-05-18 14:24:51]

https://codereview.chromium.org/1992773002/diff/1/syzygy/agent/asan/runtime.cc
File syzygy/agent/asan/runtime.cc (right):

https://codereview.chromium.org/1992773002/diff/1/syzygy/agent/asan/runtime.c...
syzygy/agent/asan/runtime.cc:515: GenerateRandomFeatureSet();
uh??

[Sigurður Ásgeirsson, 2016-05-18 14:26:16]

https://codereview.chromium.org/1992773002/diff/1/syzygy/agent/asan/runtime.cc
File syzygy/agent/asan/runtime.cc (right):

https://codereview.chromium.org/1992773002/diff/1/syzygy/agent/asan/runtime.c...
syzygy/agent/asan/runtime.cc:515: GenerateRandomFeatureSet();
On 2016/05/18 14:24:51, Sigurður Ásgeirsson wrote:
> uh??

That is to say - this looks like a noop to me?

[chrisha, 2016-05-18 14:32:57]

https://codereview.chromium.org/1992773002/diff/1/syzygy/agent/asan/runtime.cc
File syzygy/agent/asan/runtime.cc (right):

https://codereview.chromium.org/1992773002/diff/1/syzygy/agent/asan/runtime.c...
syzygy/agent/asan/runtime.cc:515: GenerateRandomFeatureSet();
On 2016/05/18 14:26:16, Sigurður Ásgeirsson wrote:
> On 2016/05/18 14:24:51, Sigurður Ásgeirsson wrote:
> > uh??
> 
> That is to say - this looks like a noop to me?

Indeed it is. Cut and paste error.

Also, as Seb pointed out, I totally spaced on writing the unittest. Ignore this
for now :)

[chrisha, 2016-05-19 17:58:23]

Okay, now with a little more cleanup and unittest integration. I won't be
landing this until scottmg@ lands another bugfix over in Chrome land.

[Sigurður Ásgeirsson, 2016-05-19 20:25:58]

https://codereview.chromium.org/1992773002/diff/20001/syzygy/agent/asan/error...
File syzygy/agent/asan/error_info.h (right):

https://codereview.chromium.org/1992773002/diff/20001/syzygy/agent/asan/error...
syzygy/agent/asan/error_info.h:87: ASAN_FEATURE_ENABLE_PAGE_PROTECTIONS = (1 <<
0),
is it time to retire the other experiments?

https://codereview.chromium.org/1992773002/diff/20001/syzygy/agent/asan/runti...
File syzygy/agent/asan/runtime.cc (right):

https://codereview.chromium.org/1992773002/diff/20001/syzygy/agent/asan/runti...
syzygy/agent/asan/runtime.cc:392: std::unique_ptr<ReporterInterface>
CreateCrashReporterFromEnvironment(
MaybeCreate?

https://codereview.chromium.org/1992773002/diff/20001/syzygy/agent/asan/runti...
syzygy/agent/asan/runtime.cc:430: if (reporter.get() == nullptr)
this is a noop

https://codereview.chromium.org/1992773002/diff/20001/syzygy/agent/asan/runti...
syzygy/agent/asan/runtime.cc:502: PropagateFeatureSet(feature_set);
I don't follow the logic in this file at all - there seems to be a scattering of
policy decision all over the place, and then piecewise acting on it.
Will this e.g. report the CrashPad experiment if parameters specify
randomization and the environment forces Kasko?

Maybe it makes more sense to consolidate the policy decision somewhere, and to
report the crash reporter used as the group?

https://codereview.chromium.org/1992773002/diff/20001/syzygy/agent/asan/syzya...
File syzygy/agent/asan/syzyasan_rtl.cc (right):

https://codereview.chromium.org/1992773002/diff/20001/syzygy/agent/asan/syzya...
syzygy/agent/asan/syzyasan_rtl.cc:44: {ASAN_FEATURE_ENABLE_CRASHPAD,
"SyzyASANCrashpad"},
you need backend configuration for this as well.

[chrisha, 2016-05-19 20:31:32]

https://codereview.chromium.org/1992773002/diff/20001/syzygy/agent/asan/runti...
File syzygy/agent/asan/runtime.cc (right):

https://codereview.chromium.org/1992773002/diff/20001/syzygy/agent/asan/runti...
syzygy/agent/asan/runtime.cc:502: PropagateFeatureSet(feature_set);
On 2016/05/19 20:25:58, Sigurður Ásgeirsson wrote:
> I don't follow the logic in this file at all - there seems to be a scattering
of
> policy decision all over the place, and then piecewise acting on it.
> Will this e.g. report the CrashPad experiment if parameters specify
> randomization and the environment forces Kasko?
> 
> Maybe it makes more sense to consolidate the policy decision somewhere, and to
> report the crash reporter used as the group?

I agree, it's scattered and messy. I really don't want to have to refactor this,
and would much rather Seb own that.

The reporting is already separated from all of this code, and the actual crash
reporter in use is the one that will be reported. See 'GetEnabledFeatureSet'
below.

https://codereview.chromium.org/1992773002/diff/20001/syzygy/agent/asan/syzya...
File syzygy/agent/asan/syzyasan_rtl.cc (right):

https://codereview.chromium.org/1992773002/diff/20001/syzygy/agent/asan/syzya...
syzygy/agent/asan/syzyasan_rtl.cc:44: {ASAN_FEATURE_ENABLE_CRASHPAD,
"SyzyASANCrashpad"},
On 2016/05/19 20:25:58, Sigurður Ásgeirsson wrote:
> you need backend configuration for this as well.

Already landed.

[Sigurður Ásgeirsson, 2016-05-20 14:00:17]

https://codereview.chromium.org/1992773002/diff/20001/syzygy/agent/asan/runti...
File syzygy/agent/asan/runtime.cc (right):

https://codereview.chromium.org/1992773002/diff/20001/syzygy/agent/asan/runti...
syzygy/agent/asan/runtime.cc:502: PropagateFeatureSet(feature_set);
On 2016/05/19 20:31:31, chrisha (slow) wrote:
> On 2016/05/19 20:25:58, Sigurður Ásgeirsson wrote:
> > I don't follow the logic in this file at all - there seems to be a
scattering
> of
> > policy decision all over the place, and then piecewise acting on it.
> > Will this e.g. report the CrashPad experiment if parameters specify
> > randomization and the environment forces Kasko?
> > 
> > Maybe it makes more sense to consolidate the policy decision somewhere, and
to
> > report the crash reporter used as the group?
> 
> I agree, it's scattered and messy. I really don't want to have to refactor
this,
> and would much rather Seb own that.
> 

Filed https://github.com/google/syzygy/issues/45 and assigned to Seb.

> The reporting is already separated from all of this code, and the actual crash
> reporter in use is the one that will be reported. See 'GetEnabledFeatureSet'
> below.

That's returning a binary value for a trinary decision :(.

https://codereview.chromium.org/1992773002/diff/20001/syzygy/agent/asan/runti...
syzygy/agent/asan/runtime.cc:1223: enabled_features |=
ASAN_FEATURE_ENABLE_CRASHPAD;
Given I can't follow the code above, I'm not convinced that crash_reporter_
can't be nullptr?
Also, we really have three groups - Kasko, Breakpad, CrashPad?

[chrisha, 2016-05-31 20:45:15]

Another look?

https://codereview.chromium.org/1992773002/diff/20001/syzygy/agent/asan/runti...
File syzygy/agent/asan/runtime.cc (right):

https://codereview.chromium.org/1992773002/diff/20001/syzygy/agent/asan/runti...
syzygy/agent/asan/runtime.cc:392: std::unique_ptr<ReporterInterface>
CreateCrashReporterFromEnvironment(
On 2016/05/19 20:25:58, Sigurður Ásgeirsson wrote:
> MaybeCreate?

Acknowledged.

https://codereview.chromium.org/1992773002/diff/20001/syzygy/agent/asan/runti...
syzygy/agent/asan/runtime.cc:430: if (reporter.get() == nullptr)
On 2016/05/19 20:25:58, Sigurður Ásgeirsson wrote:
> this is a noop

Acknowledged.

https://codereview.chromium.org/1992773002/diff/20001/syzygy/agent/asan/runti...
syzygy/agent/asan/runtime.cc:1223: enabled_features |=
ASAN_FEATURE_ENABLE_CRASHPAD;
On 2016/05/20 14:00:17, Sigurður Ásgeirsson wrote:
> Given I can't follow the code above, I'm not convinced that crash_reporter_
> can't be nullptr?
> Also, we really have three groups - Kasko, Breakpad, CrashPad?

Breakpad can't really happen unless manually overridden, as Kasko is always
present. I could add it, but it felt like overkill. I'd have to rework the
entire experiment exporting logic, which can only handle true false.

Other than via a very specific bit of manual intervention, the crash reporter
can only be one of Crashpad or Kasko.

[chrisha, 2016-05-31 20:45:45]

(Also, confirmed that the Crashpad inflated-crash-dumps fix has landed in
Chrome, and it now generates entirely reasonable crash dumps.)

[chrisha, 2016-05-31 20:45:59]

Description was changed from

==========
[SyzyAsan] Enabled Crashpad reporter as a 50/50 experiment.

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

to

==========
[SyzyAsan] Enable Crashpad reporter as a 50/50 experiment.

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

[Sébastien Marchand, 2016-06-01 16:38:55]

https://codereview.chromium.org/1992773002/diff/60001/syzygy/agent/asan/runti...
File syzygy/agent/asan/runtime.cc (right):

https://codereview.chromium.org/1992773002/diff/60001/syzygy/agent/asan/runti...
syzygy/agent/asan/runtime.cc:515: CrashReporterType crash_reporter_type =
We also have a 'disable_breakpad_reporting ' flag in asan_parameters.h
(https://github.com/google/syzygy/blob/master/syzygy/common/asan_parameters.h)
that should be updated I think ? CF doesn't want to use a crash reporter (it
uses agent_logger to generate the minidumps).

[chrisha, 2016-06-02 17:45:30]

https://codereview.chromium.org/1992773002/diff/60001/syzygy/agent/asan/runti...
File syzygy/agent/asan/runtime.cc (right):

https://codereview.chromium.org/1992773002/diff/60001/syzygy/agent/asan/runti...
syzygy/agent/asan/runtime.cc:515: CrashReporterType crash_reporter_type =
On 2016/06/01 16:38:55, Sébastien Marchand wrote:
> We also have a 'disable_breakpad_reporting ' flag in asan_parameters.h
> (https://github.com/google/syzygy/blob/master/syzygy/common/asan_parameters.h)
> that should be updated I think ? CF doesn't want to use a crash reporter (it
> uses agent_logger to generate the minidumps).

Yup, the flag still exists and works. This line is only getting the preferred
*type* of crash reporter to create. It isn't created until line 537.

https://codereview.chromium.org/1992773002/diff/60001/syzygy/agent/asan/runti...
syzygy/agent/asan/runtime.cc:537: CreateCrashReporter(logger(),
crash_reporter_type).release());
And it won't be created if --disable_breakpad_reporting is specified.

[Sébastien Marchand, 2016-06-02 19:54:18]

lgtm

https://codereview.chromium.org/1992773002/diff/60001/syzygy/agent/asan/repor...
File syzygy/agent/asan/reporters/crashpad_reporter.cc (right):

https://codereview.chromium.org/1992773002/diff/60001/syzygy/agent/asan/repor...
syzygy/agent/asan/reporters/crashpad_reporter.cc:173: // a maximum of 1MB.
What's the actual limit in crash/ ?

[chrisha, 2016-06-02 20:20:18]

Thanks. Any other comments Siggi? Otherwise I'll commit this soon.

https://codereview.chromium.org/1992773002/diff/60001/syzygy/agent/asan/repor...
File syzygy/agent/asan/reporters/crashpad_reporter.cc (right):

https://codereview.chromium.org/1992773002/diff/60001/syzygy/agent/asan/repor...
syzygy/agent/asan/reporters/crashpad_reporter.cc:173: // a maximum of 1MB.
On 2016/06/02 19:54:18, Sébastien Marchand wrote:
> What's the actual limit in crash/ ? 

Bigger than this. The other things in the minidump take already around
500-750KB, so setting this to 1MB keeps crash reports under 2MB, which is
similar to their current sizes. I'll make the comment more detailed.

[Sigurður Ásgeirsson, 2016-06-02 20:26:48]

lgtm with a nit

https://codereview.chromium.org/1992773002/diff/80001/syzygy/agent/asan/runti...
File syzygy/agent/asan/runtime.cc (right):

https://codereview.chromium.org/1992773002/diff/80001/syzygy/agent/asan/runti...
syzygy/agent/asan/runtime.cc:433: std::unique_ptr<ReporterInterface>
CreateCrashReporter(
CreateCrashReporterWithTypeHint or TypePreference?

https://codereview.chromium.org/1992773002/diff/80001/syzygy/agent/asan/runti...
syzygy/agent/asan/runtime.cc:524: if (crash_reporter_type ==
kDefaultCrashReporterType &&
thanks for reworking this - it's practically comprehensible now.

https://codereview.chromium.org/1992773002/diff/80001/syzygy/agent/asan/runti...
syzygy/agent/asan/runtime.cc:536: crash_reporter_.reset(
Except this bit. Maybe CreateCrashReporterWithHint?

[chrisha, 2016-06-02 20:59:33]

Thanks, committing!

https://codereview.chromium.org/1992773002/diff/80001/syzygy/agent/asan/runti...
File syzygy/agent/asan/runtime.cc (right):

https://codereview.chromium.org/1992773002/diff/80001/syzygy/agent/asan/runti...
syzygy/agent/asan/runtime.cc:433: std::unique_ptr<ReporterInterface>
CreateCrashReporter(
On 2016/06/02 20:26:48, Sigurður Ásgeirsson wrote:
> CreateCrashReporterWithTypeHint or TypePreference?

Done.

https://codereview.chromium.org/1992773002/diff/80001/syzygy/agent/asan/runti...
syzygy/agent/asan/runtime.cc:524: if (crash_reporter_type ==
kDefaultCrashReporterType &&
On 2016/06/02 20:26:48, Sigurður Ásgeirsson wrote:
> thanks for reworking this - it's practically comprehensible now.

Acknowledged.

https://codereview.chromium.org/1992773002/diff/80001/syzygy/agent/asan/runti...
syzygy/agent/asan/runtime.cc:536: crash_reporter_.reset(
On 2016/06/02 20:26:48, Sigurður Ásgeirsson wrote:
> Except this bit. Maybe CreateCrashReporterWithHint?

Done.

[chrisha, 2016-06-02 20:59:37]

The CQ bit was checked by chrisha@chromium.org

[chrisha, 2016-06-02 20:59:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
sebmarchand@chromium.org, siggi@chromium.org
Link to the patchset: https://codereview.chromium.org/1992773002/#ps100001
(title: "Address siggi's comments.")

[commit-bot: I haz the power, 2016-06-02 20:59:47]

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

[commit-bot: I haz the power, 2016-06-02 21:35:45]

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

[commit-bot: I haz the power, 2016-06-02 21:35:47]

Try jobs failed on following builders:
  win_rel_try on master.tryserver.client.syzygy (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.syzygy/builders/win_rel_try/bui...)

[chrisha, 2016-06-02 21:45:53]

The CQ bit was checked by chrisha@chromium.org

[chrisha, 2016-06-02 21:45:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
sebmarchand@chromium.org, siggi@chromium.org
Link to the patchset: https://codereview.chromium.org/1992773002/#ps120001
(title: "Fix broken unittest.")

[commit-bot: I haz the power, 2016-06-02 21:45:55]

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

[commit-bot: I haz the power, 2016-06-02 22:18:49]

Description was changed from

==========
[SyzyAsan] Enable Crashpad reporter as a 50/50 experiment.

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

to

==========
[SyzyAsan] Enable Crashpad reporter as a 50/50 experiment.

BUG=

Committed:
https://github.com/google/syzygy/commit/858cdc69baa55a673fcad7ff294c864891d89128
==========

[commit-bot: I haz the power, 2016-06-02 22:18:50]

Committed patchset #7 (id:120001) as
https://github.com/google/syzygy/commit/858cdc69baa55a673fcad7ff294c864891d89128