Closed Bug 943505 Opened 7 years ago Closed 7 years ago
Use fallible allocation in ns
Zip Item Ptr _base::ns Zip Item Ptr _base
One of the top OOM crashers is the infallible alloc at http://hg.mozilla.org/releases/mozilla-release/annotate/d20d499b219f/modules/libjar/nsZipArchive.cpp#l1126 which is often 500k. See https://crash-stats.mozilla.com/report/index/d47de7ff-63cd-4f02-a649-d44c42131122 for an example. This may require punting some error checking up into callers, since this is a constructor.
(In reply to Benjamin Smedberg [:bsmedberg] from comment #0) > One of the top OOM crashers is the infallible alloc at > http://hg.mozilla.org/releases/mozilla-release/annotate/d20d499b219f/modules/ > libjar/nsZipArchive.cpp#l1126 which is often 500k. See > https://crash-stats.mozilla.com/report/index/d47de7ff-63cd-4f02-a649- > d44c42131122 for an example. > > This may require punting some error checking up into callers, since this is > a constructor. Hi Benjamin: I am interested in fixing this bug, are you available for mentoring?
use fallible allocation moz_malloc
Assignee: nobody → pylaurent1314
Attachment #8344511 - Flags: review?(benjamin)
Comment on attachment 8344511 [details] [diff] [review] bug943505.patch mAutoBuf is a nsAutoArrayPtr, which means that it is going to be cleaned up with operator delete, which potentially isn't the same allocator with moz_malloc. This should instead be using the fallible version of operator new, which would be I think: mAutoBuf = new (fallible_t()) uint8_t[size];
Attachment #8344511 - Flags: review?(benjamin) → review-
Thank you. I update the patch.
Oh, a small mistake, update again :)
Why did version 2 not work? I'd prefer the single-line version if possible.
Comment on attachment 8344679 [details] [diff] [review] bug943505-V3.patch I'm going to mark r- on this because I prefer version 2. You can have an r+ on that version, or feel free to re-request review on this version if there's a reason it has to be this way.
Attachment #8344679 - Flags: review?(benjamin) → review-
Thanks :) Patch update again. the version 2 is compiled error. Now this patch is good.
Comment on attachment 8345660 [details] [diff] [review] bug943505-Final.patch I will not ask why the extra set of parens was necessary ;-)
Attachment #8345660 - Flags: review?(benjamin) → review+
Comment on attachment 8345660 [details] [diff] [review] bug943505-Final.patch Actually though, this uses the fallible allocator but doesn't null-check. Don't you need to early-return if the allocation failed?
Attachment #8345660 - Flags: review+ → review-
Comment on attachment 8345914 [details] [diff] [review] bug943505-Final-V2.patch I had to verify that this would actually propagate, but http://hg.mozilla.org/mozilla-central/annotate/12ea03a70243/modules/libjar/nsZipArchive.cpp#l232 is the error checking which makes this correct. Thank you!
Attachment #8345914 - Flags: review?(benjamin) → review+
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.