Compile in Breakpad by default on Linux

Compile Breakpad into Chromium by default on Linux

This brings the Linux build into line with the Windows and Mac builds,
where Breakpad is compiled in by default.  This allows us to test
Breakpad on the trybots and buildbots.

It's still possible to omit Breakpad support using a Gyp option.

As with Windows and Mac, we don't want to enable Breakpad in Chromium
at run time by default (since that would spam the crash server, which
doesn't have symbols for Chromium builds anyway), so we put this
behind a run time flag.  We also don't compile in debug info (-g) by
default.

We extend NaCl's Breakpad tests to be able to locate crash dumps on
Linux.

We add some "#ifdef GOOGLE_CHROME_BUILD"s to prevent Breakpad from
being enabled accidentally without the run time flag, but this is just
in case because there should be no GUI option for enabling stats/crash
reporting inside non-Chrome Chromium builds.

BUG=105778
TEST=breakpad_browser_process_crash_test in nacl_integration

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176911

[Mark Seaborn, 2013-01-09 21:31:23]

@ncbray for ppapi/nativeclient
@thestig for everything else

[Lei Zhang, 2013-01-09 22:36:49]

Keep in mind this switches Breakpad on for CrOS and Android as well.

jcivelli: Can you take a look at this from an Android perspective?
pastarmovj: Is the USE_LINUX_BREAKPAD section in
chrome/browser/chromeos/login/wizard_controller.cc ok with this CL?

https://codereview.chromium.org/10407058/diff/8002/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main_linux.cc (right):

https://codereview.chromium.org/10407058/diff/8002/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main_linux.cc:66: bool breakpad_enabled =
I'm concerned that a Chromium build can have this pref set, for whatever reason.
With this CL, the Chromium build will start sending crash reports. We should set
this to false for Chromium builds.

For buildbots, it shouldn't matter, since they will enable Breakpad via the
mechanisms below.

https://codereview.chromium.org/10407058/diff/8002/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main_linux.cc:85: breakpad_enabled =
getenv("CHROME_ENABLE_BREAKPAD") != NULL;
Likely some other pieces of C++ code will use this, so put it in
chrome/common/env_vars.h?

[Nick Bray (chromium), 2013-01-09 23:09:56]

LGTM for the NaCl part of the CL, but that's the simple part.

https://codereview.chromium.org/10407058/diff/8002/ppapi/native_client/tests/...
File ppapi/native_client/tests/breakpad_crash_test/crash_dump_tester.py (right):

https://codereview.chromium.org/10407058/diff/8002/ppapi/native_client/tests/...
ppapi/native_client/tests/breakpad_crash_test/crash_dump_tester.py:136: elif
sys.platform == 'linux2':
sys.platform.startwith('linux')? x2

[Mark Seaborn, 2013-01-10 00:48:28]

https://codereview.chromium.org/10407058/diff/8002/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main_linux.cc (right):

https://codereview.chromium.org/10407058/diff/8002/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main_linux.cc:66: bool breakpad_enabled =
On 2013/01/09 22:36:49, Lei Zhang wrote:
> I'm concerned that a Chromium build can have this pref set, for whatever
reason.
> With this CL, the Chromium build will start sending crash reports. We should
set
> this to false for Chromium builds.
> 
> For buildbots, it shouldn't matter, since they will enable Breakpad via the
> mechanisms below.

To clarify, is your concern about when Chromium is run as normal desktop app, or
about when Chromium code is run by tests on the bots that might enable metrics
reporting (e.g. to test UMA) but that doesn't intend to enable Breakpad?

I've made this code conditional.

https://codereview.chromium.org/10407058/diff/8002/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main_linux.cc:85: breakpad_enabled =
getenv("CHROME_ENABLE_BREAKPAD") != NULL;
On 2013/01/09 22:36:49, Lei Zhang wrote:
> Likely some other pieces of C++ code will use this, so put it in
> chrome/common/env_vars.h?

OK, done.

https://codereview.chromium.org/10407058/diff/8002/ppapi/native_client/tests/...
File ppapi/native_client/tests/breakpad_crash_test/crash_dump_tester.py (right):

https://codereview.chromium.org/10407058/diff/8002/ppapi/native_client/tests/...
ppapi/native_client/tests/breakpad_crash_test/crash_dump_tester.py:136: elif
sys.platform == 'linux2':
On 2013/01/09 23:09:56, Nick Bray (chromium) wrote:
> sys.platform.startwith('linux')? x2

OK, done.

[Lei Zhang, 2013-01-10 00:58:49]

Looking good. Let's see what the Android/CrOS folks say.

https://codereview.chromium.org/10407058/diff/8002/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main_linux.cc (right):

https://codereview.chromium.org/10407058/diff/8002/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main_linux.cc:66: bool breakpad_enabled =
On 2013/01/10 00:48:28, Mark Seaborn wrote:
> On 2013/01/09 22:36:49, Lei Zhang wrote:
> > I'm concerned that a Chromium build can have this pref set, for whatever
> reason.
> > With this CL, the Chromium build will start sending crash reports. We should
> set
> > this to false for Chromium builds.
> > 
> > For buildbots, it shouldn't matter, since they will enable Breakpad via the
> > mechanisms below.
> 
> To clarify, is your concern about when Chromium is run as normal desktop app,
or
> about when Chromium code is run by tests on the bots that might enable metrics
> reporting (e.g. to test UMA) but that doesn't intend to enable Breakpad?
> 
> I've made this code conditional.

My concern is about users running it on their desktop. I doubt bots will have
metrics enabled.

[Mark Seaborn, 2013-01-10 01:23:02]

https://codereview.chromium.org/10407058/diff/8002/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main_linux.cc (right):

https://codereview.chromium.org/10407058/diff/8002/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main_linux.cc:66: bool breakpad_enabled =
On 2013/01/10 00:58:49, Lei Zhang wrote:
> On 2013/01/10 00:48:28, Mark Seaborn wrote:
> > On 2013/01/09 22:36:49, Lei Zhang wrote:
> > > I'm concerned that a Chromium build can have this pref set, for whatever
> > > reason.
> > > With this CL, the Chromium build will start sending crash reports. We
> > > should set this to false for Chromium builds.
> > > 
> > > For buildbots, it shouldn't matter, since they will enable Breakpad via
the
> > > mechanisms below.
> > 
> > To clarify, is your concern about when Chromium is run as normal desktop
> > app, or about when Chromium code is run by tests on the bots that might
> > enable metrics reporting (e.g. to test UMA) but that doesn't intend to
> > enable Breakpad?
> > 
> > I've made this code conditional.
> 
> My concern is about users running it on their desktop. I doubt bots will have
> metrics enabled.

Oh, I thought you'd say the opposite.  I'm not so worried about the case of
users running Chromium on their desktops, because the UMA/Breakpad option is not
supposed to be displayed in Chromium.  If crash reporting is turned on without
the user seeing the option, there are bigger problems, because crash reporting
is only supposed to happen if the user consents.

[Jay Civelli, 2013-01-10 17:56:27]

You need to update chrome_browser_main_android.cc as well.

[Mark Seaborn, 2013-01-10 19:38:31]

On 2013/01/10 17:56:27, Jay Civelli wrote:
> You need to update chrome_browser_main_android.cc as well.

Done.  I have made it check CHROME_ENABLE_BREAKPAD for consistency with the
non-Android case, although there aren't any tests that need this yet.

[Jay Civelli, 2013-01-11 18:54:55]

lgtm

[Lei Zhang, 2013-01-11 20:21:16]

lgtm

https://codereview.chromium.org/10407058/diff/8005/chrome/common/env_vars.cc
File chrome/common/env_vars.cc (right):

https://codereview.chromium.org/10407058/diff/8005/chrome/common/env_vars.cc#...
chrome/common/env_vars.cc:9: // Enable Breakpad crash reporting.
nit: Can you mention this is used for automated testing?

[Mark Seaborn, 2013-01-11 20:34:52]

https://codereview.chromium.org/10407058/diff/8005/chrome/common/env_vars.cc
File chrome/common/env_vars.cc (right):

https://codereview.chromium.org/10407058/diff/8005/chrome/common/env_vars.cc#...
chrome/common/env_vars.cc:9: // Enable Breakpad crash reporting.
On 2013/01/11 20:21:16, Lei Zhang wrote:
> nit: Can you mention this is used for automated testing?

Done.

[Mark Seaborn, 2013-01-12 00:28:55]

On 2013/01/09 22:36:49, Lei Zhang wrote:
> pastarmovj: Is the USE_LINUX_BREAKPAD section in
> chrome/browser/chromeos/login/wizard_controller.cc ok with this CL?

OK, I have added an additional "#ifdef GOOGLE_CHROME_BUILD" to that file just in
case, to prevent crash reporting accidentally being turned on there in a
non-Chrome Chromium OS build.  It didn't seem worthwhile to check for the
CHROME_ENABLE_BREAKPAD env var there.

@pastarmovj/thestig: can either of you OK this part?

[Mark Seaborn, 2013-01-15 01:52:39]

On 2013/01/12 00:28:55, Mark Seaborn wrote:
> On 2013/01/09 22:36:49, Lei Zhang wrote:
> > pastarmovj: Is the USE_LINUX_BREAKPAD section in
> > chrome/browser/chromeos/login/wizard_controller.cc ok with this CL?
> 
> OK, I have added an additional "#ifdef GOOGLE_CHROME_BUILD" to that file just
in
> case, to prevent crash reporting accidentally being turned on there in a
> non-Chrome Chromium OS build.  It didn't seem worthwhile to check for the
> CHROME_ENABLE_BREAKPAD env var there.
> 
> @pastarmovj/thestig: can either of you OK this part?

+dpolukhin, since you're in OWNERS for chrome/browser/chromeos/login, can you
look at the change to wizard_controller.cc, please?

[dpolukhin, 2013-01-15 06:29:09]

LGTM for changes in wizard_controller.cc

[commit-bot: I haz the power, 2013-01-15 12:01:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mseaborn@chromium.org/10407058/31001

[commit-bot: I haz the power, 2013-01-15 13:21:07]

Retried try job too often on mac_rel for step(s) unit_tests