Closed Bug 943511 Opened 9 years ago Closed 9 years ago
Use fallible allocation in ns
Segmented Buffer::Append New Segment
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: [firstname.lastname@example.org][lang=c++] → [email@example.com][lang=c++][crashkill:P1]
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?
Yes, these should be all be changed to remove the allocator parameter.
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?
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...
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.
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?
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.
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.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
I can still reproduce the issue from bug 842941 on Firefox 27 beta 5. https://crash-stats.mozilla.com/report/index/bc731ee0-fa11-4ca5-90a3-0e43e2140110 https://crash-stats.mozilla.com/report/index/a949b79c-531d-440b-8464-529202140110 https://crash-stats.mozilla.com/report/index/8cfba5ca-4734-4e35-aacd-d55592140110 https://crash-stats.mozilla.com/report/index/f8cc3584-0fa8-4943-8cfe-bf44b2140110 https://crash-stats.mozilla.com/report/index/b15d4b0b-d4ab-4e20-9eb8-4a5702140110 Using latest Nightly I was unable to get the same crash signature but it still crashes using STR. https://crash-stats.mozilla.com/report/index/9c6df7eb-847b-4795-8b2e-74c2a2140110 STR: 1. Open http://pioul.fr/lolgl/ 2. Reload the page.
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?
You're correct. This can ride another train.
You need to log in before you can comment on or make changes to this bug.