Use fallible allocation in nsSegmentedBuffer::AppendNewSegment

VERIFIED FIXED in mozilla29

Status

()

defect
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: benjamin, Assigned: Dexter)

Tracking

(Blocks 1 bug)

unspecified
mozilla29
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox27- wontfix, firefox28- wontfix)

Details

(Whiteboard: [mentor=benjamin@smedbergs.us][lang=c++][crashkill:P1], crash signature)

Attachments

(2 attachments, 2 obsolete attachments)

This is the most common OOM signature on Windows. Currently segmented buffers have a dynamic nsIMemory allocator via the constructor, but that's stupid and none of the clients appear to require it. So we should stop using the dynamic allocator and use fallible allocation for the buffers throughout. AppendNewSegment already returns null in normal situations (when we hit max-size), so I don't think there will be any problems with callers.

https://crash-stats.mozilla.com/report/index/39e1eeec-6a30-4677-b753-de49c2131122

http://hg.mozilla.org/releases/mozilla-release/annotate/d20d499b219f/xpcom/io/nsSegmentedBuffer.cpp#l71
Whiteboard: [mentor=benjamin@smedbergs.us][lang=c++]
Whiteboard: [mentor=benjamin@smedbergs.us][lang=c++] → [mentor=benjamin@smedbergs.us][lang=c++][crashkill:P1]
Assignee: nobody → alessio.placitelli
nsSegmentedBuffer::AppendNewSegment is now using fallible allocations (moz_malloc). Is this the right fallible allocator to use in this context?
Attachment #8344303 - Flags: review?(continuation)
Comment on attachment 8344303 [details] [diff] [review]
appendNewSegment.patch

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

Looks good to me, but bsmedberg should review.
Attachment #8344303 - Flags: review?(continuation) → review?(benjamin)
I'm sorry mccr8, I've erroneously added you as the reviewer. Thanks for fixing that.
No problem!  You are welcome, and thanks for the patch.
Comment on attachment 8344303 [details] [diff] [review]
appendNewSegment.patch

This patch is fine as far as it goes, but nsSegmentedBuffer::Init still takes an allocator parameter. Can you fix that as well (in another patch, if you like... this one is ready for checkin).
Attachment #8344303 - Flags: review?(benjamin) → review+
Thank you for the review. Removing the allocator parameter from the constructor influences nsStorageStream and nsPipe, which in turn take an allocator parameter in the Init function and pass it as the 3rd parameter for nsSegmentedBuffer::Init. 

Should I change those as well, by removing the allocator parameter from their Init functions? I just noticed those classes have a corresponding IDL file as well. Should it be changed?
Flags: needinfo?(benjamin)
Yes, these should be all be changed to remove the allocator parameter.
Flags: needinfo?(benjamin)
Duplicate of this bug: 842941
It seems to me the xpcom\tests\TestPipe.cpp test is made useless by this patch, since I will be using fallible allocations are removing the allocator parameters from the constructor. Am I missing something?
Flags: needinfo?(benjamin)
From reading the test, I think you can just remove all the allocator stuff and it will still test the right thing. The allocator bits are only there to check whether the pipe code uses the allocator, which is no longer necessary to test...
Flags: needinfo?(benjamin)
I've removed all the allocator parameters from the classes using nsSegmentedBuffer.
Attachment #8345953 - Flags: review?(benjamin)
Comment on attachment 8345953 [details] [diff] [review]
AllocatorParametersRemoved.patch

You need to make a new IID for the nsIPipe interface since you're changing it (use uuidgen or "/msg firebot uuid" on IRC). r=me with that change
Attachment #8345953 - Flags: review?(benjamin) → review+
Oh, nsIStorageStream also.
Posted patch IDL_UUIDs.patch (obsolete) — Splinter Review
Thank you very much for the review again and for all your suggestions.
Attachment #8345983 - Flags: review?(benjamin)
Comment on attachment 8345983 [details] [diff] [review]
IDL_UUIDs.patch

didn't need to review this again, but thanks, it looks good.
Attachment #8345983 - Flags: review?(benjamin) → review+
The last two patches should be folded together for final commit.
Folded the two last patches. Do I need to flag for a review this one as well?
Attachment #8345953 - Attachment is obsolete: true
Attachment #8345983 - Attachment is obsolete: true
Flags: needinfo?(benjamin)
No, we're good. Have you done a try run for these? They're pretty low risk but it might be worth just making sure they work on all platforms.
Flags: needinfo?(benjamin)
Yes, I've run the compiled Firefox and experimented with it. Works just fine on Windows 7 x64!
If you've made sure that TestPipe still passes (via `make check` in xpcom/tests) then set checkin-needed on the bug.
https://hg.mozilla.org/mozilla-central/rev/20b94cfce30c
https://hg.mozilla.org/mozilla-central/rev/d48fc2819410
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Duplicate of this bug: 764342
This can ride the trains. Note that usual STR here is just going to be "use all the memory and then crash". This bug is specifically about one place where we do variable-size allocation, but if we're crashing at other locations that are doing small fixed-size allocations, the crash itself is not a bug.
Moved signature of duped bug to here so it is correctly picked up by Socorro.
Status: RESOLVED → VERIFIED
Crash Signature: [@ mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xmalloc | nsMemoryImpl::Alloc(unsigned int)]
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #25)
> This can ride the trains. 

At what point would we re-evaluate this position? This is currently showing up on Firefox 28 at #13 (up 7 positions in the last week):
https://crash-stats.mozilla.com/report/list?product=Firefox&range_value=7&range_unit=days&date=2014-02-26&signature=mozalloc_abort%28char+const%2A+const%29+%7C+mozalloc_handle_oom%28unsigned+int%29+%7C+moz_xmalloc+%7C+nsSegmentedBuffer%3A%3AAppendNewSegment%28%29&version=Firefox%3A28.0b
I had a second look at this in terms of Crashes per Install over time and this looks less concerning than I originally thought.

Firefox 28 Beta:
 * 7 days before 2014-02-27: 1.064 crashes per install
 * 7 days before 2014-02-20: 1.084 crashes per install
 * 7 days before 2014-02-13: 1.092 crashes per install

It looks to me like the numbers are somewhat stable, if not declining. This doesn't change the fact that it's a topcrash but does make me more inclined to agree with letting this ride the trains.

Benjamin, can you check this to make sure I'm right in my assumptions?
Flags: needinfo?(benjamin)
You're correct. This can ride another train.
Flags: needinfo?(benjamin)
You need to log in before you can comment on or make changes to this bug.