Breakpad: Compile Breakpad into Chromium by default on Mac

Breakpad: Compile Breakpad into Chromium by default on Mac

This is intended to make it easier to test Breakpad crash dumping on
the buildbots.  Crash dumping will be disabled in Chromium by default
at run time unless CHROME_HEADLESS is set.

The bots set CHROME_HEADLESS, but we don't really want the bots to be
uploading crash dumps that are useless without symbols, so uploading
is disabled except for official builds and when building with
mac_breakpad_uploads=1.

This should not affect stack backtraces that occur in tests (such as
browser_tests) via StackDumpSignalHandler().  Although Breakpad would
normally take priority over this signal handler, Breakpad does not get
enabled in these test suites because is_browser is false inside
InitCrashReporter().

This change leaves the Gyp option "mac_breakpad" for enabling the
creation of Breakpad files.  This remains off by default for Chromium
builds, because the symbol dumping is slow and because it has problems
in the makefile-based Mac builds.

BUG=105778
TEST=run Chromium with --crash-test and CHROME_HEADLESS=1; a *.dmp file
  should appear in ~/Library/Application Support/Chromium/Crash Reports


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114468

[Mark Mentovai, 2011-12-06 22:06:03]

Now that I’m thinking about this further…

I guess this might introduce a behavior change on the bots. Right now, when we
run tests, we catch signals and respond by printing a stack trace before
exiting. With a Breakpad handler in place, this will change, and make bot test
crashes more difficult to debug.

I will LGTM this if it doesn’t impact the test runs in that way, but that’s a
pretty major impediment to development if we lose those stack traces.

[Mark Seaborn, 2011-12-06 23:45:33]

On 6 December 2011 14:06, <mark@chromium.org> wrote:

> Now that I’m thinking about this further…
>
> I guess this might introduce a behavior change on the bots. Right now,
> when we
> run tests, we catch signals and respond by printing a stack trace before
> exiting. With a Breakpad handler in place, this will change, and make bot
> test
> crashes more difficult to debug.
>
> I will LGTM this if it doesn’t impact the test runs in that way, but
> that’s a
> pretty major impediment to development if we lose those stack traces.
>

Are you thinking of the stack traces that are enabled via
base::EnableInProcessStackDumping(), called from base/test/test_suite.cc?
I am not sure how this will interact with Mac Breakpad.

There are also some other places that call PrintBacktrace(), including:
chrome/test/base/in_process_browser_test.cc
content/test/content_browser_test.cc
-- but these only handle SIGTERM.

In the browser process, I think Mac Breakpad is supposed to return control
to the crashing process so that it could print its own stack backtrace (or
let OS X's Crash Reporter run), but it doesn't do this for renderer
processes.

Maybe the bots should not be setting CHROME_HEADLESS to enable Breakpad.
However, this seems to get used on the Windows bots, which have a special
step to look for Breakpad crash dumps and decode them.  This suggests that
Breakpad is getting used in preference to the backtraces from
EnableInProcessStackDumping() on Windows.

Mark

[Mark Mentovai, 2011-12-06 23:59:00]

The StackDumpSignalHandler is exactly what I’m talking about. We need to make
sure that works properly even with Breakpad enabled.

[Mark Seaborn, 2011-12-07 23:44:28]

On 6 December 2011 15:59, <mark@chromium.org> wrote:

> The StackDumpSignalHandler is exactly what I’m talking about. We need to
> make
> sure that works properly even with Breakpad enabled.
>

OK, I did an experiment:  I inserted a crash into
ChromeRenderMessageFilter::OnMessageReceived() and ran try jobs for this
with and without the Mac Breakpad-enabling change.

Without Breakpad enabled:
http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/6542
With Breakpad change:
http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/6567

There seems to be no change in whether a step produces stack backtraces.
safe_browsing_tests, browser_tests and interactive_ui_tests produced stack
backtraces in both cases.  ui_tests did not produce stack backtraces in
either case.

The reason seems to be that although InitCrashReporter() is called in (for
example) browser_tests, it has is_browser == 0, so Breakpad is not enabled.

For comparison, the Windows try runs produce both stack backtraces in the
logs (without meaningful symbol names, i.e. all Chromium code comes out as
"DelayedLowerToken") and Breakpad crash dumps (which are decoded by the
"process_dumps" step, which also lacks symbols):
http://build.chromium.org/p/tryserver.chromium/builders/win/builds/6984
(This makes sense because after in-process Windows Breakpad lets the
out-of-process handler run, it runs the previous in-process
SetUnhandledExceptionFilter() callback function.)

Mark

[Mark Mentovai, 2011-12-07 23:59:01]

OK, then I’ll LGTM this, with the understanding that it may need to be tweaked.
You may want to send a note to chromium-dev saying what you’re doing and
clarifying that Breakpad uploads will remain disabled in exactly the same
situations that Breakpad was disabled in the past.

[commit-bot: I haz the power, 2011-12-08 02:01:50]

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

[commit-bot: I haz the power, 2011-12-08 02:50:28]

Try job failure for 8824003-1006 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Mark Seaborn, 2011-12-08 16:41:24]

On 7 December 2011 18:50, <commit-bot@chromium.org> wrote:

> Try job failure for 8824003-1006 (retry) on mac_rel for step "compile"
> (clobber
> build).
> It's a second try, previously, step "compile" failed.
> http://build.chromium.org/p/**tryserver.chromium/**
>
buildstatus?builder=mac_rel&**number=3649<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=3649>
>
>
http://codereview.chromium.**org/8824003/<http://codereview.chromium.org/8824...
>

It looks like setting mac_breakpad=1 fails with the Mac makefile build when
doing a Release build.  I get the following build failure:

  MACTOOL copy-info-plist chrome/app/helper-Info.plist
  LINK(target) out/Release/Chromium Helper.app/Contents/MacOS/Chromium
Helper
  LINK(target) out/Release/Chromium Helper.app/Contents/MacOS/Chromium
Helper: Finished
DSYMUTIL(helper_app)
STRIP(helper_app)
strip: no files specified
POSTBUILD(helper_app) Make Empty Localizations
POSTBUILD(helper_app) Fix Framework Link
POSTBUILD(helper_app) Tweak Info.plist
POSTBUILD(helper_app) Verify No Objective-C
POSTBUILD(helper_app) Change Mach-O Flags
make: *** [out/Release/Chromium Helper.app] Error 1

The command in the makefile is:

"echo STRIP\\(helper_app\\)" "strip -x -s $(CHROMIUM_STRIP_SAVE_FILE)
$(builddir)/Chromium\\ Helper.app/Contents/MacOS/Chromium\\ Helper"

but for some reason, CHROMIUM_STRIP_SAVE_FILE is not set, and the 4th
argument to 'strip' becomes the 3rd argument.  This is despite the
generated makefile containing:

$(builddir)/Chromium\ Helper.app: export CHROMIUM_STRIP_SAVE_FILE :=
chrome/app/app.saves

This suggests that the makefile build does not work with official Chrome
builds.

Mark

[Mark Seaborn, 2011-12-09 23:17:12]

Since there were problems with dumping Mac Breakpad symbols in the
makefile-based Release build (which is used by the commit queue) -- and since it
is probably not desirable to slow down the Release Chromium builders with
dumping Breakpad symbols -- I have made a further change, to split a
mac_breakpad_symbols option out from mac_breakpad.

This would mean the Chromium build won't dump Breakpad symbols by default. 
Please take another look.

Cheers,
Mark

[Mark Mentovai, 2011-12-09 23:18:55]

LGTM to this too.

[commit-bot: I haz the power, 2011-12-10 02:23:34]

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

[commit-bot: I haz the power, 2011-12-10 02:23:37]

Presubmit check for 8824003-9001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
ppapi/native_client/src/trusted/plugin/plugin.gyp,remoting/remoting.gyp

[Mark Seaborn, 2011-12-10 02:38:45]

Noel: Can you sign off on the plugin.gyp change?

Lambros: Can you sign off on the remoting.gyp change?

Cheers,
Mark

[Mark Seaborn, 2011-12-10 03:40:36]

Mark: I noticed that third_party/ffmpeg/ffmpeg.gyp also checks "mac_breakpad" to
set "mac_real_dsym".  This file is pulled in via DEPS so would require 2 extra
commits to change.  It's not a big problem if it checks mac_breakpad until then:
it will just generate a .dSYM unnecessarily.

It's a pity this "if mac_breakpad, set mac_real_dsym" clause has been copied
around.  Maybe we can replace this with an unconditional assignment and move the
"if mac_breakpad" into common.gypi?

It's also brittle that mac_real_dsym has to be turned on explicitly, because it
makes it easy to miss a dynamic library that should get Breakpad info.  How much
would it slow down the build to make mac_real_dsym=1 the default?

Mark

[noelallen1, 2011-12-10 04:41:39]

One issue to double check please, otherwise the GYP changes LGTM.

http://codereview.chromium.org/8824003/diff/9001/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/plugin.gyp (right):

http://codereview.chromium.org/8824003/diff/9001/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.gyp:117: ['OS=="mac" and
mac_breakpad_symbols==1', {
Up to this point, I believe you only used mac_breakpad_symbols when you where
already conditionally inside of OS=="mac".   I believe this may be uninitialized
on other platforms.
I am not sure it is legal to assume that the OS=="mac" will safely prevent
mac_breakpad_symbols from being dereferenced at the same unfolding depth.

[Lambros, 2011-12-12 19:58:47]

LGTM for remoting.gyp

[Mark Seaborn, 2011-12-12 21:52:33]

As discussed with Mark, I decided to switch around the Gyp options, to avoid the
problem with ffmpeg.gyp.

I'm leaving "mac_breakpad" as enabling dumping the symbols at build time.

The new Gyp option "mac_breakpad_compiled_in" compiles in the runtime support. 
It's an ugly name, but I decided against the name "mac_breakpad_runtime" for
this option because the latter is ambiguous (it builds in the runtime support
but doesn't necessarily enable it at runtime).

Mark

[Mark Mentovai, 2011-12-14 17:43:39]

LGTM

[commit-bot: I haz the power, 2011-12-14 17:50:22]

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

[commit-bot: I haz the power, 2011-12-14 19:10:57]

Change committed as 114468