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

[Stephan Assmus]

Am 25.08.2010 23:34, schrieb Adam Barth:
> On Wed, Aug 25, 2010 at 2:31 PM, Stephan Assmus<superstippi at gmx.de>  wrote:
>> 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.
>
> Dunno.  The crash-on-allocation failure assumption is baked into lots
> of lines of code.

I just thought that if my observations are correct, and on the subject 
of advertising a certain way to write code (with regards to your initial 
email), perhaps new code (and eventually old code) should also follow a 
guideline that allows to handle allocation failures gracefully. For 
example, if no allocations are to be done in constructors, but rather 
within a dedicated init() method, objects remain always valid, even if 
init() throws half-way through, and they could be deallocated gracefully.

Best regards,
-Stephan