Add optional file size limit for minidumps

Add optional file size limit for minidumps

When there are upwards of 200 threads in a crashing process, each having an
8KB stack, this can result in a huge, 1.8MB minidump file.  So I added a
parameter that, if set, can compel the minidump writer to dump less stack.
More specifically, if the writer expects to go over the limit (due to the
number of threads), then it will dump less of a thread's stack after the
first 20 threads.

There are two ways to specify the limit, depending on how you write minidumps:
1) If you call WriteMinidump() directly, there's now a version of the
   function that takes the minidump size limit as an argument.
2) If you use the ExceptionHandler class, the MinidumpDescriptor object you
   pass to it now has a set_size_limit() method you would call before
   passing it to the constructor.

BUG=chromium-os:31447, chromium:154546
TEST=Wrote a size-limit unittest; Ran unittests
Committed: https://code.google.com/p/google-breakpad/source/detail?r=1082

[Michael Krebs]

Could one (or both) of you please review this?  There's been an increase in
crashes with large minidumps on Chrome OS, which are a lot harder to track down
because our crash server throws away the too-large minidumps.  Thanks!

[Mark Mentovai]

I’m unable to perform reviews for at least the rest of this week.


On Sat, Oct 13, 2012 at 1:33 AM, <mkrebs@chromium.org> wrote:

> Reviewers: Mark Mentovai, Ted Mielczarek,
>
> Message:
> Could one (or both) of you please review this?  There's been an increase
> in crashes with large minidumps on Chrome OS, which are a lot harder to
> track down because our crash server throws away the too-large minidumps.
>  Thanks!
>
>
> Description:
> Add optional file size limit for minidumps
>
> When there are upwards of 200 threads in a crashing process, each having
> an
> 8KB stack, this can result in a huge, 1.8MB minidump file.  So I added a
> parameter that, if set, can compel the minidump writer to dump less
> stack.
> More specifically, if the writer expects to go over the limit (due to
> the
> number of threads), then it will dump less of a thread's stack after the
> first 20 threads.
>
> There are two ways to specify the limit, depending on how you write
> minidumps:
> 1) If you call WriteMinidump() directly, there's now a version of the
>    function that takes the minidump size limit as an argument.
> 2) If you use the ExceptionHandler class, the MinidumpDescriptor object
> you
>    pass to it now has a set_size_limit() method you would call before
>    passing it to the constructor.
>
> BUG=chromium-os:31447
> TEST=Wrote a size-limit unittest; Ran unittests
>
> Please review this at
https://breakpad.appspot.com/**487002/<https://breakpad.appspot.com/487002/>
>
> Affected files:
>   M     src/client/linux/handler/**exception_handler.cc
>   M     src/client/linux/handler/**minidump_descriptor.h
>   M     src/client/linux/minidump_**writer/minidump_writer.cc
>   M     src/client/linux/minidump_**writer/minidump_writer.h
>   M     src/client/linux/minidump_**writer/minidump_writer_**unittest.cc
>
>
>

[Michael Krebs]

Adding benchan@ as reviewer in case he can look at this CL this week...

[Michael Krebs]

Adding Ivan, as maybe he can take a look at this...

[Ivan Penkov]

LGTM

https://breakpad.appspot.com/487002/diff/3001/src/client/linux/handler/minidu...
File src/client/linux/handler/minidump_descriptor.h (right):

https://breakpad.appspot.com/487002/diff/3001/src/client/linux/handler/minidu...
src/client/linux/handler/minidump_descriptor.h:78: off_t size_limit() const {
return size_limit_; }
Don't you have to include sys/types.h in order to use type off_t?

[Michael Krebs]

Thanks, Ivan!

https://breakpad.appspot.com/487002/diff/3001/src/client/linux/handler/minidu...
File src/client/linux/handler/minidump_descriptor.h (right):

https://breakpad.appspot.com/487002/diff/3001/src/client/linux/handler/minidu...
src/client/linux/handler/minidump_descriptor.h:78: off_t size_limit() const {
return size_limit_; }
Good catch.  It compiled for me, so it must have been included by other headers.
 I went ahead and explicitly added it.