mozalloc_handle_oom should propogate the failed allocation size into crash reports

RESOLVED FIXED in mozilla12

Status

()

Core
General
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: ted, Assigned: Benjamin Smedberg)

Tracking

Trunk
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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).
(Assignee)

Updated

6 years ago
Assignee: nobody → benjamin
(Assignee)

Comment 1

6 years ago
<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...
(Assignee)

Comment 2

6 years ago
Created attachment 587111 [details] [diff] [review]
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.
(Reporter)

Comment 3

6 years ago
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
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).
(Assignee)

Comment 5

6 years ago
Created attachment 590502 [details] [diff] [review]
Annotate using a static var, rev. 1

This totally ignores child processes but I'm pretty sure it should work for the main process.
Attachment #587111 - Attachment is obsolete: true
Attachment #590502 - Flags: review?(ted.mielczarek)
(Reporter)

Comment 6

6 years ago
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.
Attachment #590502 - Flags: review?(ted.mielczarek) → review-
(Assignee)

Comment 7

6 years ago
Created attachment 590880 [details] [diff] [review]
Annotate using a static var, rev. 2
Attachment #590502 - Attachment is obsolete: true
(Assignee)

Comment 8

6 years ago
Created attachment 591054 [details] [diff] [review]
Annotate using a static var, rev. 2.1

This time with feeling! (and tests that pass)
Attachment #590880 - Attachment is obsolete: true
Attachment #591054 - Flags: review?(ted.mielczarek)
(Reporter)

Comment 9

6 years ago
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.
Attachment #591054 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 10

6 years ago
Created attachment 591120 [details] [diff] [review]
google-breakpad patch
Attachment #591120 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 11

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d6cb73f892b
(Reporter)

Comment 12

6 years ago
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.
(Assignee)

Comment 13

6 years ago
I added this field to the request in bug 669108.
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/4d6cb73f892b
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Reporter)

Comment 15

6 years ago
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.
Attachment #591120 - Flags: review?(ted.mielczarek) → review+
(Reporter)

Comment 16

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

Updated

6 years ago
Blocks: 725024

Updated

6 years ago
Blocks: 733262

Updated

6 years ago
Blocks: 669108
Did we regress this? We now crash in nsTArrayInfallibleAllocator::SizeTooBig with no info on the size of the allocation.
(Assignee)

Comment 18

5 years ago
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.
You need to log in before you can comment on or make changes to this bug.