Bug 18670 – fastMalloc should crash on failed allocs

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

fastMalloc should crash on failed allocs

We don't check the results of of malloc most of the time anyway.  We might as well make fastMalloc rash when an alloc fails.  Yes, we'd have to fix the few cases where we check for null, to instead do a range-check before passing it off to malloc.

[Darin Adler]

I agree completely that for callers that can't handle null it would be good to do something like abort() or CRASH().

Some callers do want to handle null; I'm not sure if there are any that are actually doing it. RenderTableSection::ensureRows is an example: it wants to return false if it can't grow the grid. But instead it should be designed so we don't need to allocate something based on the total number of rows.

Bug 9806 is one thing to look at. Another is <http://trac.webkit.org/projects/webkit/changeset/10959>.

[Mark Rowe (bdash)]

KJS::UString is a good example of code that already checks for failure of fastMalloc and fastRealloc.

[Eric Seidel (no email)]

Sure, but all of the above mentioned cases could be replaced by a pre-alloc check against some fixed size.  One could even provide a "safeMalloc", "mallocWhichMayFail" which did what the current fastMalloc does...  Every other time we assume that fastMalloc gives back a non-NULL pointer, or we crash.

[Mark Rowe (bdash)]

How can it be replaced by a check against a fixed size?  Allocating a very large number of a smaller strings can trigger a situation that will lead to fastMalloc failing just as easily as allocating a small number of very large strings.

[Eric Seidel (no email)]

(In reply to comment #4)
> How can it be replaced by a check against a fixed size?  Allocating a very
> large number of a smaller strings can trigger a situation that will lead to
> fastMalloc failing just as easily as allocating a small number of very large
> strings.

Sure, and that fastMalloc should fail (like it would today), and CRASH right then and there (like it should, but doesn't today).  The only "valid" fastMalloc failure conditions, are when fastMalloc is used as a integer size check for large user-provided data.  Those can be replaced with a check against some arbitrary fixed size.  ("WebCore does not support allocating more than 2G of data", or more likely a much smaller number.)

You end up with two (logical) allocators. One which does small allocations, and never fails (when it does, you CRASH then and there.)  A second, which could be used for larger allocations, but that you have to null check (what we have today).

Hum... I guess another way to look at this is to just turn fastMalloc into a wrapper function around a new fastMallocInternal() brave clients who are sure they will NULL check, can use the internal one.  Less brave clients get the wrapper by default, which CRASHs on NULL.

[Mark Rowe (bdash)]

(In reply to comment #5)
> (In reply to comment #4)
> > How can it be replaced by a check against a fixed size?  Allocating a very
> > large number of a smaller strings can trigger a situation that will lead to
> > fastMalloc failing just as easily as allocating a small number of very large
> > strings.
> 
> Sure, and that fastMalloc should fail (like it would today), and CRASH right
> then and there (like it should, but doesn't today). 

JavaScriptCore detects this case and propagates it to the calling script as an "Out of memory" exception.  How is crashing an improvement?

[Alexey Proskuryakov]

Fixed by Mitz in <http://trac.webkit.org/projects/webkit/changeset/35691>.

[David Kilzer (:ddkilzer)]

See <rdar://problem/6121636>.