Enable Runtime Memory Leak Detector on x86_64 only

Enable Runtime Memory Leak Detector on x86_64 only

The leak detector requires Chrome to be compiled with frame pointer,
which is the case for Chrome OS x86_64. But not on 32-bit x86 or ARM.

BUG=chromium:605320
TEST=build successfully. Add a CHECK(false) to leak detector and build
for x86_64 and ARM. It should crash on x86_64 but not on ARM.

R=asvitkine@chromium.org, isherman@chromium.org

Committed: https://crrev.com/961ce9c8e5f7aae2685286356c2130373ba14fe0
Cr-Commit-Position: refs/heads/master@{#388694}

[Simon Que, 2016-04-20 22:51:44]

I'm not sure whether this is the correct place for this code. I could see it
going into leak_detector_controller.cc as well, or into
components/metrics/leak_detector/leak_detector.cc.

[Ilya Sherman, 2016-04-20 23:18:03]

LGTM

On 2016/04/20 22:51:44, Simon Que wrote:
> I'm not sure whether this is the correct place for this code. I could see it
> going into leak_detector_controller.cc as well, or into
> components/metrics/leak_detector/leak_detector.cc.

Here seems good, because it allows you to only test the field trial state for
potentially eligible users.  This has the advantage of not tagging ineligible
users as having participated in the field trial, which often makes metrics
analysis easier down the line.  Even if you don't think you're likely to look at
field trial-split metrics for this study, it's useful to set things up this way,
in case you develop an interest in such an analysis later on.

[Simon Que, 2016-04-20 23:34:02]

On 2016/04/20 23:18:03, Ilya Sherman wrote:
> LGTM
> 
> On 2016/04/20 22:51:44, Simon Que wrote:
> > I'm not sure whether this is the correct place for this code. I could see it
> > going into leak_detector_controller.cc as well, or into
> > components/metrics/leak_detector/leak_detector.cc.
> 
> Here seems good, because it allows you to only test the field trial state for
> potentially eligible users.  This has the advantage of not tagging ineligible
> users as having participated in the field trial, which often makes metrics
> analysis easier down the line.  Even if you don't think you're likely to look
at
> field trial-split metrics for this study, it's useful to set things up this
way,
> in case you develop an interest in such an analysis later on.

On a non-x86_64 system, how would the field trial know that this feature was
skipped over? This code is only disabling the check
"base::FeatureList::IsEnabled(features::kRuntimeMemoryLeakDetector)" but IIUC,
the variations system may have already designated this system as a
leak-detector-enabled system.

I was planning to filter UMA reports using the
SystemProfile::Hardware::cpu_architecture field.

[Simon Que, 2016-04-21 04:04:02]

The CQ bit was checked by sque@chromium.org

[commit-bot: I haz the power, 2016-04-21 04:04:09]

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

[commit-bot: I haz the power, 2016-04-21 04:43:07]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-04-22 19:31:35]

Description was changed from

==========
Enable Runtime Memory Leak Detector on x86_64 only

The leak detector requires Chrome to be compiled with frame pointer,
which is the case for Chrome OS x86_64. But not on 32-bit x86 or ARM.

BUG=chromium:605320
TEST=build successfully. Add a CHECK(false) to leak detector and build
for x86_64 and ARM. It should crash on x86_64 but not on ARM.

R=asvitkine@chromium.org, isherman@chromium.org
==========

to

==========
Enable Runtime Memory Leak Detector on x86_64 only

The leak detector requires Chrome to be compiled with frame pointer,
which is the case for Chrome OS x86_64. But not on 32-bit x86 or ARM.

BUG=chromium:605320
TEST=build successfully. Add a CHECK(false) to leak detector and build
for x86_64 and ARM. It should crash on x86_64 but not on ARM.

R=asvitkine@chromium.org, isherman@chromium.org

Committed: https://crrev.com/961ce9c8e5f7aae2685286356c2130373ba14fe0
Cr-Commit-Position: refs/heads/master@{#388694}
==========

[commit-bot: I haz the power, 2016-04-22 19:31:36]

Patchset 1 (id:??) landed as
https://crrev.com/961ce9c8e5f7aae2685286356c2130373ba14fe0
Cr-Commit-Position: refs/heads/master@{#388694}

[Ilya Sherman, 2016-04-25 18:46:38]

On 2016/04/20 23:34:02, Simon Que wrote:
> On 2016/04/20 23:18:03, Ilya Sherman wrote:
> > LGTM
> > 
> > On 2016/04/20 22:51:44, Simon Que wrote:
> > > I'm not sure whether this is the correct place for this code. I could see
it
> > > going into leak_detector_controller.cc as well, or into
> > > components/metrics/leak_detector/leak_detector.cc.
> > 
> > Here seems good, because it allows you to only test the field trial state
for
> > potentially eligible users.  This has the advantage of not tagging
ineligible
> > users as having participated in the field trial, which often makes metrics
> > analysis easier down the line.  Even if you don't think you're likely to
look
> at
> > field trial-split metrics for this study, it's useful to set things up this
> way,
> > in case you develop an interest in such an analysis later on.
> 
> On a non-x86_64 system, how would the field trial know that this feature was
> skipped over? This code is only disabling the check
> "base::FeatureList::IsEnabled(features::kRuntimeMemoryLeakDetector)" but IIUC,
> the variations system may have already designated this system as a
> leak-detector-enabled system.

Field trials are only reported as enabled when there is some code that queries
for the enabled state.  This is precisely to prevent users who don't interact
with the feature from confusing the data.  So, prior to the
base::FeatureList::IsEnabled() call, the variations service is prepared to
assign the system to the enabled state, but it doesn't actually make the
assignment until that method is called.