Closed
Bug 943511
Opened 11 years ago
Closed 10 years ago
Use fallible allocation in nsSegmentedBuffer::AppendNewSegment
Categories
(Core :: XPCOM, defect)
Tracking
()
VERIFIED
FIXED
mozilla29
People
(Reporter: benjamin, Assigned: Dexter)
References
(Blocks 1 open bug)
Details
(Whiteboard: [mentor=benjamin@smedbergs.us][lang=c++][crashkill:P1])
Crash Data
Attachments
(2 files, 2 obsolete files)
4.79 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
20.49 KB,
patch
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•11 years ago
|
Whiteboard: [mentor=benjamin@smedbergs.us][lang=c++]
Reporter | ||
Updated•10 years ago
|
Whiteboard: [mentor=benjamin@smedbergs.us][lang=c++] → [mentor=benjamin@smedbergs.us][lang=c++][crashkill:P1]
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → alessio.placitelli
Assignee | ||
Comment 1•10 years ago
|
||
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 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
I'm sorry mccr8, I've erroneously added you as the reviewer. Thanks for fixing that.
Comment 4•10 years ago
|
||
No problem! You are welcome, and thanks for the patch.
Reporter | ||
Comment 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
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)
Reporter | ||
Comment 7•10 years ago
|
||
Yes, these should be all be changed to remove the allocator parameter.
Flags: needinfo?(benjamin)
Assignee | ||
Comment 9•10 years ago
|
||
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)
Reporter | ||
Comment 10•10 years ago
|
||
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)
Assignee | ||
Comment 11•10 years ago
|
||
I've removed all the allocator parameters from the classes using nsSegmentedBuffer.
Attachment #8345953 -
Flags: review?(benjamin)
Reporter | ||
Comment 12•10 years ago
|
||
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+
Reporter | ||
Comment 13•10 years ago
|
||
Oh, nsIStorageStream also.
Assignee | ||
Comment 14•10 years ago
|
||
Thank you very much for the review again and for all your suggestions.
Attachment #8345983 -
Flags: review?(benjamin)
Reporter | ||
Comment 15•10 years ago
|
||
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+
Reporter | ||
Comment 16•10 years ago
|
||
The last two patches should be folded together for final commit.
Assignee | ||
Comment 17•10 years ago
|
||
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)
Reporter | ||
Comment 18•10 years ago
|
||
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)
Assignee | ||
Comment 19•10 years ago
|
||
Yes, I've run the compiled Firefox and experimented with it. Works just fine on Windows 7 x64!
Reporter | ||
Comment 20•10 years ago
|
||
If you've made sure that TestPipe still passes (via `make check` in xpcom/tests) then set checkin-needed on the bug.
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 21•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/20b94cfce30c https://hg.mozilla.org/integration/mozilla-inbound/rev/d48fc2819410
Flags: in-testsuite+
Keywords: checkin-needed
Comment 22•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/20b94cfce30c https://hg.mozilla.org/mozilla-central/rev/d48fc2819410
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment 24•10 years ago
|
||
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.
status-firefox27:
--- → affected
status-firefox28:
--- → affected
tracking-firefox27:
--- → ?
tracking-firefox28:
--- → ?
Reporter | ||
Comment 25•10 years ago
|
||
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.
Comment 26•10 years ago
|
||
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)]
Comment 27•10 years ago
|
||
(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
Comment 28•10 years ago
|
||
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)
Reporter | ||
Comment 29•10 years ago
|
||
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.
Description
•