943505 - Use fallible allocation in nsZipItemPtr_base::nsZipItemPtr_base

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Benjamin Smedberg]

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.

Comment 1

[Arun Khetarpal]

(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?

Comment 2

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug943505.patch

use fallible allocation moz_malloc

Comment 3

[Benjamin Smedberg]

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];

Comment 4

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug943505-V2.patch

Thank you. I update the patch.

Comment 5

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug943505-V3.patch

Oh, a small mistake, update again :)

Comment 6

[Benjamin Smedberg]

Why did version 2 not work? I'd prefer the single-line version if possible.

Comment 7

[Benjamin Smedberg]

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.

Comment 8

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug943505-Final.patch

Thanks :) Patch update again. the version 2 is compiled error. Now this patch is good.

Comment 9

[Benjamin Smedberg]

Comment on attachment 8345660 [details] [diff] [review]
bug943505-Final.patch

I will not ask why the extra set of parens was necessary ;-)

Comment 10

[Benjamin Smedberg]

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?

Comment 11

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug943505-Final-V2.patch

Thanks!

Comment 12

[Benjamin Smedberg]

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!

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ac5cccfd354

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5ac5cccfd354