[webkit-dev] Checking allocation failures [was: Re: Naked new considered harmful]

[Adam Barth]

On Wed, Aug 25, 2010 at 7:09 AM, Stephan Assmus <superstippi at gmx.de> wrote:
> Am 24.08.2010 19:46, schrieb Adam Barth:
>> One thing Darin and I discussed at WWDC (yes, this email has been a
>> long time coming) is better programming patterns to prevent memory
>> leaks.  As I'm sure you know, whenever you allocate a RefCounted
>> object, you need to call adoptRef to prevent a memory leak by adopting
>> the object's initial reference:
>>
>> adoptRef(new FancyRefCountedObject())
>>
>> We now have an ASSERT that enforces this programming pattern, so we
>> should be able to catch errors pretty easily.  Recently, Darin also
>> introduced an analogous function for OwnPtr:
>>
>> adoptPtr(new NiftyNonRefCountedObject())
>>
>> What adoptPtr does is shove the newly allocated object into a
>> PassOwnPtr, which together with OwnPtr, makes sure we don't leak the
>> object.  By using one of these two functions every time we call new,
>> it's easy to see that we don't have any memory leaks.  In the cases
>> where we have an intentional memory leak (e.g., for a static), please
>> use the leakPtr() member function to document the leak.
>>
>> In writing new code, please avoid using "naked" calls to new.  If
>> you're making an object that you expect to be heap-allocated, please
>> add a "create" method, similar to the create method we use for
>> RefCounted objects:
>>
>> static PassOwnPtr<NiftyNonRefCountedObject>  create(ParamClass* param)
>> {
>>     return adoptPtr(new NiftyNonRefCountedObject(param));
>> }
>>
>> You should also make the constructor non-public so folks are forced to
>> call the create method.  (Of course, stack-allocated objects should
>> have public constructors.)
>>
>> I'm going through WebCore and removing as many naked news and I can.
>> At some point, we'll add a stylebot rule to flag violations for new
>> code.  If you'd like to help out, pick a directory and change all the
>> calls to new to use adoptRef or adoptPtr, as appropriate.
>
> this reminds me that I've always been wondering about checks for allocation
> failure in WebCore (versus concerns about leaks). A plain call to new may
> throw an std::bad_alloc exception. If this is not considered, it may leave
> objects in an invalid state, even when using objects such as RefPtr to wrap
> arround allocations. I don't remember any specific place in the WebCore code
> where it would be a problem, I just don't remember seeing any code that
> deals with allocation failures. Is this a design choice somehow? If so, is
> there some online documentation/discussion about it? (Tried to google it but
> found nothing...)

Failed allocations in WebKit call CRASH().  This prevents us from
ending up in an inconsistent state.

Adam