Add breakpad support to mojo shell apk.

Add breakpad support to mojo shell apk.

This enables breakpad and save minidump locally when the application crashes,
but at this point it doesn't upload those yet.

R=viettrungluu@chromium.org, tonyg@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/648e05901af0e916f60558cac248e0c8b3329a66

[qsr, 2015-07-09 14:58:53]

https://codereview.chromium.org/1227333002/diff/20001/shell/crash/breakpad_li...
File shell/crash/breakpad_linux.cc (right):

https://codereview.chromium.org/1227333002/diff/20001/shell/crash/breakpad_li...
shell/crash/breakpad_linux.cc:1: // Copyright 2013 The Chromium Authors. All
rights reserved.
This code is forked (and heavily simplified) from chromium, I didn't change the
copyright because o this.

[tonyg, 2015-07-09 15:40:24]

tonyg@chromium.org changed reviewers:
+ viettrungluu@chromium.org

[tonyg, 2015-07-09 15:41:03]

woot! This lg2m, but I feel like a new dependency at this level is something
that Trung should approve instead of me.

https://codereview.chromium.org/1227333002/diff/20001/DEPS
File DEPS (right):

https://codereview.chromium.org/1227333002/diff/20001/DEPS#newcode58
DEPS:58: 'src/breakpad/src':
I take it that something is hardcoded about this path that prevents us from
putting it into third_party?

https://codereview.chromium.org/1227333002/diff/20001/breakpad/BUILD.gn
File breakpad/BUILD.gn (right):

https://codereview.chromium.org/1227333002/diff/20001/breakpad/BUILD.gn#newcode1
breakpad/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights reserved.
If third_party works, this could go into //build/secondary

https://codereview.chromium.org/1227333002/diff/20001/shell/crash/breakpad_li...
File shell/crash/breakpad_linux.cc (right):

https://codereview.chromium.org/1227333002/diff/20001/shell/crash/breakpad_li...
shell/crash/breakpad_linux.cc:1: // Copyright 2013 The Chromium Authors. All
rights reserved.
On 2015/07/09 14:58:53, qsr wrote:
> This code is forked (and heavily simplified) from chromium, I didn't change
the
> copyright because o this.

Would there be any value in stating the original location in a comment in this
file?

[viettrungluu, 2015-07-09 15:47:49]

I thought this had previously been discussed, and it was decided that it was
premature.

I'm pretty sceptical that we want to bring in all of breakpad (which even
Chromium is trying to replace), especially for the sake of local crash dumps.

[qsr, 2015-07-09 16:11:34]

On 2015/07/09 15:47:49, viettrungluu wrote:
> I thought this had previously been discussed, and it was decided that it was
> premature.
> 
> I'm pretty sceptical that we want to bring in all of breakpad (which even
> Chromium is trying to replace), especially for the sake of local crash dumps.

The issue is that we have plenty of use case where we do not have the symbols
for the so crashing on the local machine.

The network service is one (and what made this CL started), but also when using
the CDN applications.

Without breakpad, we have no way to debug those crashes. Also, the dumps are
local only for this first CL, I plan to upload those to the crash servers next.

And about changing breakpad, we will follow chrome when the next library is
ready, but right now, it doesn't support linux and has no timeline for
supporting it: http://docs.crashpad.googlecode.com/git/doc/status.html

[viettrungluu, 2015-07-09 17:03:28]

On 2015/07/09 16:11:34, qsr wrote:
> On 2015/07/09 15:47:49, viettrungluu wrote:
> > I thought this had previously been discussed, and it was decided that it was
> > premature.
> > 
> > I'm pretty sceptical that we want to bring in all of breakpad (which even
> > Chromium is trying to replace), especially for the sake of local crash
dumps.
> 
> The issue is that we have plenty of use case where we do not have the symbols
> for the so crashing on the local machine.
> 
> The network service is one (and what made this CL started), but also when
using
> the CDN applications.
> 
> Without breakpad, we have no way to debug those crashes. Also, the dumps are
> local only for this first CL, I plan to upload those to the crash servers
next.
> 
> And about changing breakpad, we will follow chrome when the next library is
> ready, but right now, it doesn't support linux and has no timeline for
> supporting it: http://docs.crashpad.googlecode.com/git/doc/status.html

I think this is incurring too much cost, too early:

* In addition to the dependency (plus maintenance) costs, there's the trouble of
generating/uploading symbols (which either is either a direct human cost, or an
infrastructure cost).
* There's the question of how this will interact with multiprocess and
sandboxing.
* There's the question of how this should probably work, in a true mojo
universe.

So not lgtm. At the very least, we should wait until jamesr gets back to discuss
further.

[tonyg, 2015-07-09 17:13:48]

> I think this is incurring too much cost, too early:
> 
> * In addition to the dependency (plus maintenance) costs, there's the trouble
of
> generating/uploading symbols (which either is either a direct human cost, or
an
> infrastructure cost).

Getting symbols is the precise goal here, which I don't think is a premature
goal for our state. Currently a private repo I'm maintaining that consumes the
mojo sdk is hitting crashes in mojo applications that we have no symbols for or
reasonable way to debug. I'm not sure how we'll continue to polish things if we
can't debug. I respect your opinion that breakpad might be more complicated than
necessary to get symbols. But the only alternative we were able to think of is
to make the buildbots also upload debug binaries in addition to release binaries
(which also has a significant infra cost). The trouble with that approach is
that sometimes debug binaries can be so large that they don't run on constrained
android devices.

So if breakpad isn't going to fly right now, could you please help us brainstorm
another solution for us to symbolize these stacks?

[viettrungluu, 2015-07-09 17:34:11]

On 2015/07/09 17:13:48, tonyg wrote:
> > I think this is incurring too much cost, too early:
> > 
> > * In addition to the dependency (plus maintenance) costs, there's the
trouble
> of
> > generating/uploading symbols (which either is either a direct human cost, or
> an
> > infrastructure cost).
> 
> Getting symbols is the precise goal here, which I don't think is a premature
> goal for our state. Currently a private repo I'm maintaining that consumes the
> mojo sdk is hitting crashes in mojo applications that we have no symbols for
or
> reasonable way to debug. I'm not sure how we'll continue to polish things if
we
> can't debug. I respect your opinion that breakpad might be more complicated
than
> necessary to get symbols. But the only alternative we were able to think of is
> to make the buildbots also upload debug binaries in addition to release
binaries
> (which also has a significant infra cost). The trouble with that approach is
> that sometimes debug binaries can be so large that they don't run on
constrained
> android devices.

I don't think you mean "debug" versus "release". Our release binaries are built
with symbols, even if we only upload them after they've been stripped.

We should also upload the unstripped binaries, at least for
debugging/symbolification purposes. Note that doing so would be necessary to
make symbolification on the crash servers work (with breakpad). (Depending on
how you run your symbol upload pipeline, you might even want to keep the symbols
text file, taken by upload_syms, around, and that tends to be even bigger.)

> 
> So if breakpad isn't going to fly right now, could you please help us
brainstorm
> another solution for us to symbolize these stacks?

In what form are you getting the stacks?

[tonyg, 2015-07-13 16:49:01]

On 2015/07/09 17:34:11, viettrungluu wrote:
> On 2015/07/09 17:13:48, tonyg wrote:
> > > I think this is incurring too much cost, too early:
> > > 
> > > * In addition to the dependency (plus maintenance) costs, there's the
> trouble
> > of
> > > generating/uploading symbols (which either is either a direct human cost,
or
> > an
> > > infrastructure cost).
> > 
> > Getting symbols is the precise goal here, which I don't think is a premature
> > goal for our state. Currently a private repo I'm maintaining that consumes
the
> > mojo sdk is hitting crashes in mojo applications that we have no symbols for
> or
> > reasonable way to debug. I'm not sure how we'll continue to polish things if
> we
> > can't debug. I respect your opinion that breakpad might be more complicated
> than
> > necessary to get symbols. But the only alternative we were able to think of
is
> > to make the buildbots also upload debug binaries in addition to release
> binaries
> > (which also has a significant infra cost). The trouble with that approach is
> > that sometimes debug binaries can be so large that they don't run on
> constrained
> > android devices.
> 
> I don't think you mean "debug" versus "release". Our release binaries are
built
> with symbols, even if we only upload them after they've been stripped.
> 
> We should also upload the unstripped binaries, at least for
> debugging/symbolification purposes. Note that doing so would be necessary to
> make symbolification on the crash servers work (with breakpad). (Depending on
> how you run your symbol upload pipeline, you might even want to keep the
symbols
> text file, taken by upload_syms, around, and that tends to be even bigger.)

Sounds reasonable to me. Ben, would that work?

> 
> > 
> > So if breakpad isn't going to fly right now, could you please help us
> brainstorm
> > another solution for us to symbolize these stacks?
> 
> In what form are you getting the stacks?

Right now, they're output in logcat and a tombstone is saved. mojo devtools has
a "debugger" command that parses and symbolizes. I'm sure Ben or Przymek would
know more about the specifics.

[tonyg, 2015-07-13 16:49:16]

tonyg@chromium.org changed reviewers:
+ ppi@chromium.org

[qsr, 2015-07-15 09:24:06]

On 2015/07/13 16:49:01, tonyg wrote:
> On 2015/07/09 17:34:11, viettrungluu wrote:
> > On 2015/07/09 17:13:48, tonyg wrote:
> > > > I think this is incurring too much cost, too early:
> > > > 
> > > > * In addition to the dependency (plus maintenance) costs, there's the
> > trouble
> > > of
> > > > generating/uploading symbols (which either is either a direct human
cost,
> or
> > > an
> > > > infrastructure cost).
> > > 
> > > Getting symbols is the precise goal here, which I don't think is a
premature
> > > goal for our state. Currently a private repo I'm maintaining that consumes
> the
> > > mojo sdk is hitting crashes in mojo applications that we have no symbols
for
> > or
> > > reasonable way to debug. I'm not sure how we'll continue to polish things
if
> > we
> > > can't debug. I respect your opinion that breakpad might be more
complicated
> > than
> > > necessary to get symbols. But the only alternative we were able to think
of
> is
> > > to make the buildbots also upload debug binaries in addition to release
> > binaries
> > > (which also has a significant infra cost). The trouble with that approach
is
> > > that sometimes debug binaries can be so large that they don't run on
> > constrained
> > > android devices.
> > 
> > I don't think you mean "debug" versus "release". Our release binaries are
> built
> > with symbols, even if we only upload them after they've been stripped.
> > 
> > We should also upload the unstripped binaries, at least for
> > debugging/symbolification purposes. Note that doing so would be necessary to
> > make symbolification on the crash servers work (with breakpad). (Depending
on
> > how you run your symbol upload pipeline, you might even want to keep the
> symbols
> > text file, taken by upload_syms, around, and that tends to be even bigger.)
> 
> Sounds reasonable to me. Ben, would that work?

 We should talk more in person next week. To try to answer your question, it
will work when the problem happen on for a dev and is reproducible. We will then
have correct gdb support and we will be able to debug. But if it is not
reproducible, or not on happening for a developer, then breakpad would allow us
to get stack that we will not get with just having the symbols somewhere.

> 
> > 
> > > 
> > > So if breakpad isn't going to fly right now, could you please help us
> > brainstorm
> > > another solution for us to symbolize these stacks?
> > 
> > In what form are you getting the stacks?
> 
> Right now, they're output in logcat and a tombstone is saved. mojo devtools
has
> a "debugger" command that parses and symbolizes. I'm sure Ben or Przymek would
> know more about the specifics.

 The debugger can only parse and symbolize if the system has been able to walk
the stack, which is not always possible.

[qsr, 2015-07-27 14:06:53]

Trung -> Can you take a look.

https://codereview.chromium.org/1227333002/diff/20001/DEPS
File DEPS (right):

https://codereview.chromium.org/1227333002/diff/20001/DEPS#newcode58
DEPS:58: 'src/breakpad/src':
On 2015/07/09 15:41:03, tonyg wrote:
> I take it that something is hardcoded about this path that prevents us from
> putting it into third_party?

Nope, just that I followed what chromium is doing. Moved to third_party.

https://codereview.chromium.org/1227333002/diff/20001/breakpad/BUILD.gn
File breakpad/BUILD.gn (right):

https://codereview.chromium.org/1227333002/diff/20001/breakpad/BUILD.gn#newcode1
breakpad/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/07/09 15:41:03, tonyg wrote:
> If third_party works, this could go into //build/secondary

I did put this in //third_party/breakpad, with the sources in
//third_party/breakpad/src

https://codereview.chromium.org/1227333002/diff/20001/shell/crash/breakpad_li...
File shell/crash/breakpad_linux.cc (right):

https://codereview.chromium.org/1227333002/diff/20001/shell/crash/breakpad_li...
shell/crash/breakpad_linux.cc:1: // Copyright 2013 The Chromium Authors. All
rights reserved.
On 2015/07/09 15:41:03, tonyg wrote:
> On 2015/07/09 14:58:53, qsr wrote:
> > This code is forked (and heavily simplified) from chromium, I didn't change
> the
> > copyright because o this.
> 
> Would there be any value in stating the original location in a comment in this
> file?

Done.

[viettrungluu, 2015-07-27 20:28:36]

https://codereview.chromium.org/1227333002/diff/40001/shell/crash/BUILD.gn
File shell/crash/BUILD.gn (right):

https://codereview.chromium.org/1227333002/diff/40001/shell/crash/BUILD.gn#ne...
shell/crash/BUILD.gn:17: # Want these files on both Linux and Android.
I think you should just go ahead and call these files "breakpad.{cc,h}" instead.

https://codereview.chromium.org/1227333002/diff/40001/shell/crash/DEPS
File shell/crash/DEPS (right):

https://codereview.chromium.org/1227333002/diff/40001/shell/crash/DEPS#newcode1
shell/crash/DEPS:1: include_rules = [
I don't think we need this file, since we don't do DEPS checks.

https://codereview.chromium.org/1227333002/diff/40001/shell/crash/breakpad_li...
File shell/crash/breakpad_linux.cc (right):

https://codereview.chromium.org/1227333002/diff/40001/shell/crash/breakpad_li...
shell/crash/breakpad_linux.cc:28: #include "base/base_switches.h"
You shouldn't use switches from base_switches.*.

If you want to support them, you should add them to switches.{cc,h}, add them to
the list, document them, etc. (Currently, trying to pass them would result in
error.

https://codereview.chromium.org/1227333002/diff/40001/shell/crash/breakpad_li...
shell/crash/breakpad_linux.cc:29: #include "base/basictypes.h"
nit: Does it compile with including the non-deprecated base/macros.h instead of
base/basictypes.h?

https://codereview.chromium.org/1227333002/diff/40001/shell/crash/breakpad_li...
File shell/crash/breakpad_linux.h (right):

https://codereview.chromium.org/1227333002/diff/40001/shell/crash/breakpad_li...
shell/crash/breakpad_linux.h:9: #ifndef COMPONENTS_CRASH_APP_BREAKPAD_LINUX_H_
Please update the include guards.

https://codereview.chromium.org/1227333002/diff/40001/shell/crash/breakpad_li...
shell/crash/breakpad_linux.h:12: #include <string>
This include doesn't appear to be needed.

https://codereview.chromium.org/1227333002/diff/40001/shell/crash/breakpad_li...
shell/crash/breakpad_linux.h:15: #include "build/build_config.h"
"

https://codereview.chromium.org/1227333002/diff/40001/shell/crash/breakpad_li...
shell/crash/breakpad_linux.h:20: extern void InitCrashReporter(const
base::FilePath& dumps_path);
Why is this declared extern?

[qsr, 2015-07-28 08:46:23]

https://codereview.chromium.org/1227333002/diff/40001/shell/crash/BUILD.gn
File shell/crash/BUILD.gn (right):

https://codereview.chromium.org/1227333002/diff/40001/shell/crash/BUILD.gn#ne...
shell/crash/BUILD.gn:17: # Want these files on both Linux and Android.
On 2015/07/27 20:28:35, viettrungluu wrote:
> I think you should just go ahead and call these files "breakpad.{cc,h}"
instead.

Done.

https://codereview.chromium.org/1227333002/diff/40001/shell/crash/DEPS
File shell/crash/DEPS (right):

https://codereview.chromium.org/1227333002/diff/40001/shell/crash/DEPS#newcode1
shell/crash/DEPS:1: include_rules = [
On 2015/07/27 20:28:35, viettrungluu wrote:
> I don't think we need this file, since we don't do DEPS checks.

Done.

https://codereview.chromium.org/1227333002/diff/40001/shell/crash/breakpad_li...
File shell/crash/breakpad_linux.cc (right):

https://codereview.chromium.org/1227333002/diff/40001/shell/crash/breakpad_li...
shell/crash/breakpad_linux.cc:28: #include "base/base_switches.h"
On 2015/07/27 20:28:35, viettrungluu wrote:
> You shouldn't use switches from base_switches.*.
> 
> If you want to support them, you should add them to switches.{cc,h}, add them
to
> the list, document them, etc. (Currently, trying to pass them would result in
> error.

 Remove the switched for now. We can always add those back if we decide we need
those.

https://codereview.chromium.org/1227333002/diff/40001/shell/crash/breakpad_li...
shell/crash/breakpad_linux.cc:29: #include "base/basictypes.h"
On 2015/07/27 20:28:35, viettrungluu wrote:
> nit: Does it compile with including the non-deprecated base/macros.h instead
of
> base/basictypes.h?

It does. I didn't know basictypes was deprecated. Thanks.

https://codereview.chromium.org/1227333002/diff/40001/shell/crash/breakpad_li...
File shell/crash/breakpad_linux.h (right):

https://codereview.chromium.org/1227333002/diff/40001/shell/crash/breakpad_li...
shell/crash/breakpad_linux.h:9: #ifndef COMPONENTS_CRASH_APP_BREAKPAD_LINUX_H_
On 2015/07/27 20:28:36, viettrungluu wrote:
> Please update the include guards.

Done.

https://codereview.chromium.org/1227333002/diff/40001/shell/crash/breakpad_li...
shell/crash/breakpad_linux.h:12: #include <string>
On 2015/07/27 20:28:36, viettrungluu wrote:
> This include doesn't appear to be needed.

Done.

https://codereview.chromium.org/1227333002/diff/40001/shell/crash/breakpad_li...
shell/crash/breakpad_linux.h:15: #include "build/build_config.h"
On 2015/07/27 20:28:36, viettrungluu wrote:
> "

Done.

https://codereview.chromium.org/1227333002/diff/40001/shell/crash/breakpad_li...
shell/crash/breakpad_linux.h:20: extern void InitCrashReporter(const
base::FilePath& dumps_path);
On 2015/07/27 20:28:36, viettrungluu wrote:
> Why is this declared extern?

Because I missed this from the place it has been forked. Thanks. Removed.

[viettrungluu, 2015-07-28 16:45:06]

lgtm w/nits

https://codereview.chromium.org/1227333002/diff/60001/DEPS
File DEPS (right):

https://codereview.chromium.org/1227333002/diff/60001/DEPS#newcode159
DEPS:159: Var('chromium_git') + '/external/google-breakpad/src.git' + '@' +
'242fb9a38db6ba534b1f7daa341dd4d79171658b', # from svn revision 1471
nit: indentation

https://codereview.chromium.org/1227333002/diff/60001/shell/crash/breakpad.cc
File shell/crash/breakpad.cc (right):

https://codereview.chromium.org/1227333002/diff/60001/shell/crash/breakpad.cc...
shell/crash/breakpad.cc:744: // TODO(qsr): Is there any key we want to put.
nit: '?' instead of '.'

[qsr, 2015-07-29 08:49:31]

Committed patchset #5 (id:80001) manually as
648e05901af0e916f60558cac248e0c8b3329a66 (presubmit successful).

[qsr, 2015-07-29 09:00:47]

https://codereview.chromium.org/1227333002/diff/60001/DEPS
File DEPS (right):

https://codereview.chromium.org/1227333002/diff/60001/DEPS#newcode159
DEPS:159: Var('chromium_git') + '/external/google-breakpad/src.git' + '@' +
'242fb9a38db6ba534b1f7daa341dd4d79171658b', # from svn revision 1471
On 2015/07/28 16:45:05, viettrungluu wrote:
> nit: indentation

Done.

https://codereview.chromium.org/1227333002/diff/60001/shell/crash/breakpad.cc
File shell/crash/breakpad.cc (right):

https://codereview.chromium.org/1227333002/diff/60001/shell/crash/breakpad.cc...
shell/crash/breakpad.cc:744: // TODO(qsr): Is there any key we want to put.
On 2015/07/28 16:45:05, viettrungluu wrote:
> nit: '?' instead of '.'

Done.