18670 2008-04-21 14:26:57 -0700 fastMalloc should crash on failed allocs 2008-08-13 06:08:41 -0700 1 1 1 Unclassified WebKit Web Template Framework 528+ (Nightly build) Mac OS X 10.5 RESOLVED FIXED P2 Normal --- 1 eric webkit-unassigned ap darin ddkilzer mitz mjs mrowe sam oldest_to_newest 78473 0 eric 2008-04-21 14:26:57 -0700 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. 78478 1 darin 2008-04-21 15:35:01 -0700 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>. 78483 2 mrowe 2008-04-21 16:27:57 -0700 KJS::UString is a good example of code that already checks for failure of fastMalloc and fastRealloc. 78487 3 eric 2008-04-21 16:57:32 -0700 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. 78498 4 mrowe 2008-04-21 17:59:42 -0700 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. 78722 5 eric 2008-04-23 18:17:46 -0700 (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. 78725 6 mrowe 2008-04-23 19:17:21 -0700 (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? 88378 7 ap 2008-08-13 00:13:20 -0700 Fixed by Mitz in <http://trac.webkit.org/projects/webkit/changeset/35691>. 88391 8 ddkilzer 2008-08-13 06:08:41 -0700 See <rdar://problem/6121636>.