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

[Stephan Assmus]

Am 25.08.2010 18:35, schrieb 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.

Ok, but WebKit is used on devices where allocation failure is somewhat 
of a realistic possibility, no? Wouldn't it be desirable to handle such 
a situation gracefully? (I.e. display of an error message when trying to 
open one more tab rather than crashing the entire browser, for example.) 
Hopefully I am not missing something obvious.

Best regards,
-Stephan