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

[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.

Adam