716638 - mozalloc_handle_oom should propogate the failed allocation size into crash reports

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[(not currently active) Ted Mielczarek]

We should figure out a way to get the failed allocation size into crash reports from mozalloc_handle_oom. Currently it's hard to tell if we're OOMing from memory/address space exhaustion, or from a single large failed allocation (where we might want to change the caller to use fallible alloc).

Comment 1

[Benjamin Smedberg]

<bsmedberg> ted: I was going to implement the mozalloc abort size annotation
<bsmedberg> but then I noticed that if we're really low on memory at that point...
<bsmedberg> we shouldn't be calling AppendAppNotesToCrashReport because it allocates
<bsmedberg> unless all of AppendAppNotesToCrashReport is OOM-safe

Not sure what to do here...

Comment 2

[Benjamin Smedberg]

Attachment: Annotate with the size before crashing due to OOM, unsafe WIP

As noted in the patch, this is incomplete and unsafe and the whole approach is probably flawed, but I thought I'd attach it for record keeping.

Comment 3

[(not currently active) Ted Mielczarek]

I mentioned this in IRC, but I'll repeat it here:
What I'd probably do is just add a new API for it. Add a static  uint64_t OOMAllocationSize = 0; and SetOOMAllocationSize(size_t), and write it out specially like we do with a few other things:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/nsExceptionHandler.cpp#427

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Note that that interface technically needs to be thread-safe, but if we hit a small-alloc OOM then stomping that value from another small-alloc failure doesn't matter too much (as long as the read/write are atomic).

Comment 5

[Benjamin Smedberg]

Attachment: Annotate using a static var, rev. 1

This totally ignores child processes but I'm pretty sure it should work for the main process.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 590502 [details] [diff] [review]
Annotate using a static var, rev. 1

Review of attachment 590502 [details] [diff] [review]:
-----------------------------------------------------------------

::: memory/mozalloc/mozalloc_oom.cpp
@@ +60,5 @@
>      mozalloc_abort("out of memory");
>  }
> +
> +void
> +mozalloc_set_oom_abort_handler(mozalloc_oom_abort_handler handler)

I don't see anywhere in this patch that you actually call this.

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +333,5 @@
>  
>    return str;
>  }
>  
> +static size_t gOOMAllocationSize;

You want to initialize this, right?

@@ +478,5 @@
> +        _ui64toa(gOOMAllocationSize, buffer, 10);
> +        bufferLen = strlen(buffer);
> +        WriteFile(hFile, buffer, bufferlen, &nBytes, NULL);
> +        WriteFile(hFile, "\n", 1, &nBytes, NULL);
> +      }

You only handled the Windows case here, not the POSIX case.

Comment 7

[Benjamin Smedberg]

Attachment: Annotate using a static var, rev. 2

Comment 8

[Benjamin Smedberg]

Attachment: Annotate using a static var, rev. 2.1

This time with feeling! (and tests that pass)

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 591054 [details] [diff] [review]
Annotate using a static var, rev. 2.1

Review of attachment 591054 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/crashreporter/google-breakpad/src/common/linux/linux_libc_support.h
@@ +109,1 @@
>    if (!i)

This change needs to wind up upstream. Can you give me a patch against Breakpad SVN?

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +338,5 @@
>  
>    return str;
>  }
>  
> +static size_t gOOMAllocationSize;

Still needs to be initialized...

::: toolkit/crashreporter/test/unit/test_crash_oom.js
@@ +12,5 @@
> +    },
> +    function(mdump, extra) {
> +      do_check_eq(extra.TestingOOMCrash, "Yes");
> +      do_check_true("OOMAllocationSize" in extra);
> +      do_check_true(Number(extra.OOMAllocationSize) > 0);

Thanks for adding a test. I think you should also add a test that does a non-OOM crash, and checks that "OOMAllocationSize" is not present.

Comment 10

[Benjamin Smedberg]

Attachment: google-breakpad patch

Comment 11

[Benjamin Smedberg]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d6cb73f892b

Comment 12

[(not currently active) Ted Mielczarek]

You should probably file a Socorro bug about getting this exposed in the UI so we don't have to grovel around in the raw JSON for it.

Comment 13

[Benjamin Smedberg]

I added this field to the request in bug 669108.

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/4d6cb73f892b

Comment 15

[(not currently active) Ted Mielczarek]

Comment on attachment 591120 [details] [diff] [review]
google-breakpad patch

Review of attachment 591120 [details] [diff] [review]:
-----------------------------------------------------------------

I'll land this upstream for you in a minute.

Comment 16

[(not currently active) Ted Mielczarek]

http://code.google.com/p/google-breakpad/source/detail?r=909

Comment 17

[Gian-Carlo Pascutto [:gcp]]

Did we regress this? We now crash in nsTArrayInfallibleAllocator::SizeTooBig with no info on the size of the allocation.

Comment 18

[Benjamin Smedberg]

No, this has not regressed. nsTArrayInfallibleAllocator doesn't use the infallible malloc API but only calls mozalloc_abort directly, so there is no size information to put in the crash report. See also bug 811483 which intends to annotate whenever an allocation fails, regardless of whether it was an infallible alloc or not.