Closed Bug 943505 Opened 6 years ago Closed 6 years ago

Use fallible allocation in nsZipItemPtr_base::nsZipItemPtr_base

Categories

(Core :: XPCOM, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: benjamin, Assigned: lpy)

References

(Blocks 1 open bug)

Details

(Whiteboard: [mentor=benjamin@smedbergs.us][lang=c++])

Attachments

(1 file, 4 obsolete files)

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?
Attached patch bug943505.patch (obsolete) — Splinter Review
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-
Attached patch bug943505-V2.patch (obsolete) — Splinter Review
Thank you. I update the patch.
Attachment #8344511 - Attachment is obsolete: true
Attachment #8344672 - Flags: review?(benjamin)
Attached patch bug943505-V3.patch (obsolete) — Splinter Review
Oh, a small mistake, update again :)
Attachment #8344672 - Attachment is obsolete: true
Attachment #8344672 - Flags: review?(benjamin)
Attachment #8344679 - Flags: review?(benjamin)
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-
Attached patch bug943505-Final.patch (obsolete) — Splinter Review
Thanks :) Patch update again. the version 2 is compiled error. Now this patch is good.
Attachment #8344679 - Attachment is obsolete: true
Attachment #8345660 - Flags: review?(benjamin)
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-
Thanks!
Attachment #8345660 - Attachment is obsolete: true
Attachment #8345914 - Flags: review?(benjamin)
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+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5ac5cccfd354
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.