Crash component: adds API for clients to handle the crash minidump.

Crash component: adds API for clients to handle the crash minidump.

R=rsesek@chromium.org
BUG=422613
Committed: https://crrev.com/353ee4a076787064adb454c02551d34680625753
Cr-Commit-Position: refs/heads/master@{#302849}

[gunsch, 2014-10-31 22:46:11]

gunsch@chromium.org changed reviewers:
+ rsesek@chromium.org
- resek@chromium.org

[gunsch, 2014-10-31 22:46:11]

gunsch@chromium.org changed required reviewers:
+ rsesek@chromium.org

[Robert Sesek, 2014-11-03 22:08:58]

Design-wise this is fine. Mac Breakpad has a similar notion of this with the
concept of "filters", but that's handled within Breakpad itself.

https://codereview.chromium.org/692093005/diff/1/components/crash/app/crash_r...
File components/crash/app/crash_reporter_client.h (right):

https://codereview.chromium.org/692093005/diff/1/components/crash/app/crash_r...
components/crash/app/crash_reporter_client.h:110: virtual bool
HandleCrashDump(const char* crashdump_filename);
This should be a base::FilePath.

[gunsch, 2014-11-03 22:11:41]

https://codereview.chromium.org/692093005/diff/1/components/crash/app/crash_r...
File components/crash/app/crash_reporter_client.h (right):

https://codereview.chromium.org/692093005/diff/1/components/crash/app/crash_r...
components/crash/app/crash_reporter_client.h:110: virtual bool
HandleCrashDump(const char* crashdump_filename);
On 2014/11/03 22:08:58, Robert Sesek wrote:
> This should be a base::FilePath.

Hmm, I thought the crash code went to pretty great lengths to not allocate
anything new during crash-handling time, even things as simple as std::string.
What makes FilePath okay in this case?

[Robert Sesek, 2014-11-03 22:19:02]

https://codereview.chromium.org/692093005/diff/1/components/crash/app/crash_r...
File components/crash/app/crash_reporter_client.h (right):

https://codereview.chromium.org/692093005/diff/1/components/crash/app/crash_r...
components/crash/app/crash_reporter_client.h:110: virtual bool
HandleCrashDump(const char* crashdump_filename);
On 2014/11/03 22:11:41, gunsch wrote:
> On 2014/11/03 22:08:58, Robert Sesek wrote:
> > This should be a base::FilePath.
> 
> Hmm, I thought the crash code went to pretty great lengths to not allocate
> anything new during crash-handling time, even things as simple as std::string.
> What makes FilePath okay in this case?

In the crashed context that's definitely true. Is this code going to run in a
crashed context? Are you going to enforce the non-allocating requirement on API
consumers?

[gunsch, 2014-11-03 23:40:16]

https://codereview.chromium.org/692093005/diff/1/components/crash/app/crash_r...
File components/crash/app/crash_reporter_client.h (right):

https://codereview.chromium.org/692093005/diff/1/components/crash/app/crash_r...
components/crash/app/crash_reporter_client.h:110: virtual bool
HandleCrashDump(const char* crashdump_filename);
On 2014/11/03 22:19:02, Robert Sesek wrote:
> On 2014/11/03 22:11:41, gunsch wrote:
> > On 2014/11/03 22:08:58, Robert Sesek wrote:
> > > This should be a base::FilePath.
> > 
> > Hmm, I thought the crash code went to pretty great lengths to not allocate
> > anything new during crash-handling time, even things as simple as
std::string.
> > What makes FilePath okay in this case?
> 
> In the crashed context that's definitely true. Is this code going to run in a
> crashed context? Are you going to enforce the non-allocating requirement on
API
> consumers?

The code path I'm invoking this from in breakpad_linux.cc runs in a compromised
context (CrashDone --> HandleCrashDump -->
CrashReporterClient::HandleCrashDump). I don't feel like there would be a way to
allow a content embedder to handle the crash dump outside of a crashed context.

I can document this fact (added), but I can't really "enforce" the requirement,
no?

[Robert Sesek, 2014-11-05 18:37:23]

LGTM

https://codereview.chromium.org/692093005/diff/1/components/crash/app/crash_r...
File components/crash/app/crash_reporter_client.h (right):

https://codereview.chromium.org/692093005/diff/1/components/crash/app/crash_r...
components/crash/app/crash_reporter_client.h:110: virtual bool
HandleCrashDump(const char* crashdump_filename);
On 2014/11/03 23:40:15, gunsch wrote:
> On 2014/11/03 22:19:02, Robert Sesek wrote:
> > On 2014/11/03 22:11:41, gunsch wrote:
> > > On 2014/11/03 22:08:58, Robert Sesek wrote:
> > > > This should be a base::FilePath.
> > > 
> > > Hmm, I thought the crash code went to pretty great lengths to not allocate
> > > anything new during crash-handling time, even things as simple as
> std::string.
> > > What makes FilePath okay in this case?
> > 
> > In the crashed context that's definitely true. Is this code going to run in
a
> > crashed context? Are you going to enforce the non-allocating requirement on
> API
> > consumers?
> 
> The code path I'm invoking this from in breakpad_linux.cc runs in a
compromised
> context (CrashDone --> HandleCrashDump -->
> CrashReporterClient::HandleCrashDump). I don't feel like there would be a way
to
> allow a content embedder to handle the crash dump outside of a crashed
context.

It depends on the crash type. A renderer crash is handled by the browser, for
example.

> I can document this fact (added), but I can't really "enforce" the
requirement,
> no?

You can't really enforce it, no. At some point, the allocator object in
HandleCrashDump may be a good parameter to pass, but it's not required yet.

[gunsch, 2014-11-05 18:40:07]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-11-05 18:40:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/692093005/20001

[commit-bot: I haz the power, 2014-11-05 19:42:33]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-11-05 19:43:19]

Patchset 2 (id:??) landed as
https://crrev.com/353ee4a076787064adb454c02551d34680625753
Cr-Commit-Position: refs/heads/master@{#302849}